[analyzer] Should we invalidate the `this` pointer?

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[analyzer] Should we invalidate the `this` pointer?

Manuel Klimek via cfe-dev
Hi all,

I recently encountered a assertion failure as shown below.

`Assertion `!InitValWithAdjustments.getAs<Loc>() || Loc::isLocType(Result->getType()) || Result->getType()->isMemberPointerType()' failed`

The code that will trigger this assertion failed.
----------------------------------------------------------------------------------------------------
struct BlockId {
BlockId();
};

void goo(BlockId id);

BlockId::BlockId() {
int count = 10;
do {

} while (count--);
}

int main() {
goo(BlockId());
}
----------------------------------------------------------------------------------------------------

The reason is that the analyzer invalidate the `this` pointer at loop-widen. The more essential question is "Should we invalidate the `this` pointer?"

Thanks in advance!

Henry Wong
Qihoo 360 Codesafe Team

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [analyzer] Should we invalidate the `this` pointer?

Manuel Klimek via cfe-dev
This assertion is pretty fundamental. Invalidation, if done correctly,
should not have triggered it - after all, invalidation could occur for
any other reason, not necessarily because of loop widening.

Invalidation of this-region contents (that is, not of the
CXXThisObjectRegion of the current stack frame, but of the actual
this-region which is a pointee of the CXXThisObjectRegion) sounds
reasonable if the region is modified within the loop - which is going to
often be the case.

On 3/31/18 2:02 AM, Henry Wong via cfe-dev wrote:

> Hi all,
>
> I recently encountered a assertion failure as shown below.
>
> `Assertion `!InitValWithAdjustments.getAs<Loc>() ||
> Loc::isLocType(Result->getType()) ||
> Result->getType()->isMemberPointerType()' failed`
>
> The code that will trigger this assertion failed.
> ----------------------------------------------------------------------------------------------------
> struct BlockId {
> BlockId();
> };
>
> void goo(BlockId id);
>
> BlockId::BlockId() {
> int count = 10;
> do {
>
> } while (count--);
> }
>
> int main() {
> goo(BlockId());
> }
> ----------------------------------------------------------------------------------------------------
>
> The reason is that the analyzer invalidate the `this` pointer
> at loop-widen. The more essential question is "Should we invalidate
> the `this` pointer?"
>
> Thanks in advance!
>
> Henry Wong
> Qihoo 360 Codesafe Team
>
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [analyzer] Should we invalidate the `this` pointer?

Manuel Klimek via cfe-dev
Hi Artem,

Thank you for your explanation1 You are right, invalidation of the region 
contents of the class object is correct and common. However `this` pointer i
s no-lvalue and it's a `prvalue expression` in c++17. IMHO, invalidation of `CXXThisObjectRegion` is incorrect and violates the C++ standard.

Given the code below:
--------------------------------------------------------------------------------
   // $ clang -cc1 -analyze -analyzer-checker=core,debug.ExprInspection 
   //    -analyzer-config widen-loops=true test.cpp

  1 void clang_analyzer_eval(int);
  2
  3 struct A {
  4     int num;
  5     void func(int i) {
  6         int sum = 0;
  7         clang_analyzer_eval(sum == 0); // should be true
  8         for (i = 0; i < 100; ++i) { sum++; }
  9         num = 0;
 10     }
 11 };
 12
 13 int main() {
 14     A a;
 15     a.num = 10;
 16     a.func(10);
 17     clang_analyzer_eval(a.num == 0); // UNKNOWN, should be true.
 18 }
--------------------------------------------------------------------------------

Before invalidation,
--------------------------------------------------------------------------------
Store (direct and default bindings), 0x7f9de8014d90 :
(a,0,direct) : 10 S32b

(i,0,direct) : 3 S32b

(this,0,direct) : &a

(sum,0,direct) : 3 S32b
--------------------------------------------------------------------------------

After invalidation,
--------------------------------------------------------------------------------
Store (direct and default bindings), 0x7f9de8015828 :
(a,0,default) : conj_$2{int}

(i,0,direct) : conj_$3{int}

(this,0,direct) : &SymRegion{conj_$1{struct A *}}

(sum,0,direct) : conj_$0{int}
--------------------------------------------------------------------------------

`(this,0,direct) : &a` -> ` (this,0,direct) : &SymRegion{conj_$1{struct A *}}`
is  inaccurate and too conservative. The more serious problem is that the 
corresponding relationship between `this` pointer and its corresponding 
Object-Region has been broken. Modifications to data member do not affect 
the actual Object-Region because at this time `this` pointer is pointing to a
`SymbolicRegion`. For example, there should emit `TRUE` at the line 17 in 
the sample code, but emitted `UNKNOWN` instead.

Henry Wong
Qihoo 360 Codesafe Team

From: Artem Dergachev <[hidden email]>
Sent: Monday, April 2, 2018 8:31
To: Henry Wong; [hidden email]
Cc: Péter Szécsi
Subject: Re: [cfe-dev] [analyzer] Should we invalidate the `this` pointer?
 
This assertion is pretty fundamental. Invalidation, if done correctly,
should not have triggered it - after all, invalidation could occur for
any other reason, not necessarily because of loop widening.

