Page MenuHomePhabricator

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

Authored by lebedev.ri on Aug 17 2018, 8:03 AM.

Diff Detail

Event Timeline

lebedev.ri created this revision.Aug 17 2018, 8:03 AM

Rebased, should be NFC.

Ping once again :)

@vsk / @filcab - ping.
Assuming the clang part gets accepted, is this OK?

vitalybuka added inline comments.
test/ubsan/TestCases/ImplicitConversion/integer-truncation.c
96 ↗(On Diff #162041)

Could you please split refactoring and behavior changes into separate patches?

lebedev.ri added inline comments.Sep 27 2018, 12:04 AM
test/ubsan/TestCases/ImplicitConversion/integer-truncation.c
96 ↗(On Diff #162041)

I'm not sure i follow.
By refactoring you mean the test rewrite?

lebedev.ri marked 2 inline comments as done.
  • 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.

filcab added inline comments.Sep 27 2018, 7:53 AM
lib/ubsan/ubsan_handlers.h
128–130

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
11

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–130

@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)

filcab added inline comments.Sep 27 2018, 9:28 AM
lib/ubsan/ubsan_handlers.h
128–130

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

lebedev.ri added inline comments.Sep 27 2018, 9:40 AM
lib/ubsan/ubsan_handlers.h
128–130

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?

vitalybuka added inline comments.Sep 27 2018, 11:14 AM
lib/ubsan/ubsan_handlers.h
128–130

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

lebedev.ri marked 5 inline comments as done.
  • Rebased
  • Tentatively restored the legacy (ICCK_IntegerTruncation) handling, pending @filcab's decision.

Thanks for working on this,
Filipe

lib/ubsan/ubsan_handlers.cc
468

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).

lebedev.ri added inline comments.Sep 29 2018, 8:42 AM
lib/ubsan/ubsan_handlers.cc
468

I think it shifts/duplicates the responsibilities.
At the time of the call void @__ubsan_handle_implicit_conversion we already *know* the kind.
If we pass some generic kind, we will then have to re-figure it out here in compiler-rt.
And both the places will need to be kept in sync.
And double the attention needs to be paid to the tests.
I'd honestly better emit ErrorType::GenericUB in place of ICCK_IntegerTruncation instead of not passing the right kind..

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

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.

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
11

That summary line is the very essence of what we are checking in this test.
We can only do that in non-standalone builds, because standalone builds only print generic undefined-behavior error name, while ubsan is coupled with some other sanitizer, an actual error name is printed.
We have discussed this with you in https://reviews.llvm.org/D48959#inline-429528

Ping! It seemed we were so close here :)

filcab accepted this revision.Oct 10 2018, 7:46 AM

Very sorry for the delay, and especially sorry for asking about the same thing over and over :|
Thanks for your patience,

LGTM

This revision is now accepted and ready to land.Oct 10 2018, 7:46 AM

Very sorry for the delay, and especially sorry for asking about the same thing over and over :|
Thanks for your patience,

LGTM

Yay! Thank you for the review very much :)
1 is a go, 3 left...