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

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added inline comments.


================
Comment at: lib/ubsan/ubsan_handlers.cc:487
+    break;
+  case ICCK_SignedIntegerTruncationOrSignChange:
+    ET = ErrorType::ImplicitSignedIntegerTruncationOrSignChange;
----------------
lebedev.ri wrote:
> 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`.
> 
Sounds good. My only request is the types being consistent then. Otherwise LGTM!


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D50251