Invalidation of this-region contents (that is, not of the
CXXThisObjectRegion of the current stack frame, but of the actual
this-region which is a pointee of the CXXThisObjectRegion) sounds
reasonable if the region is modified within the loop - which is going to
often be the case.

On 3/31/18 2:02 AM, Henry Wong via cfe-dev wrote:
> Hi all,
>
> I recently encountered a assertion failure as shown below.
>
> `Assertion `!InitValWithAdjustments.getAs<Loc>() ||
> Loc::isLocType(Result->getType()) ||
> Result->getType()->isMemberPointerType()' failed`
>
> The code that will trigger this assertion failed.
> ----------------------------------------------------------------------------------------------------
> struct BlockId {
> BlockId();
> };
>
> void goo(BlockId id);
>
> BlockId::BlockId() {
> int count = 10;
> do {
>
> } while (count--);
> }
>
> int main() {
> goo(BlockId());
> }
> ----------------------------------------------------------------------------------------------------
>
> The reason is that the analyzer invalidate the `this` pointer
> at loop-widen. The more essential question is "Should we invalidate
> the `this` pointer?"
>
> Thanks in advance!
>
> Henry Wong
> Qihoo 360 Codesafe Team
>
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [analyzer] Should we invalidate the `this` pointer?

Manuel Klimek via cfe-dev
Yeah, this looks pretty broken. One does not simply overwrite his this-pointer using valid C++. Feel free to fix :)

On 4/2/18 12:47 AM, Henry Wong wrote:
Hi Artem,

Thank you for your explanation1 You are right, invalidation of the region 
contents of the class object is correct and common. However `this` pointer i
s no-lvalue and it's a `prvalue expression` in c++17. IMHO, invalidation of `CXXThisObjectRegion` is incorrect and violates the C++ standard.

Given the code below:
--------------------------------------------------------------------------------
   // $ clang -cc1 -analyze -analyzer-checker=core,debug.ExprInspection 
   //    -analyzer-config widen-loops=true test.cpp

  1 void clang_analyzer_eval(int);
  2
  3 struct A {
  4     int num;
  5     void func(int i) {
  6         int sum = 0;
  7         clang_analyzer_eval(sum == 0); // should be true
  8         for (i = 0; i < 100; ++i) { sum++; }
  9         num = 0;
 10     }
 11 };
 12
 13 int main() {
 14     A a;
 15     a.num = 10;
 16     a.func(10);
 17     clang_analyzer_eval(a.num == 0); // UNKNOWN, should be true.
 18 }
--------------------------------------------------------------------------------

Before invalidation,
--------------------------------------------------------------------------------
Store (direct and default bindings), 0x7f9de8014d90 :
(a,0,direct) : 10 S32b

(i,0,direct) : 3 S32b

(this,0,direct) : &a

(sum,0,direct) : 3 S32b
--------------------------------------------------------------------------------

After invalidation,
--------------------------------------------------------------------------------
Store (direct and default bindings), 0x7f9de8015828 :
(a,0,default) : conj_$2{int}

(i,0,direct) : conj_$3{int}

(this,0,direct) : &SymRegion{conj_$1{struct A *}}

(sum,0,direct) : conj_$0{int}
--------------------------------------------------------------------------------

`(this,0,direct) : &a` -> ` (this,0,direct) : &SymRegion{conj_$1{struct A *}}`
is  inaccurate and too conservative. The more serious problem is that the 
corresponding relationship between `this` pointer and its corresponding 
Object-Region has been broken. Modifications to data member do not affect 
the actual Object-Region because at this time `this` pointer is pointing to a
`SymbolicRegion`. For example, there should emit `TRUE` at the line 17 in 
the sample code, but emitted `UNKNOWN` instead.

Henry Wong
Qihoo 360 Codesafe Team

From: Artem Dergachev [hidden email]
Sent: Monday, April 2, 2018 8:31
To: Henry Wong; [hidden email]
Cc: Péter Szécsi
Subject: Re: [cfe-dev] [analyzer] Should we invalidate the `this` pointer?
 
This assertion is pretty fundamental. Invalidation, if done correctly,
should not have triggered it - after all, invalidation could occur for
any other reason, not necessarily because of loop widening.

Invalidation of this-region contents (that is, not of the
CXXThisObjectRegion of the current stack frame, but of the actual
this-region which is a pointee of the CXXThisObjectRegion) sounds
reasonable if the region is modified within the loop - which is going to
often be the case.

On 3/31/18 2:02 AM, Henry Wong via cfe-dev wrote:
> Hi all,
>
> I recently encountered a assertion failure as shown below.
>
> `Assertion `!InitValWithAdjustments.getAs<Loc>() ||
> Loc::isLocType(Result->getType()) ||
> Result->getType()->isMemberPointerType()' failed`
>
> The code that will trigger this assertion failed.
> ----------------------------------------------------------------------------------------------------
> struct BlockId {
> BlockId();
> };
>
> void goo(BlockId id);
>
> BlockId::BlockId() {
> int count = 10;
> do {
>
> } while (count--);
> }
>
> int main() {
> goo(BlockId());
> }
> ----------------------------------------------------------------------------------------------------
>
> The reason is that the analyzer invalidate the `this` pointer
> at loop-widen. The more essential question is "Should we invalidate
> the `this` pointer?"
>
> Thanks in advance!
>
> Henry Wong
> Qihoo 360 Codesafe Team
>
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev