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

[MTC]

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.

[george.karpenkov]

@MTC what happens for

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

?

[NoQ]

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]

In D45491#1063364, @george.karpenkov wrote:

@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]

lib/StaticAnalyzer/Core/RegionStore.cpp

1024–1027

Make sense, will do.

2053–2056

Excellent advice, thank you! Will do.

[MTC]

• Move the CXXThisRegion's check to LoopWidening.cpp
• Use isa<CXXThisRegion>(R) instead of CXXThisRegion::classof(R).

[NoQ]

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.

[MTC]

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.

lib/StaticAnalyzer/Core/LoopWidening.cpp

ProgramStateRef getWidenedLoopState(ProgramStateRef PrevState,
   const MemRegion *Regions[] = {MRMgr.getStackLocalsRegion(STC),
                                 MRMgr.getStackArgumentsRegion(STC),
                                 MRMgr.getGlobalsRegion()};
   RegionAndSymbolInvalidationTraits ITraits;
   for (auto *Region : Regions) {
     ITraits.setTrait(Region,
                      RegionAndSymbolInvalidationTraits::TK_EntireMemSpace);
   }
+
+  // 'this' pointer is not an lvalue, we should not invalidate it. If the loop
+  // is located in a method, constructor or destructor, the value of 'this'
+  // pointer shoule remain unchanged.
+  if (const CXXMethodDecl *CXXMD = dyn_cast<CXXMethodDecl>(STC->getDecl())) {
+    const CXXThisRegion *ThisR = MRMgr.getCXXThisRegion(
+        CXXMD->getThisType(STC->getAnalysisDeclContext()->getASTContext()),
+        STC);
+    ITraits.setTrait(ThisR,
+                     RegionAndSymbolInvalidationTraits::TK_PreserveContents);
+  }
+
   return PrevState->invalidateRegions(Regions, getLoopCondition(LoopStmt),
                                       BlockCount, LCtx, true, nullptr, nullptr,
                                       &ITraits);
 }
 
 } // end namespace ento
 } // end namespace clang

lib/StaticAnalyzer/Core/RegionStore.cpp

if (Optional<nonloc::LazyCompoundVal> LCS =
 
     return;
   }
 }
 
 void invalidateRegionsWorker::VisitCluster(const MemRegion *baseR,
                                            const ClusterBindings *C) {
 
   bool PreserveRegionsContents =
       ITraits.hasTrait(baseR,
                        RegionAndSymbolInvalidationTraits::TK_PreserveContents);
 
   if (C) {
     for (ClusterBindings::iterator I = C->begin(), E = C->end(); I != E; ++I)
       VisitBinding(I.getData());
 
     // Invalidate regions contents.
     if (!PreserveRegionsContents)
       B = B.remove(baseR);
   }
if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(R)) {
     // to element 0.
     QualType T = SR->getSymbol()->getType();
     if (T->isAnyPointerType() || T->isReferenceType())
       T = T->getPointeeType();
 
     R = GetElementZeroRegion(SR, T);
   }
 
+  assert((!isa<CXXThisRegion>(R) || !B.lookup(R)) &&
+         "'this' pointer is not an l-value and is not assignable");
+
   // Clear out bindings that may overlap with this binding.
   RegionBindingsRef NewB = removeSubRegionBindings(B, cast<SubRegion>(R));
   return NewB.addBinding(BindingKey::Make(R, BindingKey::Direct), V);
 }
 
 RegionBindingsRef
 RegionStoreManager::setImplicitDefaultValue(RegionBindingsConstRef B,
                                             const MemRegion *R,
                                             QualType T) {
   SVal V;

test/Analysis/this-pointer.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config widen-loops=true -analyzer-disable-retry-exhausted -verify %s
+
+void clang_analyzer_eval(bool);
+void clang_analyzer_dump(int);
+
+// 'this' pointer is not an lvalue, we should not invalidate it.
+namespace this_pointer_after_loop_widen {
+class A {
+public:
+  A() {
+    int count = 10;
+    do {
+    } while (count--);
+  }
+};
+
+void goo(A a);
+void test_temporary_object() {
+  goo(A()); // no-crash
+}
+
+struct B {
+  int mem;
+  B() : mem(0) {
+    for (int i = 0; i < 10; ++i) {
+    }
+    mem = 0;
+  }
+};
+
+void test_ctor() {
+  B b;
+  clang_analyzer_eval(b.mem == 0); // expected-warning{{TRUE}}
+}
+
+struct C {
+  int mem;
+  C() : mem(0) {}
+  void set() {
+    for (int i = 0; i < 10; ++i) {
+    }
+    mem = 10;
+  }
+};
+
+void test_method() {
+  C c;
+  clang_analyzer_eval(c.mem == 0); // expected-warning{{TRUE}}
+  c.set();
+  clang_analyzer_eval(c.mem == 10); // expected-warning{{TRUE}}
+}
+
+struct D {
+  int mem;
+  D() : mem(0) {}
+  void set() {
+    for (int i = 0; i < 10; ++i) {
+    }
+    mem = 10;
+  }
+};
+
+void test_new() {
+  D *d = new D;
+  clang_analyzer_eval(d->mem == 0); // expected-warning{{TRUE}}
+  d->set();
+  clang_analyzer_eval(d->mem == 10); // expected-warning{{TRUE}}
+}
+
+struct E {
+  int mem;
+  E() : mem(0) {}
+  void set() {
+    for (int i = 0; i < 10; ++i) {
+    }
+    setAux();
+  }
+  void setAux() {
+    this->mem = 10;
+  }
+};
+
+void test_chained_method_call() {
+  E e;
+  e.set();
+  clang_analyzer_eval(e.mem == 10); // expected-warning{{TRUE}}
+}
+} // namespace this_pointer_after_loop_widen