r311182 - [analyzer] Fix modeling of constructors

[Alexander Shaposhnikov via cfe-commits]

Thanks!

On Mon, Aug 21, 2017 at 1:28 PM, Hans Wennborg via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> 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
> >>>
> >>>
> >
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170821/7bf288e3/attachment-0001.html>