This is compiler-rt part.
clang part is D50901.
Details
- Reviewers
rsmith vsk filcab - Group Reviewers
Restricted Project - Commits
- rGd32c0d1466de: [compiler-rt][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned…
rL344231: [compiler-rt][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned…
rCRT344231: [compiler-rt][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned…
Diff Detail
- Repository
- rCRT Compiler Runtime
Event Timeline
test/ubsan/TestCases/ImplicitConversion/integer-truncation.c | ||
---|---|---|
96 ↗ | (On Diff #162041) | Could you please split refactoring and behavior changes into separate patches? |
test/ubsan/TestCases/ImplicitConversion/integer-truncation.c | ||
---|---|---|
96 ↗ | (On Diff #162041) | I'm not sure i follow. |
- Split off the NFC preparatory changes.
- Rebased.
- Drop legacy handling
test/ubsan/TestCases/ImplicitConversion/integer-truncation.c | ||
---|---|---|
96 ↗ | (On Diff #162041) | Hopefully this is better. |
lib/ubsan/ubsan_handlers.h | ||
---|---|---|
128 | 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? | |
test/ubsan/TestCases/ImplicitConversion/signed-integer-truncation-summary.cpp | ||
10 | 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!
lib/ubsan/ubsan_handlers.h | ||
---|---|---|
128 | @filcab @vitalybuka
Please make up your mind :) (If we do, i can just reinstate all that shiny upgrade logic, which is fully-functional) |
lib/ubsan/ubsan_handlers.h | ||
---|---|---|
128 | 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. Thank you, |
lib/ubsan/ubsan_handlers.h | ||
---|---|---|
128 |
I agree with this. That is why i have implemented that very solution in the initial version of this review. |
- Rebased
- Tentatively restored the legacy (ICCK_IntegerTruncation) handling, pending @filcab's decision.
Thanks for working on this,
Filipe
lib/ubsan/ubsan_handlers.cc | ||
---|---|---|
462 | 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. | |
lib/ubsan/ubsan_handlers.h | ||
128–130 | 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). |
lib/ubsan/ubsan_handlers.cc | ||
---|---|---|
462 | 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
runtime.
- please add a mention that the 0 enumerator for the check kind was only
emitted on llvm7.0
Thank you,
Filipe
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.
Thank you,
Filipe
As requested in https://reviews.llvm.org/D50902#1250110, actually document that ICCK_IntegerTruncation was only emitted by clang 7.
test/ubsan/TestCases/ImplicitConversion/signed-integer-truncation-summary.cpp | ||
---|---|---|
10 | That summary line is the very essence of what we are checking in this test. |
Very sorry for the delay, and especially sorry for asking about the same thing over and over :|
Thanks for your patience,
LGTM
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?