This is an archive of the discontinued LLVM Phabricator instance.

Added warning for unary minus used with unsigned type
ClosedPublic

Authored by xbolva00 on Sep 15 2018, 10:55 AM.

Diff Detail

Event Timeline

xbolva00 created this revision.Sep 15 2018, 10:55 AM
xbolva00 added a subscriber: RKSimon.EditedSep 15 2018, 11:10 AM

/home/xbolva00/LLVM/build/lib/clang/8.0.0/include/bmiintrin.h:312:16: error: unary minus operator applied to type 'unsigned long long', result value is still unsigned

return __X & -__X;

@RKSimon what do you think? valid? Can we fix that header to return X & -(int)X;?
@craig.topper

xbolva00 updated this revision to Diff 165651.Sep 15 2018, 11:10 AM

I don't think this makes much sense.

/home/xbolva00/LLVM/build/lib/clang/8.0.0/include/bmiintrin.h:312:16: error: unary minus operator applied to type 'unsigned long long', result value is still unsigned

return __X & -__X;

@RKSimon what do you think? valid?

https://godbolt.org/z/2n3lQp <- i'm not sure why the second one is better, how how the first one is broken.

lib/Sema/SemaExpr.cpp
12651 ↗(On Diff #165651)

Why is this an error?

xbolva00 added inline comments.Sep 15 2018, 11:18 AM
lib/Sema/SemaExpr.cpp
12651 ↗(On Diff #165651)

Ok, I will switch to warning.

/home/xbolva00/LLVM/build/lib/clang/8.0.0/include/bmiintrin.h:312:16: error: unary minus operator applied to type 'unsigned long long', result value is still unsigned

return __X & -__X;

@RKSimon what do you think? valid? Can we fix that header to return X & -(int)X;?
@craig.topper

That is not correct transformation as far as i can see.
https://godbolt.org/z/qbQDSq
https://rise4fun.com/Alive/wsPn

xbolva00 added a comment.EditedSep 15 2018, 11:22 AM

The first is not broken, but it is now detected by this new check. I think the second form is more explicit and better.

X & - (int)X is not correct
-> yeah, I see now. Thanks.

@RKSimon are fine to rewrite X & -X to X & ((~X)+1) in bmiintrin.h?

found some occurrences of this expression on our code base.

Standard question: what is the SNR of this warning? What value it brings?
How many times it fired? How many of these are actual real bugs?

xbolva00 added a comment.EditedSep 15 2018, 11:41 AM

Cca 6-8 times / 10 000 lines. I checked and first two are bugs, I don't track other warnings yet.

Anyway, I think this pattern should be diagnosed, as confusing and potentially buggy.

xbolva00 updated this revision to Diff 165654.EditedSep 15 2018, 11:49 AM
  1. Error -> Warning

More tests need to be adjusted...

But I will wait now for @rsmith's opinion.

I think we can and should do better about false positives here. If you move this check to SemaChecking, you can produce the warning in a context where you know what the final type is -- I don't think there's any reason to warn if the final type is signed and no wider than the promoted type of the negation.

I think we should also not warn when the negation is an operand of a & or | operator whose width is no greater than the promoted type of the negation, because -power_of_2 is an idiomatic way to form a mask.

I think we can and should do better about false positives here. If you move this check to SemaChecking, you can produce the warning in a context where you know what the final type is -- I don't think there's any reason to warn if the final type is signed and no wider than the promoted type of the negation.

I share your general concern about false positives, but in the specific case you mentioned—

void use(int16_t x)
uint8_t u8 = 1;
use(-u8);

—I think it'd be surprising to maybe 50% of average programmers that x's received value wasn't int16_t(255) but rather int16_t(-1). The only cases I'd personally consider "false positives" would be cases where -u32 was being used as a convenient shorthand for ~u32 + 1 a.k.a. (0u - u32)... but maybe in those cases it wouldn't be much of a burden for the programmer to just accept a fixit to (0u - u32) and be done with it.

xbolva00 added a comment.EditedSep 15 2018, 1:18 PM

I think we can emit fixit hint always, noy only for -u32 case :)

This comment was removed by xbolva00.
xbolva00 updated this revision to Diff 165663.Sep 15 2018, 3:05 PM

Added fixit hints

xbolva00 updated this revision to Diff 165664.Sep 15 2018, 3:10 PM

Hm, isUnsignedIntegerType seems to ignore uintN_t types?

int f(void) {

uint8_t u8 = 1;
uint8_t b = -u8;

}

No warning now :/

joerg added a subscriber: joerg.Sep 16 2018, 11:25 AM

I find this warning confusing. I find a4 to be perfectly expected. IMO this warning should be applied only, if the effective value of the expression is not the same as in the modulo-n arithmetic. This means that if (-x) is explicitly or implicitly cast to a less wide unsigned type, it should not warn. It would consider a warning for the case of using (-x) if integer promotion rules makes it negative though. The question is, how to best patch around the warning though. What options does MSVC have for that? I.e. what equivalent expressions do not trigger this warning?

MSVC warns for every case in the test file I uploaded here. https://godbolt.org/z/jiMFp6

I think we can and should do better about false positives here. If you move this check to SemaChecking, you can produce the warning in a context where you know what the final type is -- I don't think there's any reason to warn if the final type is signed and no wider than the promoted type of the negation.

I share your general concern about false positives, but in the specific case you mentioned—

void use(int16_t x)
uint8_t u8 = 1;
use(-u8);

—I think it'd be surprising to maybe 50% of average programmers that x's received value wasn't int16_t(255) but rather int16_t(-1).

You may be right. But 50% is a *far* too high false positive rate for a Clang warning, so the conclusion should be that we do not warn on that case. Compare that to:

unsigned int n = //...
long m = -n; // almost certainly a bug (if long is wider than int)

or

if (-n < x && x < n) // almost certainly a bug

If 50% of people turn this warning off because it uselessly warns on non-bugs, we are doing our users a disservice.

We need to have a clear idea of what classes of bugs we want to catch here. Unary negation applied to an unsigned type does not deserve a warning by itself, but there are certainly some contexts in which we should warn. The interesting part is identifying them.

test/Sema/unary-minus-unsigned.c
8 ↗(On Diff #165664)

It's unreasonable to warn on this. The user clearly meant the result type to be unsigned: they wrote that type on the left-hand side.

9–10 ↗(On Diff #165664)

For these cases, we should warn that the high order bits are zeroes, since that's the surprising thing, not that the result is unsigned (which was obviously intended).

12 ↗(On Diff #165664)

I don't see any point in warning here. We guarantee that the result is exactly the same as that of

int a3 = -(int)a;
13–14 ↗(On Diff #165664)

This is a much less helpful warning than we could give; a better warning would be that the resulting value is always non-negative. (With appropriate modifications when sizeof(int) == sizeof(long).)

16–18 ↗(On Diff #165664)

Warning on these is unreasonable. These are idiomatic ways of writing certain unsigned constants.

"High bits are zeros" and "result is always non negative" are good candidatates to be catched here. Thanks for detailed review, Richard. Now I will look on it.

xbolva00 updated this revision to Diff 167784.Oct 1 2018, 11:19 AM

Suggestions implemented

rsmith accepted this revision.Oct 1 2018, 3:09 PM

Thanks!

lib/Sema/SemaChecking.cpp
10899

Add braces to these outer ifs.

test/Sema/unary-minus-integer-impcast.c
2–9

This test is not portable (it will fail on a target where sizeof(int) == sizeof(long long)); please add a -triple flag.

Please also add a test that we don't warn for the same-size cases (eg, pick a target where sizeof(int) == sizeof(long) and test that unsigned long b2 = -a; and long c2 = -a; do not warn).

This revision is now accepted and ready to land.Oct 1 2018, 3:09 PM
xbolva00 updated this revision to Diff 167861.Oct 1 2018, 5:28 PM
  • Addressed comments, added tests
rsmith added inline comments.Oct 1 2018, 6:21 PM
test/Sema/unary-minus-integer-impcast-2.c
1 ↗(On Diff #167861)

You can combine the two tests together into a single file using a #ifdef to detect which set of warnings to expect:

unsigned long b2 = -a;
#ifdef __X86_64__
// expected-warning@-2 {{higher order bits are zeroes}}
#endif
long c2 = -a;
#ifdef __X86_64__
// expected-warning@-2 {{value is always non-negative}}
#else
// expected-warning@-4 {{implicit conversion changes signedness}}
#endif
xbolva00 updated this revision to Diff 167868.Oct 1 2018, 6:41 PM
  • tests merged
xbolva00 marked an inline comment as done.Oct 1 2018, 6:42 PM
xbolva00 added inline comments.
test/Sema/unary-minus-integer-impcast-2.c
1 ↗(On Diff #167861)

ok, thanks!

xbolva00 updated this revision to Diff 167869.Oct 1 2018, 6:44 PM
xbolva00 marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.