[analyzer] Fix modeling of ctors

[alexander-shaposhnikov]

Summary

This diff attempts to fix 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

[dcoughlin]

Thanks for the patch!

@NoQ and I were discussing this approach this morning. One alternative we discussed was performing this logic in RegionStore instead and skipping the default binding there if we saw that the base region was empty. What do you think of that approach? (We would have to be careful for exactly the reasons described in your FIXME).

[alexander-shaposhnikov]

>One alternative we discussed was performing this logic in RegionStore instead and skipping the default binding there 
>if we saw that the base region was empty. What do you think of that approach? (We would have to be careful for exactly the reasons described in your FIXME)

yeah, i thought about this option as well, i can update my diff to try that and see how it works.

[alexander-shaposhnikov]

Skip the default binding for empty bases.

[NoQ]

Yeah, thanks!

After the FIXME about record layout is addressed, i guess the next action item is to make sure the trivial constructor for the empty base is not evaluated conservatively (doh!). Because whatever bindings we make, for now they'd be blown away immediately during "construction".

lib/StaticAnalyzer/Core/RegionStore.cpp

public: // Part of public interface to class.
   StoreRef Bind(Store store, Loc LV, SVal V) override {
     return StoreRef(bind(getRegionBindings(store), LV, V).asStore(), *this);
   }
 
   RegionBindingsRef bind(RegionBindingsConstRef B, Loc LV, SVal V);
 
   // BindDefault is only used to initialize a region with a default value.
   StoreRef BindDefault(Store store, const MemRegion *R, SVal V) override {
+    // 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 them from offsets/alignments
+    // seems to be error-prone and non-trivial because of the trailing padding.
+    // As a temporary mitigation we don't create bindings for empty bases.
+    if (R->getKind() == MemRegion::CXXBaseObjectRegionKind &&
+        cast<CXXBaseObjectRegion>(R)->getDecl()->isEmpty())
+      return StoreRef(store, *this);
+
     RegionBindingsRef B = getRegionBindings(store);
     assert(!B.lookup(R, BindingKey::Direct));
 
     BindingKey Key = BindingKey::Make(R, BindingKey::Default);
     if (B.lookup(Key)) {
       const SubRegion *SR = cast<SubRegion>(R);
       assert(SR->getAsOffset().getOffset() ==
              SR->getSuperRegion()->getAsOffset().getOffset() &&

test/Analysis/ctor.mm

struct S s = {
       C(),
       ({
         int a, b = 0;
         0;
       })
     };
   }
 }
+
+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;
+  }
+}