[PATCH] D36851: [analyzer] Fix modeling of ctors

[Alexander Shaposhnikov via Phabricator via cfe-commits]

alexshap created this revision.
Herald added a subscriber: xazax.hun.

This diff attempts to fixe analyzer's crash (triggered assert) on the newly added test case.
The assert being discussed is assert(!B.lookup(R, BindingKey::Direct)) in lib/StaticAnalyzer/Core/RegionStore.cpp,
however the root cause appears to be different.
For classes with empty bases the offsets might be tricky.
For example, let's assume we have

  struct S: NonEmptyBase, EmptyBase {
     ...
  };

In this case Clang applies empty base class optimization and the offset of EmptyBase will be 0
(it can be verified via clang -cc1 -x c++ -v -fdump-record-layouts main.cpp -emit-llvm -o /dev/null).
When the analyzer tries to do zero initialization of EmptyBase it will hit the assert because that region has already been
"written" by the constructor of  NonEmptyBase.

Test plan:
make check-all


Repository:
  rL LLVM

https://reviews.llvm.org/D36851

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


Index: test/Analysis/ctor.mm
===================================================================
--- test/Analysis/ctor.mm
+++ test/Analysis/ctor.mm
@@ -704,3 +704,20 @@
     };
   }
 }
+
+namespace NoCrashOnEmptyBaseOptimization {
+  struct NonEmptyBase {
+    int X;
+    explicit NonEmptyBase(int X) : X(X) {}
+  };
+
+  struct EmptyBase {};
+
+  struct S : NonEmptyBase, EmptyBase {
+    S() : NonEmptyBase(0), EmptyBase() {}
+  };
+
+  void testSCtorNoCrash() {
+    S s;
+  }
+}
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -297,7 +297,17 @@
   ExplodedNodeSet PreInitialized;
   {
     StmtNodeBuilder Bldr(DstPreVisit, PreInitialized, *currBldrCtx);
-    if (CE->requiresZeroInitialization()) {
+    // FIXME: The offsets of empty bases can be tricky because of
+    // of the so called "empty base class optimization".
+    // If a base class has been optimized out
+    // we should not try to create a binding, otherwise we should.
+    // Unfortunately, at the moment ASTRecordLayout doesn't expose
+    // the actual sizes of the empty bases
+    // and trying to infer this from offsets/alignments
+    // seems to be error-prone and tricky because of the trailing padding.
+    // As a temporary mitigation we don't create a binding for empty bases.
+    if (CE->requiresZeroInitialization() &&
+        !CE->getConstructor()->getParent()->isEmpty()) {
       // Type of the zero doesn't matter.
       SVal ZeroVal = svalBuilder.makeZeroVal(getContext().CharTy);
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D36851.111580.patch
Type: text/x-patch
Size: 1648 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170817/e04280c6/attachment.bin>