<div dir="ltr">Thanks!</div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 21, 2017 at 1:28 PM, Hans Wennborg via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Merged in r311378.<br>
<br>
Thanks,<br>
Hans<br>
<div class="HOEnZb"><div class="h5"><br>
On Mon, Aug 21, 2017 at 1:12 PM, Anna Zaks <<a href="mailto:ganna@apple.com">ganna@apple.com</a>> wrote:<br>
> I approve.Thanks Hans!<br>
> Anna<br>
>> On Aug 21, 2017, at 1:05 PM, Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>> wrote:<br>
>><br>
>> I'm ok with it if Anna approves.<br>
>><br>
>> On Mon, Aug 21, 2017 at 9:06 AM, Artem Dergachev <<a href="mailto:noqnoqneo@gmail.com">noqnoqneo@gmail.com</a>> wrote:<br>
>>> Hello,<br>
>>><br>
>>> Do we have time to merge this change into release 5.0.0? It's an assertion<br>
>>> failure fix, which shows up on C++ code involving double-inheritance with<br>
>>> empty base classes.<br>
>>><br>
>>> Artem.<br>
>>><br>
>>><br>
>>> On 8/18/17 9:20 PM, Alexander Shaposhnikov via cfe-commits wrote:<br>
>>>><br>
>>>> Author: alexshap<br>
>>>> Date: Fri Aug 18 11:20:43 2017<br>
>>>> New Revision: 311182<br>
>>>><br>
>>>> URL:<a href="http://llvm.org/viewvc/llvm-project?rev=311182&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/<wbr>llvm-project?rev=311182&view=<wbr>rev</a><br>
>>>> Log:<br>
>>>> [analyzer] Fix modeling of constructors<br>
>>>><br>
>>>> This diff fixes analyzer's crash (triggered assert) on the newly added<br>
>>>> test case.<br>
>>>> The assert being discussed is assert(!B.lookup(R, BindingKey::Direct))<br>
>>>> in lib/StaticAnalyzer/Core/<wbr>RegionStore.cpp, however the root cause is<br>
>>>> different.<br>
>>>> For classes with empty bases the offsets might be tricky.<br>
>>>> For example, let's assume we have<br>
>>>>  struct S: NonEmptyBase, EmptyBase {<br>
>>>>      ...<br>
>>>>  };<br>
>>>> In this case Clang applies empty base class optimization and<br>
>>>> the offset of EmptyBase will be 0, it can be verified via<br>
>>>> clang -cc1 -x c++ -v -fdump-record-layouts main.cpp -emit-llvm -o<br>
>>>> /dev/null.<br>
>>>> When the analyzer tries to perform zero initialization of EmptyBase<br>
>>>> it will hit the assert because that region<br>
>>>> has already been "written" by the constructor of NonEmptyBase.<br>
>>>><br>
>>>> Test plan:<br>
>>>> make check-all<br>
>>>><br>
>>>> Differential revision:<a href="https://reviews.llvm.org/D36851" rel="noreferrer" target="_blank">https://reviews.llvm.<wbr>org/D36851</a><br>
>>>><br>
>>>> Modified:<br>
>>>>     cfe/trunk/lib/StaticAnalyzer/<wbr>Core/RegionStore.cpp<br>
>>>>     cfe/trunk/test/Analysis/<a href="http://ctor.mm" rel="noreferrer" target="_blank">ctor.<wbr>mm</a><br>
>>>><br>
>>>> Modified: cfe/trunk/lib/StaticAnalyzer/<wbr>Core/RegionStore.cpp<br>
>>>><br>
>>>> URL:<a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp?rev=311182&r1=311181&r2=311182&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/<wbr>llvm-project/cfe/trunk/lib/<wbr>StaticAnalyzer/Core/<wbr>RegionStore.cpp?rev=311182&r1=<wbr>311181&r2=311182&view=diff</a><br>
>>>><br>
>>>> ==============================<wbr>==============================<wbr>==================<br>
>>>> --- cfe/trunk/lib/StaticAnalyzer/<wbr>Core/RegionStore.cpp (original)<br>
>>>> +++ cfe/trunk/lib/StaticAnalyzer/<wbr>Core/RegionStore.cpp Fri Aug 18 11:20:43<br>
>>>> 2017<br>
>>>> @@ -409,6 +409,19 @@ public: // Part of public interface to c<br>
>>>>      // BindDefault is only used to initialize a region with a default<br>
>>>> value.<br>
>>>>    StoreRef BindDefault(Store store, const MemRegion *R, SVal V) override<br>
>>>> {<br>
>>>> +    // FIXME: The offsets of empty bases can be tricky because of<br>
>>>> +    // of the so called "empty base class optimization".<br>
>>>> +    // If a base class has been optimized out<br>
>>>> +    // we should not try to create a binding, otherwise we should.<br>
>>>> +    // Unfortunately, at the moment ASTRecordLayout doesn't expose<br>
>>>> +    // the actual sizes of the empty bases<br>
>>>> +    // and trying to infer them from offsets/alignments<br>
>>>> +    // seems to be error-prone and non-trivial because of the trailing<br>
>>>> padding.<br>
>>>> +    // As a temporary mitigation we don't create bindings for empty<br>
>>>> bases.<br>
>>>> +    if (R->getKind() == MemRegion::<wbr>CXXBaseObjectRegionKind &&<br>
>>>> +        cast<CXXBaseObjectRegion>(R)-><wbr>getDecl()->isEmpty())<br>
>>>> +      return StoreRef(store, *this);<br>
>>>> +<br>
>>>>      RegionBindingsRef B = getRegionBindings(store);<br>
>>>>      assert(!B.lookup(R, BindingKey::Direct));<br>
>>>><br>
>>>> Modified: cfe/trunk/test/Analysis/<a href="http://ctor.mm" rel="noreferrer" target="_blank">ctor.<wbr>mm</a><br>
>>>><br>
>>>> URL:<a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/ctor.mm?rev=311182&r1=311181&r2=311182&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/<wbr>llvm-project/cfe/trunk/test/<wbr>Analysis/ctor.mm?rev=311182&<wbr>r1=311181&r2=311182&view=diff</a><br>
>>>><br>
>>>> ==============================<wbr>==============================<wbr>==================<br>
>>>> --- cfe/trunk/test/Analysis/<a href="http://ctor.mm" rel="noreferrer" target="_blank">ctor.<wbr>mm</a> (original)<br>
>>>> +++ cfe/trunk/test/Analysis/<a href="http://ctor.mm" rel="noreferrer" target="_blank">ctor.<wbr>mm</a> Fri Aug 18 11:20:43 2017<br>
>>>> @@ -704,3 +704,20 @@ namespace PR19579 {<br>
>>>>      };<br>
>>>>    }<br>
>>>>  }<br>
>>>> +<br>
>>>> +namespace NoCrashOnEmptyBaseOptimization {<br>
>>>> +  struct NonEmptyBase {<br>
>>>> +    int X;<br>
>>>> +    explicit NonEmptyBase(int X) : X(X) {}<br>
>>>> +  };<br>
>>>> +<br>
>>>> +  struct EmptyBase {};<br>
>>>> +<br>
>>>> +  struct S : NonEmptyBase, EmptyBase {<br>
>>>> +    S() : NonEmptyBase(0), EmptyBase() {}<br>
>>>> +  };<br>
>>>> +<br>
>>>> +  void testSCtorNoCrash() {<br>
>>>> +    S s;<br>
>>>> +  }<br>
>>>> +}<br>
>>>><br>
>>>><br>
>>>> ______________________________<wbr>_________________<br>
>>>> cfe-commits mailing list<br>
>>>> <a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
>>>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
>>><br>
>>><br>
><br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div>