This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Do not invalidate the `this` pointer.
ClosedPublic

Authored by MTC on Apr 10 2018, 8:34 AM.

Details

Summary

this pointer is not an l-value, although we have modeled CXXThisRegion for this pointer, we can only bind it once, which is when we start to inline method. And this patch fixes https://bugs.llvm.org/show_bug.cgi?id=35506.

In addition, I didn't find any other cases other than loop-widen that could invalidate this pointer.

Diff Detail

Repository
rC Clang

Event Timeline

MTC created this revision.Apr 10 2018, 8:34 AM
MTC edited the summary of this revision. (Show Details)Apr 10 2018, 8:34 AM

@MTC what happens for

this.j = 0;
for (int i=0; i<100; i++)
   this.j++;

?

NoQ added inline comments.Apr 10 2018, 11:29 AM
lib/StaticAnalyzer/Core/RegionStore.cpp
1024–1027

I don't think this run-time check belongs here. The fix should be isolated in loop widening because everything else is already known to work correctly. The invalidation worker is not to be blamed for other entities throwing incorrect stuff into it.

2053–2056

This assertion is great to have.

Please use !isa<CXXThisRegion>(R) instead of CXXThisRegion::classof(R). The left argument of && is always true, so you can omit it.

2057

An accidental whitespace change here.

MTC added a comment.Apr 11 2018, 6:33 AM

@MTC what happens for

this.j = 0;
for (int i=0; i<100; i++)
   this.j++;

?

@george.karpenkov this's value will remain unchanged, j will be invalidated.

 1   void clang_analyzer_printState();
 2   struct A {
 3       int j;
 4       void foo() {
 5           this->j = 0;
 6           clang_analyzer_printState();
 7           for (int i = 0; i < 100; ++i)
 8               this->j++;
 9           clang_analyzer_printState();
10     }
11   };
12
13   void func() {
14       A a;
15       a.foo();
16   }

For the above code, given the command clang -cc1 -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-config widen-loops=true test.cpp. The output about Store is as follows.

Store (direct and default bindings), 0x7fb7d008c068 :
 (a,0,direct) : 0 S32b

 (this,0,direct) : &a

----------------------------------------------------------------------------
Store (direct and default bindings), 0x7fb7d080a618 :
 (a,0,default) : conj_$1{int}

 (this,0,direct) : &a
MTC marked 2 inline comments as done.Apr 11 2018, 6:41 AM
MTC added inline comments.
lib/StaticAnalyzer/Core/RegionStore.cpp
1024–1027

Make sense, will do.

2053–2056

Excellent advice, thank you! Will do.

MTC updated this revision to Diff 142026.Apr 11 2018, 8:59 AM
MTC marked 2 inline comments as done.
  • Move the CXXThisRegion's check to LoopWidening.cpp
  • Use isa<CXXThisRegion>(R) instead of CXXThisRegion::classof(R).
MTC marked an inline comment as done.Apr 11 2018, 8:59 AM
NoQ accepted this revision.Apr 13 2018, 8:27 PM

Yeah, i think this makes sense, thanks! It feels a bit weird that we have to add it as an exception - i wonder if there are other exceptions that we need to make. Widening over the stack memory space should be a whitelist, not a blacklist, because we can easily enumerate all stack variables and see which of them can be modified at all from the loop. But until we have that, this looks like a reasonable workaround.

This revision is now accepted and ready to land.Apr 13 2018, 8:27 PM
MTC added a comment.Apr 14 2018, 3:01 AM
In D45491#1067852, @NoQ wrote:

Yeah, i think this makes sense, thanks! It feels a bit weird that we have to add it as an exception - i wonder if there are other exceptions that we need to make. Widening over the stack memory space should be a whitelist, not a blacklist, because we can easily enumerate all stack variables and see which of them can be modified at all from the loop. But until we have that, this looks like a reasonable workaround.

You are right , it's really weird. And the better solution, D36690, seems to be blocked.

This revision was automatically updated to reflect the committed changes.