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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka 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:
> > 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?
I think if we do not support backward comparability, we should not to even try to pretend to avoid confusion, even if it's trivial like here. However I am not strong about it. So up to you and @filcab





Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D50902