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

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added inline comments.


================
Comment at: lib/ubsan/ubsan_handlers.h:128
 enum ImplicitConversionCheckKind : unsigned char {
-  ICCK_IntegerTruncation = 0,
+  // Kind 0 was used briefly.
+  ICCK_UnsignedIntegerTruncation = 1,
----------------
lebedev.ri wrote:
> filcab wrote:
> > We did release an llvm version with that kind, though.
> > 
> > Can you re-add it, with an "... (un)signed..." in the error message (or something better, of course), so we still support files compiled with an older (but released) compiler, please?
> @filcab @vitalybuka
> https://reviews.llvm.org/D50901#inline-462927
> 
> > vitalybuka
> > I don't think we try support mismatched versions of clang and compiler-rt
> 
> Please make up your mind :)
> Do we or don't we?
> 
> (If we do, i can just reinstate all that shiny upgrade logic, which is fully-functional)
We don't have a "we should always be backwards compatible" rule, or anything close to it. The llvm releases don't promise to keep the sanitizers backwards compatible.

*But*, as I mentioned on the clang part, we have, in the past, tried to keep compatibility if it's simple and easy. Otherwise, having a linker error helps avoiding surprising runtime error messages when there's a mix of objects from different versions (which can happen anyway. Would be nice on our part to not make it worse :-) ).
Example of maintaining backwards compat: https://reviews.llvm.org/D11793
Example of changing the ABI + revving up the number so we get linker errors when mixing:  https://reviews.llvm.org/D28244

In this case, we can have one of these solutions:
  - Keep backwards compatibility by supporting the older enum value, at the expense of 2 or 3 lines of code.
  - Rev up the version number for an ABI break, "just" so we can re-use that value (no savings in struct size or anything)
  - Silently re-use that value and possibly issuing the wrong signed/unsigned message if there is a mix

I think the expense of supporting the first case is not enough to make us pick one of the other two.

Thank you,
 Filipe


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D50902