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

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added a comment.

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.

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;
----------------
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.


================
Comment at: test/fuzzer/ImplicitSignedIntegerTruncationOrSignChangeTest.cpp:13
+static volatile signed char Sink;
+static volatile unsigned int Storage = (uint32_t)-1;
+
----------------
Please cast it to the same type. An `unsigned int` is not necessarily the same as a `uint32_t`.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D50251