This is compiler-rt part.
clang part is D50901.
rsmith vsk filcab
- Group Reviewers
- rL344231: [compiler-rt][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned…
rCRT344231: [compiler-rt][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned…
rGd32c0d1466de: [compiler-rt][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned…
This is compiler-rt part.
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?
Can't we simply not test on the SUMMARY line (test on the error line) and allow standalone too? I don't see what we gain by restricting the test.
Thank you for taking a look!
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 :-) ).
In this case, we can have one of these solutions:
I think the expense of supporting the first case is not enough to make us pick one of the other two.
Thanks for working on this,
If we can unambiguously know which type of error we should emit, why not have the clang change, but always emit the ICCK_IntegerTruncation check type? Then we'd only get this small chunk of code added to compiler-rt.
Mention that only clang7.0 emitted this. Then we'll know we can get rid of it if we rev up the check version (breaking the ABI anyway).
I think it shifts/duplicates the responsibilities.
0 only means “one of those two”, so I prefer your current patch then a
change to genericUB.
Your reasoning is good, and I’m ok with having different constants. Current
patch almost LGTM:
- I have questions about the tests and why not support the standalone
- please add a mention that the 0 enumerator for the check kind was only
emitted on llvm7.0
Great to hear!
Current patch almost LGTM:
- I have questions about the tests and why not support the standalone runtime.
Hm, i'm not sure what you mean.
This should support all the same things as the current monolithic Kind.
- please add a mention that the 0 enumerator for the check kind was only emitted on llvm7.0
Yes, i will do that. I didn'd actually post the updated patch yet, i will.
As requested in https://reviews.llvm.org/D50902#1250110, actually document that ICCK_IntegerTruncation was only emitted by clang 7.
That summary line is the very essence of what we are checking in this test.