r311182 - [analyzer] Fix modeling of constructors

[Hans Wennborg via cfe-commits]

Merged in r311378.

Thanks,
Hans

On Mon, Aug 21, 2017 at 1:12 PM, Anna Zaks <ganna at apple.com> wrote:
> I approve.Thanks Hans!
> Anna
>> On Aug 21, 2017, at 1:05 PM, Hans Wennborg <hans at chromium.org> wrote:
>>
>> 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
>>>
>>>
>