[PATCH] D43714: [analyzer] Don't do anything when trivial-copying an empty class object.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added a subscriber: rnkovacs.

When modeling implicit copy/move-constructor or copy/move-assignment operator of an empty class, don't do anything. The previous behavior was to take the value of the empty source object (which is always `UnknownVal` for empty classes) and assign it to the empty target object. This causes problems when the target object is a field or a base class because, due to how `RegionStore` forgets binding sizes, such `UnknownVal` would overwrite any existing store binding at the respective offset.

While testing temporary constructors and destructors, this resulted in numerous leak false positives when the raw pointer value in `unique_ptr` started disappearing from the program state when the zero-size `deleter` part of the smart pointer was trivially-copied at its offset. Note that `performTrivialCopy` doesn't cause pointer escape, because it normally doesn't need to.


https://reviews.llvm.org/D43714

Files:
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  test/Analysis/ctor.mm


Index: test/Analysis/ctor.mm
===================================================================
--- test/Analysis/ctor.mm
+++ test/Analysis/ctor.mm
@@ -729,3 +729,23 @@
     S s;
   }
 }
+
+namespace EmptyBaseAssign {
+struct B1 {};
+struct B2 { int x; };
+struct D: public B1, public B2 {
+const D &operator=(const D &d) {
+  *((B2 *)this) = d;
+  *((B1 *)this) = d;
+  return *this;
+}
+};
+
+void test() {
+  D d1;
+  d1.x = 1;
+  D d2;
+  d2 = d1;
+  clang_analyzer_eval(d2.x == 1); // expected-warning{{TRUE}}
+}
+}
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -42,19 +42,30 @@
                                     const CallEvent &Call) {
   SVal ThisVal;
   bool AlwaysReturnsLValue;
+  const CXXRecordDecl *ThisRD = nullptr;
   if (const CXXConstructorCall *Ctor = dyn_cast<CXXConstructorCall>(&Call)) {
     assert(Ctor->getDecl()->isTrivial());
     assert(Ctor->getDecl()->isCopyOrMoveConstructor());
     ThisVal = Ctor->getCXXThisVal();
+    ThisRD = Ctor->getDecl()->getParent();
     AlwaysReturnsLValue = false;
   } else {
     assert(cast<CXXMethodDecl>(Call.getDecl())->isTrivial());
     assert(cast<CXXMethodDecl>(Call.getDecl())->getOverloadedOperator() ==
            OO_Equal);
     ThisVal = cast<CXXInstanceCall>(Call).getCXXThisVal();
+    ThisRD = cast<CXXMethodDecl>(Call.getDecl())->getParent();
     AlwaysReturnsLValue = true;
   }
 
+  assert(ThisRD);
+  if (ThisRD->isEmpty()) {
+    // Do nothing for empty classes. Otherwise it'd retrieve an UnknownVal
+    // and bind it and RegionStore would think that the actual value
+    // in this region at this offset is unknown.
+    return;
+  }
+
   const LocationContext *LCtx = Pred->getLocationContext();
 
   ExplodedNodeSet Dst;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D43714.135771.patch
Type: text/x-patch
Size: 1884 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180224/1262362c/attachment.bin>