[PATCH] D50251: [compiler-rt][ubsan] Implicit Conversion Sanitizer - integer sign change - compiler-rt part

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D50251#1280267, @filcab wrote:

> I have minor questions in inline comments, but sorry, I'll request a big change :-/
>
> Please match the types of casts and parameters. You're often matching `uint32_t` to `unsigned int` (also a `signed char` with `int8_t` (not as problematic here, though), etc). We don't have a guarantee, and the "assuming `int` is 32-bit" part might cause problems eventually. I'm ok with either version:
>
> - Always use sized types like `uint32_t`
> - Always use "platform types" like `unsigned int` (or whatever the standard names them)
>
>   Otherwise, this looks good.


Yay, thank you for the feedback!

> Thank you,
> 
>   Filipe
>    
> 
> P.S: Yay, I managed not to ask about the ubsan-standalone feature! (sorry about that in the earlier reviews :-) )

:)



================
Comment at: lib/ubsan/ubsan_handlers.cc:487
+    break;
+  case ICCK_SignedIntegerTruncationOrSignChange:
+    ET = ErrorType::ImplicitSignedIntegerTruncationOrSignChange;
----------------
filcab wrote:
> This means *both* errors got triggered, right? Or can we have this one with just one being triggered?
> If it's always both, I'd rather have an `And` instead of `Or` in its name.
We emit `ICCK_SignedIntegerTruncationOrSignChange` in the case of truncation (`src` len > `dst` len) from `unsigned` to `signed`.
We could do this as a two *separate* checks:
* truncation (and emit `ICCK_SignedIntegerTruncation`)
  All the truncated bits should be the same as the sign bit of the `dst`.
* sign change (`ICCK_IntegerSignChange`)
  The `src` is `unsigned` (i.e. implicitly non-negative), therefore the `dst` should be non-negative too.
  I.e., the `dst` sign bit should be unset.

But then we do two checks for per one implicit cast, which results in code bloat, etc.
But if we combine these two checks, we end up with:
All the truncated bits should be the same as the sign bit of the `dst` AND the `dst` sign bit should be unset.
Which is optimized down to `src <= std::numeric_limits<decltype(dst)>::max()`.

You can have a cast which is sign-changing, but does not result in a truncation:
unsigned(`0b1111`) -> char(`0b11`) - it's not a truncation, all the bits are the same, but it is a sign change.

So no, this really is an `or`.



Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D50251