Thanks! On Mon, Aug 21, 2017 at 1:28 PM, Hans Wennborg via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> 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 >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
