[PATCH] D50902: [compiler-rt][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned and signed checks

[Filipe Cabecinhas via llvm-commits]

0 only means “one of those two”, so I prefer your current patch then a
change to genericUB.

Your reasoning is good, and I’m ok with having different constants. Current
patch almost LGTM:
- I have questions about the tests and why not support the standalone
runtime.
- please add a mention that the 0 enumerator for the check kind was only
emitted on llvm7.0

Thank you,
 Filipe

On Sat, 29 Sep 2018 at 16:44, Roman Lebedev via Phabricator <
reviews at reviews.llvm.org> wrote:

> lebedev.ri added inline comments.
>
>
> ================
> Comment at: lib/ubsan/ubsan_handlers.cc:468
> +  case ICCK_IntegerTruncation: { // Legacy, no longer used.
> +    // Let's figure out what it should be as per the new types, and
> upgrade.
> +    // If both types are unsigned, then it's an unsigned truncation.
> ----------------
> filcab wrote:
> > If we can unambiguously know which type of error we should emit, why not
> have the clang change, but always emit the ICCK_IntegerTruncation check
> type? Then we'd only get this small chunk of code added to compiler-rt.
> I think it shifts/duplicates the responsibilities.
> At the time of the `call void @__ubsan_handle_implicit_conversion` we
> already *know* the kind.
> If we pass some generic kind, we will then have to re-figure it out here
> in compiler-rt.
> And both the places will need to be kept in sync.
> And double the attention needs to be paid to the tests.
> I'd honestly better emit `ErrorType::GenericUB` in place of
> `ICCK_IntegerTruncation` instead of not passing the right `kind`..
>
>
> Repository:
>   rCRT Compiler Runtime
>
> https://reviews.llvm.org/D50902
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180929/ea3eafd3/attachment.html>