Page MenuHomePhabricator

[clang][ubsan] Split Implicit Integer Truncation Sanitizer into unsigned and signed checks
ClosedPublic

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

Details

Summary

As per IRC disscussion, it seems we really want to have more fine-grained -fsanitize=implicit-integer-truncation:

  • A check when both of the types are unsigned.
  • Another check for the other cases (either one of the types is signed, or both of the types is signed).

This is clang part.
Compiler-rt part is D50902.

Diff Detail

Repository
rL LLVM

Event Timeline

vsk added inline comments.Aug 22 2018, 11:49 AM
test/CodeGen/catch-implicit-integer-truncations-basics-negatives.c
23 ↗(On Diff #161249)

It looks like 'CHECK: }' is meant to check that the end of the function is reached without any diagnostic handlers being emitted. If so, you'll need something stricter to actually check that.

Considering removing all of the 'CHECK: }' lines and adding -implicit-check-not=__ubsan_handle_implicit as a FileCheck option.

test/CodeGen/catch-implicit-signed-integer-truncations-basics-negatives.c
17 ↗(On Diff #161249)

Ditto, I think this applies to the rest of the tests.

lebedev.ri added inline comments.Aug 22 2018, 12:08 PM
test/CodeGen/catch-implicit-integer-truncations-basics-negatives.c
23 ↗(On Diff #161249)

I already specify -implicit-check-not="call void @__ubsan_handle_implicit_conversion".
As for the // CHECK: }, i know it doesn't do anything by itself, so i could indeed drop that indeed.

lebedev.ri marked 2 inline comments as done.

Rebased, addressed @vsk review note, should be NFC.

@vsk thank you for taking a look!

<vedantk> LebedevRI: It looks like it's in good shape. I wasn't around for the discussion re: splitting up the (un)signed cases, so it'd be good to have someone involved with that sign off

@rsmith ?

Ping once again :)

@rsmith - ping.
This one should be rather uncontroversial i think? Is this moving in the direction you suggested? :)

vitalybuka added inline comments.
lib/CodeGen/CGExprScalar.cpp
305 ↗(On Diff #162040)

why do you need to keep it?

@vsk thanks for taking a look!

lib/CodeGen/CGExprScalar.cpp
305 ↗(On Diff #162040)

*Here* - for consistency with the compiler-rt part.

There - what about mismatch in the used clang version
(which still only produced the (ImplicitConversionCheckKind)0), and compiler-rt version
(which has (ImplicitConversionCheckKind)1 and (ImplicitConversionCheckKind)2)?
Is it 100.00% guaranteed not to happen? I'm not so sure.

@vsk thanks for taking a look!

/me can't read :)
That was supposed to be @vitalybuka, of course (:

vitalybuka added inline comments.Sep 26 2018, 5:53 PM
lib/CodeGen/CGExprScalar.cpp
305 ↗(On Diff #162040)

I don't think we try support mismatched versions of clang and compiler-rt

lebedev.ri marked 3 inline comments as done.
  • Split off the NFC preparatory changes.
  • Rebased.
filcab added inline comments.Sep 27 2018, 8:25 AM
lib/CodeGen/CGExprScalar.cpp
305 ↗(On Diff #162040)

We don't make a big effort in open source. But we try to at least make it work (check previous work on type_mismatch handler, before versions were introduced), or error "loudly" when linking (versioned checks).

I think having keeping the older check number free is a good thing and allows us to have binary compatibility with older objects (and keep supporting old object files (we *did* release llvm 7.0 with the old-type check)).

If we hadn't had a release in between, I'd be all for reusing.

lebedev.ri marked an inline comment as done and 3 inline comments as not done.
  • Rebased

As requested in https://reviews.llvm.org/D50902#1250110, actually document that ICCK_IntegerTruncation was only emitted by clang 7.

Ping! It seemed we were so close here :)

filcab added a subscriber: aizatsky.Oct 8 2018, 3:52 AM

Sorry about that. I’m away today but I don’t think you’ve answered my
questions about “why not support standalone UBSan in tests”. Sorry if I
missed the answers if you did. Will review the latest tomorrow.

Thank you,
Filipe

Sorry about that. I’m away today but I don’t think you’ve answered my
questions about “why not support standalone UBSan in tests”. Sorry if I
missed the answers if you did.

I did answer that question twice now :)

https://reviews.llvm.org/D50902#inline-463926

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.

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

Will review the latest tomorrow.

Thank you,

Filipe
rsmith accepted this revision.Oct 10 2018, 3:33 PM

This looks good to me. Sounds like @filcab is intending on doing another round of review too, so it'd make sense to double-check there before committing.

docs/UndefinedBehaviorSanitizer.rst
101 ↗(On Diff #167621)

this sanitizer -> these sanitizers

This revision is now accepted and ready to land.Oct 10 2018, 3:33 PM

LGTM on the clang side too.

Thank you,
Filipe

This looks good to me. Sounds like @filcab is intending on doing another round of review too, so it'd make sense to double-check there before committing.

LGTM on the clang side too.

Thank you,

Filipe

YAY \0/
Thank you for the review!

This revision was automatically updated to reflect the committed changes.