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
