Merged in r311378. Thanks, Hans
On Mon, Aug 21, 2017 at 1:12 PM, Anna Zaks <ga...@apple.com> wrote: > I approve.Thanks Hans! > Anna >> On Aug 21, 2017, at 1:05 PM, Hans Wennborg <h...@chromium.org> wrote: >> >> I'm ok with it if Anna approves. >> >> On Mon, Aug 21, 2017 at 9:06 AM, Artem Dergachev <noqnoq...@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@lists.llvm.org >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>> >>> > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits