[PATCH] D19668: [ubsan] Minimize size of data for type_mismatch

[Kostya Serebryany via llvm-commits]

On Thu, May 26, 2016 at 3:38 PM, Filipe Cabecinhas <
filcab+llvm.phabricator at gmail.com> wrote:

> I'm ok with that. But I'd rather try to make it work now (since we end up
> with a runtime lib that can handle both types of objects), and only pull
> that trigger when we really need to.


By "work now" you mean adding quite a bit of code needed to support both
versions?

>
> If you still think it's best to just add a version check, I can submit a
> patch to do it anyway.
>

Yes, please. I do not want to blow up the code with multiple API versions.


>
> Thank you,
>
>  Filipe
>
>
> On Thursday, 26 May 2016, Kostya Serebryany <kcc at google.com> wrote:
>
>>
>>
>> On Thu, May 26, 2016 at 2:32 PM, Filipe Cabecinhas <
>> filcab+llvm.phabricator at gmail.com> wrote:
>>
>>> I'm not sure what you mean here. Could you be more explicit?
>>>
>>> To make sure we're on the same page: if we don't even try to account for
>>> the difference in version, you can end up with lots of false positive
>>> errors, but these will be printed as regular errors. We won't be able to
>>> diagnose the older version unless we add a field to check for this.
>>>
>>
>> That's clearly unacceptable. So, the only reasonable thing to do here is
>> to ensure at link-time that the versions (compiler+run-time) match (as in
>> asan)
>>
>>
>>>
>>>
>>> On Thursday, 26 May 2016, Kostya Serebryany <kcc at google.com> wrote:
>>>
>>>>
>>>>
>>>> On Thu, May 26, 2016 at 11:49 AM, Filipe Cabecinhas <
>>>> filcab+llvm.phabricator at gmail.com> wrote:
>>>>
>>>>> Things will break loudly:
>>>>
>>>>
>>>>
>>>> Ideally, we should fail to link or at the very least verbosely fail at
>>>> startup.
>>>> Failing loudly when there is a bug report is the worst option.
>>>> But if you can make it verbose enough, let's try.
>>>>
>>>>
>>>>>
>>>>> Newer object file with older lib:
>>>>> Out of bounds access guaranteed (strict became smaller), TypeCheckKind
>>>>> will be wrong, as well as Alignment.
>>>>> I don't expect users to use an older runtime with a newer clang to
>>>>> happen much.
>>>>>
>>>>> Old objects with newer lib:
>>>>> Those fields will be wrong too.
>>>>>
>>>>> If we have the Version field (same struct size as without it), we can
>>>>> differentiate, and we can make old objects on the newer lib still work
>>>>> (same thing we did for FloatCastOverflow).
>>>>>
>>>>> I expect lots of false positives with the first two, which should make
>>>>> the users double-check their assumptions. But might end up making people
>>>>> spend a good chunk of time debugging until they find it out.
>>>>>
>>>>>
>>>>> On Thursday, 26 May 2016, Kostya Serebryany <kcc at google.com> wrote:
>>>>>
>>>>>> kcc added a comment.
>>>>>>
>>>>>> In http://reviews.llvm.org/D19668#441261, @filcab wrote:
>>>>>>
>>>>>> > In http://reviews.llvm.org/D19668#441079, @kcc wrote:
>>>>>> >
>>>>>> > > Sorry, I missed this CL.
>>>>>> > >  Could you please repeat why we need backwards compatibility?
>>>>>> > >
>>>>>> > > I would rather prefer to not have this complexity, but instead
>>>>>> make sure we don't mix old API and the new one,
>>>>>> > >  e.g. the same way we do it in asan. (see
>>>>>> asan/asan_init_version.h)
>>>>>> >
>>>>>> >
>>>>>> > The backwards compatibility was to be able to mix older files, like
>>>>>> when we upgraded the float cast overflow to be able to deal with having the
>>>>>> source location or not.
>>>>>> >
>>>>>> > From your comment it seems you'd prefer one of:
>>>>>> >
>>>>>> > - Just use the newer and users have to match versions of clang with
>>>>>> the ubsan runtime (I think this is acceptable, but I might be an edge case
>>>>>> ;-) )
>>>>>> > - Create `__ubsan_handle_type_mismatch2` and
>>>>>> `__ubsan_handle_type_mismatch2_abort` (or similar) and use the new struct
>>>>>> with that (no need to add a Version field if we do it this way, but we'll
>>>>>> have functions that might just start to bitrot)
>>>>>> >
>>>>>> >   I think doing ABI versions the way ASan does them doesn't really
>>>>>> work that well for UBSan, since the checks are mostly spot checks, which
>>>>>> don't depend on each other. I think the best it to either don't version it
>>>>>> and say users are responsible for matching revs, or have versions per check.
>>>>>>
>>>>>>
>>>>>> I don't want to have multiple versions in the code base as it makes
>>>>>> code maintenance more expensive.
>>>>>> Making "users are responsible for X" work only if the users know when
>>>>>> the break X.
>>>>>> Will they know it in this case, or things will break silently?
>>>>>>
>>>>>> > Which one would you prefer (I have mild preference for one, but no
>>>>>> strong opinion)?
>>>>>>
>>>>>>
>>>>>>
>>>>>> http://reviews.llvm.org/D19668
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160526/ffabaa93/attachment.html>