Page MenuHomePhabricator

[clang][CodeGen] Implicit Conversion Sanitizer: discover the world of CompoundAssign operators
ClosedPublic

Authored by lebedev.ri on Oct 31 2018, 12:02 PM.

Details

Summary

As reported by @regehr (thanks!) on twitter (https://twitter.com/johnregehr/status/1057681496255815686),
we (me) has completely forgot about the binary assignment operator.
In AST, it isn't represented as separate ImplicitCastExpr's,
but as a single CompoundAssignOperator, that does all the casts internally.
Which means, out of these two, only the first one is diagnosed:

auto foo() {
    unsigned char c = 255;
    c = c + 1;
    return c;
}
auto bar() {
    unsigned char c = 255;
    c += 1;
    return c;
}

https://godbolt.org/z/JNyVc4

This patch does handle the CompoundAssignOperator:

int main() {
  unsigned char c = 255;
  c += 1;
  return c;
}
$ ./bin/clang -g -fsanitize=integer /tmp/test.c && ./a.out 
/tmp/test.c:3:5: runtime error: implicit conversion from type 'int' of value 256 (32-bit, signed) to type 'unsigned char' changed the value to 0 (8-bit, unsigned)
    #0 0x2392b8 in main /tmp/test.c:3:5
    #1 0x7fec4a612b16 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x22b16)
    #2 0x214029 in _start (/build/llvm-build-GCC-release/a.out+0x214029)

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Oct 31 2018, 12:02 PM

I can test this and write a few test cases.

Looks good once some tests are added.

This patch doesn't appear to yet fix the "x++" or "x--" cases.

I can test this and write a few test cases.

I'll write the tests tomorrow, i just wanted to post the initial code diff.
(it is a shame that there isn't any working analog of llvm/utils/update_test_checks.py for clang;
writing the check-lines for these tests feels so *extremely* frustrating.),

This patch doesn't appear to yet fix the "x++" or "x--" cases.

It won't, the increment/decrement happens on the original type, there is no cast there. https://godbolt.org/z/WuWA62
Those cases are for normal signed/unsigned overflow checks.

This patch doesn't appear to yet fix the "x++" or "x--" cases.

It won't, the increment/decrement happens on the original type, there is no cast there. https://godbolt.org/z/WuWA62
Those cases are for normal signed/unsigned overflow checks.

Hm, huh, and -fsanitize=undefined -fsanitize=integer do not handle that too, great catch!
https://godbolt.org/z/lPB7n4
I'll look into that next i guess :)

I do not agree that ++ is performed on the original type. The C99 standard (6.5.3.1.2) appears to be very clear on this point: "The expression ++E is equivalent to (E+=1)."

I do not agree that ++ is performed on the original type.

I was only talking about the IR.

The C99 standard (6.5.3.1.2) appears to be very clear on this point: "The expression ++E is equivalent to (E+=1)."

That is clearly not what clang is doing here.

rsmith added a comment.EditedOct 31 2018, 2:38 PM

The C99 standard (6.5.3.1.2) appears to be very clear on this point: "The expression ++E is equivalent to (E+=1)."

That is clearly not what clang is doing here.

How so? E += 1 is equivalent to E = E + 1, which effectively performs the arithmetic in the promoted type, which is what clang (effectively) does. (The potential promotion from E to int can't change the value, but the implicit conversion from the type of E + 1 to the type of E can, if E's type is not a promoted type.)

I do not agree that ++ is performed on the original type. The C99 standard (6.5.3.1.2) appears to be very clear on this point: "The expression ++E is equivalent to (E+=1)."

The C99 standard (6.5.3.1.2) appears to be very clear on this point: "The expression ++E is equivalent to (E+=1)."

That is clearly not what clang is doing here.

How so? E += 1 is equivalent to E = E + 1, which effectively performs the arithmetic in the promoted type, which is what clang (effectively) does. (The potential promotion from E to int can't change the value, but the implicit conversion from the type of E + 1 to the type of E can, if E's type is not a promoted type.)

*effectively*. In other words, which sanitizer should be catching this?
https://godbolt.org/z/AFpRWS
Unless i'm misreading @regehr the promotions *should* happen, and this is supposed to be caught by *this* conversion sanitizer, not the usual overflow sanitizers.

lebedev.ri updated this revision to Diff 172115.Nov 1 2018, 6:38 AM
lebedev.ri edited the summary of this revision. (Show Details)
lebedev.ri added a subscriber: mclow.lists.

Added test coverage.
Please review :)

regehr added a comment.Nov 1 2018, 8:32 AM

Regarding ++ and --, I think for now it's fine to just document that these aren't instrumented.

Regarding ++ and --, I think for now it's fine to just document that these aren't instrumented.

Indeed, that is a different issue, i don't want to solve it in this differential.
Though, there currently isn't a doc where such omissions are tracked, so not sure where (if) i should document it.

lebedev.ri updated this revision to Diff 172335.Nov 2 2018, 3:41 AM

And finish the test coverage.

(sidenote: will these follow-ups again be stuck for two months? let's see!)

Seems reasonable to me.

Seems reasonable to me.

I'm very happy that it seems reasonable.
I'm waiting for either review comments, or formal approval :)

Ping.

rjmccall accepted this revision.Nov 19 2018, 10:34 AM

Heh, okay.

This revision is now accepted and ready to land.Nov 19 2018, 10:34 AM

Heh, okay.

Okay, great, thank you for the review!

This revision was automatically updated to reflect the committed changes.