r311182 - [analyzer] Fix modeling of constructors

[Hans Wennborg via cfe-commits]

I'm ok with it if Anna approves.

On Mon, Aug 21, 2017 at 9:06 AM, Artem Dergachev <noqnoqneo at gmail.com> wrote:
> Hello,
>
> Do we have time to merge this change into release 5.0.0? It's an assertion
> failure fix, which shows up on C++ code involving double-inheritance with
> empty base classes.
>
> Artem.
>
>
> On 8/18/17 9:20 PM, Alexander Shaposhnikov via cfe-commits wrote:
>>
>> Author: alexshap
>> Date: Fri Aug 18 11:20:43 2017
>> New Revision: 311182
>>
>> URL:http://llvm.org/viewvc/llvm-project?rev=311182&view=rev
>> Log:
>> [analyzer] Fix modeling of constructors
>>
>> This diff fixes 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 is
>> 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 perform 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
>>
>> Differential revision:https://reviews.llvm.org/D36851
>>
>> Modified:
>>      cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
>>      cfe/trunk/test/Analysis/ctor.mm
>>
>> Modified: cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
>>
>> URL:http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp?rev=311182&r1=311181&r2=311182&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp (original)
>> +++ cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp Fri Aug 18 11:20:43
>> 2017
>> @@ -409,6 +409,19 @@ public: // Part of public interface to c
>>       // 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));
>>
>> Modified: cfe/trunk/test/Analysis/ctor.mm
>>
>> URL:http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/ctor.mm?rev=311182&r1=311181&r2=311182&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/test/Analysis/ctor.mm (original)
>> +++ cfe/trunk/test/Analysis/ctor.mm Fri Aug 18 11:20:43 2017
>> @@ -704,3 +704,20 @@ namespace PR19579 {
>>       };
>>     }
>>   }
>> +
>> +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;
>> +  }
>> +}
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>