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

[Filipe Cabecinhas via llvm-commits]

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.

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

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
> <javascript:_e(%7B%7D,'cvml','filcab%2Bllvm.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
>> <javascript:_e(%7B%7D,'cvml','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/eddc5b91/attachment.html>