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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri 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,
----------------
filcab wrote:
> 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
> I think the expense of supporting the first case is not enough to make us pick one of the other two.

I agree with this. That is why i have implemented that very solution in the initial version of this review.
@vitalybuka do i return that code?


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D50902