<div dir="ltr">LGTM.<div><br></div><div>I'm terribly sorry it took me so long to get to it =/. Semantic change looks fine to me as well, and I don't think there's an easy way (for now) to learn whether we're still constructing virtual table from UBSan runtime.</div><div>I'll ask Hans to include this into 3.7. Let me know if I should submit it for you.</div><div><br></div><div>Thanks for tracking this down!</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jul 16, 2015 at 2:59 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I just looked at the Itanium C++ ABI to confirm that positive<br>
offsets are legal. It looks like the are explicitly called out in<br>
the "virtual table components and order" section, under the "i"<br>
bubble of the third bullet point.<br>
<br>
<a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__mentorembedded.github.io_cxx-2Dabi_abi.html-23vtable-2Dcomponents&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=70LfZBJvhHolHG1uJvjDiXt-KMtvRNtt5IIvNIqTdKw&s=q2SXxj1TLcIxUFw5wBrp7Rtn_Buu2b--ieV6-uLkDyA&e=" rel="noreferrer" target="_blank">https://mentorembedded.github.io/cxx-abi/abi.html#vtable-components</a><br>
<br>
> - The offset to top holds the displacement to the top of the object from the location within the object of the virtual table pointer that addresses this virtual table, as a ptrdiff_t. It is always present. The offset provides a way to find the top of the object from any base subobject with a virtual table pointer. This is necessary for dynamic_cast<void*> in particular.<br>
><br>
> (i) In a complete object virtual table, and therefore in all of its primary base virtual tables, the value of this offset will be zero. For the secondary virtual tables of other non-virtual bases, and of many virtual bases, it will be negative. Only in some construction virtual tables will some virtual base virtual tables have positive offsets, due to a different ordering of the virtual bases in the full object than in the subobject's standalone layout.<br>
<br>
<br>
(The last sentence is the key one.)<br>
<br>
If that's what the review is waiting on, I'm comfortable LGTM'ing<br>
the semantic change. It's clearly a false positive match. If the<br>
runtime has some way of knowing whether we're inside construction<br>
or destruction, I imagine a more limited form of the check could be<br>
reintroduced.<br>
<br>
I don't know UBSan at all, so maybe Alexei wants to confirm whether<br>
the patch itself is fine (looks okay to me).<br>
<span class=""><br>
> On 2015-Jul-16, at 02:02, Stephan Bergmann <<a href="mailto:sbergman@redhat.com">sbergman@redhat.com</a>> wrote:<br>
><br>
> ping^8<br>
><br>
> On 07/14/2015 08:45 AM, Stephan Bergmann wrote:<br>
>> ping^7<br>
>><br>
>> It would be great if somebody could get this patch pushed. (It unbreaks<br>
>> UBSan builds of LibreOffice, and it is somewhat painful to maintain as a<br>
>> local patch on all the machines I use for such builds.)<br>
>><br>
>> I updated the attached getVtablesPrefix.patch to current trunk, and also<br>
>> added the test case from my original mail to it now.<br>
>><br>
>> Thanks,<br>
>> Stephan<br>
>><br>
>> On 07/03/2015 09:22 AM, Stephan Bergmann wrote:<br>
>>> ping^6<br>
>>><br>
>>> On 06/26/2015 08:57 AM, Stephan Bergmann wrote:<br>
>>>> ping1^5<br>
>>>><br>
>>>> On 06/19/2015 02:02 PM, Stephan Bergmann wrote:<br>
>>>>> ping^4<br>
>>>>><br>
>>>>> On 06/05/2015 07:06 PM, David Blaikie wrote:<br>
</span><span class="">>>>>>> On Fri, Jun 5, 2015 at 10:02 AM, Alexey Samsonov <<a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a><br>
>>>>>> <mailto:<a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a>>> wrote:<br>
>>>>>><br>
>>>>>> I referred to David Majnemer, who touched this code a while ago.<br>
>>>>>> But<br>
>>>>>> thanks for suggesting help :)<br>
>>>>>><br>
>>>>>><br>
>>>>>> Ah, +Majnemer.<br>
>>>>>><br>
>>>>>> - Dave<br>
>>>>>><br>
>>>>>><br>
</span><span class="">>>>>>> On Fri, Jun 5, 2015 at 8:26 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a><br>
>>>>>> <mailto:<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>>> wrote:<br>
>>>>>><br>
>>>>>><br>
>>>>>><br>
>>>>>> On Thu, Jun 4, 2015 at 5:52 PM, Alexey Samsonov<br>
</span><span class="">>>>>>> <<a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a> <mailto:<a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a>>> wrote:<br>
>>>>>><br>
>>>>>> Richard or David, do you want to look into this, or you'd<br>
>>>>>> prefer to leave this for me?<br>
>>>>>><br>
>>>>>><br>
>>>>>> I can't say I have much more context in vtable layout & C++<br>
>>>>>> ABI<br>
>>>>>> than you do, most likely. Happy to bounce some the ideas<br>
>>>>>> around<br>
>>>>>> in person if that's helpful.<br>
>>>>>><br>
>>>>>> - David<br>
>>>>>><br>
>>>>>><br>
>>>>>> On Tue, Jun 2, 2015 at 3:32 AM, Stephan Bergmann<br>
</span><span class="">>>>>>> <<a href="mailto:sbergman@redhat.com">sbergman@redhat.com</a> <mailto:<a href="mailto:sbergman@redhat.com">sbergman@redhat.com</a>>> wrote:<br>
>>>>>><br>
>>>>>> ping^3<br>
>>>>>><br>
>>>>>><br>
>>>>>> On 12/16/2014 10:52 AM, Stephan Bergmann wrote:<br>
>>>>>><br>
>>>>>> ping<br>
>>>>>> On 12/05/2014 09:33 AM, Stephan Bergmann wrote:<br>
>>>>>><br>
>>>>>> ping<br>
>>>>>> On 08/12/2014 09:10 PM, Alexey Samsonov wrote:<br>
>>>>>><br>
>>>>>> +Richard<br>
>>>>>><br>
>>>>>><br>
>>>>>> On Tue, Aug 12, 2014 at 3:51 AM, Stephan<br>
</span>>>>>>> Bergmann <<a href="mailto:sbergman@redhat.com">sbergman@redhat.com</a><br>
>>>>>> <mailto:<a href="mailto:sbergman@redhat.com">sbergman@redhat.com</a>><br>
>>>>>> <mailto:<a href="mailto:sbergman@redhat.com">sbergman@redhat.com</a><br>
<span class="">>>>>>> <mailto:<a href="mailto:sbergman@redhat.com">sbergman@redhat.com</a>>>> wrote:<br>
>>>>>><br>
>>>>>> On 08/11/2014 10:19 PM, Alexey<br>
>>>>>> Samsonov<br>
>>>>>> wrote:<br>
>>>>>><br>
>>>>>> +Richard<br>
>>>>>><br>
>>>>>> Note, that you'd also have to<br>
>>>>>> update comment for<br>
>>>>>> VtablePrefix::Offset field.<br>
>>>>>><br>
>>>>>><br>
>>>>>> ah, right; updated patch<br>
>>>>>><br>
>>>>>> Stephan<br>
>>>>>><br>
>>>>>> On Mon, Aug 11, 2014 at 6:30 AM,<br>
>>>>>> Stephan Bergmann<br>
</span><span class="">>>>>>> <<a href="mailto:sbergman@redhat.com">sbergman@redhat.com</a><br>
>>>>>> <mailto:<a href="mailto:sbergman@redhat.com">sbergman@redhat.com</a>><br>
>>>>>> <mailto:<a href="mailto:sbergman@redhat.com">sbergman@redhat.com</a><br>
>>>>>> <mailto:<a href="mailto:sbergman@redhat.com">sbergman@redhat.com</a>>><br>
>>>>>> <mailto:<a href="mailto:sbergman@redhat.com">sbergman@redhat.com</a><br>
>>>>>> <mailto:<a href="mailto:sbergman@redhat.com">sbergman@redhat.com</a>><br>
>>>>>> <mailto:<a href="mailto:sbergman@redhat.com">sbergman@redhat.com</a><br>
</span>>>>>>> <mailto:<a href="mailto:sbergman@redhat.com">sbergman@redhat.com</a>>>>><br>
<div><div class="h5">>>>>>> wrote:<br>
>>>>>><br>
>>>>>> At least with recent Clang<br>
>>>>>> trunk on Linux x86_64:<br>
>>>>>><br>
>>>>>> $ cat test.cc<br>
>>>>>> #include <iostream><br>
>>>>>> struct A { virtual ~A()<br>
>>>>>> {} };<br>
>>>>>> struct B: virtual A {};<br>
>>>>>> struct C: virtual A {<br>
>>>>>> ~C()<br>
>>>>>> { std::cout << '\n'; } };<br>
>>>>>> struct D: virtual B,<br>
>>>>>> virtual C {};<br>
>>>>>> int main() { delete new<br>
>>>>>> D; }<br>
>>>>>><br>
>>>>>> $ clang++<br>
>>>>>> -fsanitize=undefined test.cc<br>
>>>>>><br>
>>>>>> $ ./a.out<br>
>>>>>> <unknown>: runtime<br>
>>>>>> error:<br>
>>>>>> member call on address<br>
>>>>>> 0x000002a35010<br>
>>>>>> which does not point<br>
>>>>>> to an<br>
>>>>>> object of type 'A'<br>
>>>>>> 0x000002a35010: note:<br>
>>>>>> object has invalid vptr<br>
>>>>>> 00 00 00 00 58 0e<br>
>>>>>> 43 00<br>
>>>>>> 00 00 00 00 30 0e 43 00 00<br>
>>>>>> 00 00 00<br>
>>>>>> 00 00 00 00 00 00<br>
>>>>>> 00 00<br>
>>>>>> e1 0f 02 00<br>
>>>>>><br>
>>>>>> ^~~~~~~~~~~~~~~~~~~~~~~<br>
>>>>>> invalid<br>
>>>>>> vptr<br>
>>>>>><br>
>>>>>><br>
>>>>>> The problem is that<br>
>>>>>> getVtablePrefix<br>
>>>>>> (lib/ubsan/ubsan_type_hash.cc)<br>
>>>>>> rejects any VtablePrefix<br>
>>>>>> with<br>
>>>>>> Offset > 0 as "This can't<br>
>>>>>> possibly be<br>
>>>>>> a valid vtable" but,<br>
>>>>>> according<br>
>>>>>> to the Itanium ABI, "in some<br>
>>>>>> construction virtual tables<br>
>>>>>> will some virtual base virtual<br>
>>>>>> tables<br>
>>>>>> have positive offsets."<br>
>>>>>><br>
>>>>>> The apparent fix is to<br>
>>>>>> remove<br>
>>>>>> the check, see the attached<br>
>>>>>> getVtablePrefix.patch.<br>
</div></div>> <getVtablesPrefix.patch>_______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr">Alexey Samsonov<br><a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a></div></div>
</div>