This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt][ubsan] Implicit Cast Sanitizer - integer truncation - compiler-rt part
ClosedPublic

Authored by lebedev.ri on Jul 5 2018, 1:18 AM.

Diff Detail

Event Timeline

lebedev.ri created this revision.Jul 5 2018, 1:18 AM
lebedev.ri created this object with visibility "All Users".
Herald added subscribers: Restricted Project, dberris, kubamracek. · View Herald TranscriptJul 5 2018, 1:18 AM
filcab added a subscriber: filcab.Jul 5 2018, 7:28 AM

Looks good. I have a few questions, and would like some additional tests (explained in the comments).

Thanks for working on this!
Filipe

lib/ubsan/ubsan_handlers.cc
457

There's only one enumerator in ImplicitCastCheckKind, why do we have these two ErrorType enumerators? It seems we only have one error we can diagnose. What is meant by ImplicitCast and ImplicitIntegerTruncation?

473

Not ATM.

test/ubsan/TestCases/ImplicitCast/integer-truncation-summary.cpp
5

Why?

test/ubsan/TestCases/ImplicitCast/integer-truncation.c
2

No need to have the warning flag.
I'd rather have the commands split since we only really need FileCheck to run on the output of %run, not the compiler output.

10

Why don't we warn here?
Nit: No need to have the extra parenthesis around the cast, nor around the 0, or the full expression. ~(uint32_t)0 looks readable enough.

18

Nit: Here I think ... = (~(uint32_t)0) & 255 is unambiguous (Added parens around the LHS so no one needs to remember that casts bind more tightly) and doesn't require parsing as many parenthesis.

35

Please add a few from 64 bit to 32 bit (128 bit to 64 bit might be a bit too non-portable). That would show that it also works when there's no (on most/all of the current platforms we support) integer promotions involved.

test/ubsan/TestCases/ImplicitCast/integer-truncation.cpp
35

Unsure we need the 257 when we already had 256 (since what happens is the same thing).

Like i said, not ready for the review :)

lib/ubsan/ubsan_handlers.cc
457

ImplicitCast is, well, implicit cast.
Implicit cast can do many things.
It can truncate to the smaller bit width (what we are currently testing)
It can be a NOP cast that turns large number into a negative number.
It can cast between floating points/integers, with loss of precision.

In other words, there are some more things it will potentially check.

test/ubsan/TestCases/ImplicitCast/integer-truncation-summary.cpp
5

Summary printing does not work otherwise, it needs some other sanitizer for that.
(This is based on compiler-rt/test/ubsan/TestCases/Integer/summary.cpp which has // REQUIRES: ubsan-asan)

test/ubsan/TestCases/ImplicitCast/integer-truncation.c
2

It's kinda noisy without the warning..

10

u32 -1 (0xFFFFFFFF) -> implicit cast -> s8 -1 (0xFF) -> extending back to the original width from the downcasted type (i.e. using the downcasted sign!) -> u32 -1 (0xFFFFFFFF)
u32 -1 (0xFFFFFFFF) == u32 -1 (0xFFFFFFFF).
That is modeled after https://godbolt.org/g/NEzXbb.

This is something for the -fsanitize=implicit-sign-change.

Like i said, not ready for the review :)

No problem, I just pointed out small things :-)

lib/ubsan/ubsan_handlers.cc
457

But won't that just be like the undefined, shift, or nullability groups? On the clang side you're not adding any other types of checks either, just their grouping. I think we should do like in the shift group: Have just the one implicit-integer-truncation on the compiler-rt side, and have the clang side deal with the groupings.

test/ubsan/TestCases/ImplicitCast/integer-truncation-summary.cpp
5

Forgot about the details involving summary. Thanks.

test/ubsan/TestCases/ImplicitCast/integer-truncation.c
2

If we don't capture the output, we won't care ;-)
Other ubsan tests have the same issues. But this is a minor nit. ok to keep if you prefer.

10

Sounds reasonable.

lebedev.ri added inline comments.Jul 5 2018, 12:58 PM
lib/ubsan/ubsan_handlers.cc
457

I'm not sure i follow.

To add -fsanitize=implicit-sign-change, on clang side, i will need a new, different logic to detect it.
I think, i won't need to have a new __ubsan_handle_ for each of those groups.
One would also like to to be able to control these groups separately,
at compile-time, and run-time blacklist time, i expect.

So i don't see why only one side should know about the grouping.

filcab added inline comments.Jul 6 2018, 4:18 AM
lib/ubsan/ubsan_checks.inc
34

There is no shift grouping here. The grouping is on the clang side and it will simply turn on both shift-base and shift-exponent.

lib/ubsan/ubsan_handlers.cc
242

Please check out how the shift grouping is handled here.

457

How is this different from the shift grouping? There is no concept of shift grouping in compiler-rt, just on clang. Clang will emit its handler calls however it wants, and on the compiler-rt side you just need knowledge about the checks themselves, not the grouping. Creation of ubsan handlers doesn't need to correspond to enumerators on ubsan check kinds. type_mismatch is the handler used for many, many unrelated checks, for example.

I've annotated the shift-* examples in the code above. Your ubsan_checks.inc additions should only have the actual check you have today, not the grouping.

463

This code should either be something like this:

// TODO(your_username): Add implicit sign change and similar checks
ErrorType ET = ErrorType::ImplicitIntegerTruncation;

or, if you want to setup some boilerplate right now (I'd rather avoid it until we have a second check):

ErrorType ET;
switch (Data->Kind) {
  default:
    CHECK(0 && "invalid implicit cast check kind");
  case ICCK_IntegerTruncation:
    ET = ErrorType::ImplicitIntegerTruncation;
    break;
  }
eugenis added inline comments.
lib/ubsan/ubsan_handlers.h
139

Please add a handler to ubsan_minimal, too.

lebedev.ri updated this revision to Diff 154587.Jul 9 2018, 7:27 AM
lebedev.ri marked 8 inline comments as done.
lebedev.ri retitled this revision from [private][compiler-rt] Implicit Cast Sanitizer - integer truncation - compiler-rt part to [compiler-rt][ubsan] Implicit Cast Sanitizer - integer truncation - compiler-rt part.
lebedev.ri edited the summary of this revision. (Show Details)
lebedev.ri added reviewers: Restricted Project, samsonov, vsk, rsmith, pcc, eugenis, kcc.
lebedev.ri added a project: Restricted Project.
lebedev.ri added a subscriber: llvm-commits.

This is a compiler-rt part.
The clang part is D48958.

lebedev.ri changed the visibility from "All Users" to "Public (No Login Required)".Jul 9 2018, 7:27 AM
lebedev.ri added inline comments.Jul 9 2018, 7:33 AM
lib/ubsan/ubsan_checks.inc
33

If there is no this grouping, then i would need to default to GenericUB,
and that is not really correct, since this is not really UB.
What harm is in having this check-that-is-just-a-group in here?

lib/ubsan/ubsan_handlers.cc
457

I'm not convinced, and i don't see any logic in that. I'm going to keep this as-is.

463

So when a mismatch between the clang and compiler-rt happens (or bad IR),
we will either crash, or report obviously-wrong check name,
instead of defaulting to sane 'implicit cast'.

I'm sorry, but is this really the best course of action?

lib/ubsan/ubsan_handlers.h
139

Yep, i was planning on doing that, just didn't go it in the first go.

test/ubsan/TestCases/ImplicitCast/integer-truncation.c
2

I have dropped the warning flag.

But i really don't see why we would want to split this into two run lines.
Don't we want the compile to succeed, before running the supposedly-built executable?

18

Dropped some parenthesis, but not all.

35

I have added tests, but i'm wary of "it also works when there's no <..> integer promotions involved." part.
I suspect this will cause test failures and reverts.

lebedev.ri marked 11 inline comments as done.
  • Drop indeed-unneeded 'grouping' ImplicitCast check. For some reason i was very sure i just didn't add some macro, and there was grouping at this level, too.
  • Added test with runtime blacklist, doubling as a test for -fno-sanitize-recover= test
lib/ubsan/ubsan_checks.inc
33

Humm, after digging a bit more, i have finally noticed that i wasn't missing some extra macro,
but indeed, there was simply no grouping for these checks in blacklists...
Dropped.

lib/ubsan/ubsan_handlers.cc
473

That is basically why it is a FIXME.

test/ubsan/TestCases/ImplicitCast/integer-truncation.cpp
35

Consistency?

vsk added inline comments.Jul 12 2018, 11:26 AM
test/ubsan/TestCases/ImplicitCast/integer-truncation.c
31

This check fails on my system:

/Users/vsk/src/llvm.org-lldbsan/llvm/projects/compiler-rt/test/ubsan/TestCases/ImplicitCast/integer-truncation.c:30:11: error: expected string not found in input
// CHECK: {{.*}}integer-truncation.c:[[@LINE-1]]:18: runtime error: implicit cast from type 'uint64_t' (aka 'unsigned long') of value 18446744073709551615 (64-bit, unsigned) to type 'uint8_t' (aka 'unsigned char') changed the value to 255 (8-bit, unsigned)
          ^
<stdin>:6:1: note: scanning from here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /Users/vsk/src/llvm.org-lldbsan/llvm/projects/compiler-rt/test/ubsan/TestCases/ImplicitCast/integer-truncation.c:26:19 in
^
<stdin>:6:1: note: with expression "@LINE-1" equal to "29"
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /Users/vsk/src/llvm.org-lldbsan/llvm/projects/compiler-rt/test/ubsan/TestCases/ImplicitCast/integer-truncation.c:26:19 in
^
<stdin>:7:89: note: possible intended match here
/Users/vsk/src/llvm.org-lldbsan/llvm/projects/compiler-rt/test/ubsan/TestCases/ImplicitCast/integer-truncation.c:29:18: runtime error: implicit cast from type 'uint64_t' (aka 'unsigned long long') of value 18446744073709551615 (64-bit, unsigned) to type 'uint8_t' (aka 'unsigned char') changed the value to 255 (8-bit, unsigned)

I guess on Darwin uint64_t is 'unsigned long long'? Might be best to loosen the check a bit.

Ping - any thoughts on the compiler-rt part?
The clang [part] needs to be fixed wrt https://bugs.llvm.org/show_bug.cgi?id=38166

@vsk thank you for taking a look!
I have relaxed those checks. Please let me know if there are any others (i'm sure bots will tell later on anyway :)

test/ubsan/TestCases/ImplicitCast/integer-truncation.c
31

Relaxed the unsigned long long' case. Any others?

vsk accepted this revision.Jul 24 2018, 11:30 AM

Thanks, lgtm. @filcab?

test/ubsan/TestCases/ImplicitCast/integer-truncation-summary.cpp
4

Per an offline discussion, this needs to include '&& !ubsan-standalone-static' to work on Darwin.

This revision is now accepted and ready to land.Jul 24 2018, 11:30 AM
lebedev.ri marked an inline comment as done.

Adjust test, as per offline disscussion.

filcab accepted this revision.Jul 26 2018, 8:21 AM

I have a nit I'd like to see addressed, otherwise LGTM.

test/ubsan/TestCases/ImplicitCast/integer-truncation.cpp
2

Is this the exact same as the other test? If so, please merge both and have two run lines, one for C, one for C++. If there's *any* difference, then it might be better to have the tests as you have them now.

lebedev.ri marked an inline comment as done.
lebedev.ri edited the summary of this revision. (Show Details)

Address @filcab's review nit - merge integer-truncation.{c,cpp} tests into one.
They are basically identical, so it seems to be the best course of action.

I have a nit I'd like to see addressed, otherwise LGTM.

Thank you for the review!

Rebase, address @rsmith review notes from D48958 - s/cast/conversion/ where appropriate.

NFC rebase, just because.

@vsk, @filcab - thank you for the review!

FYI the test/ubsan/TestCases/ImplicitCast/integer-truncation-blacklist.c
is failing on sanitizer-x86_64-linux-android builder (CC @vitalybuka).

It runs the tests via android_compile.py, so i'm not sure this
is actually *this* issue:

https://code.google.com/p/address-sanitizer/issues/detail?id=316

but this seems oddly similar to the other XFAILed cases...

Right now that seems to be the only failing builder,
so i *think* it makes sense to try to just blacklist it for now.