Added warning for unary minus used with unsigned type

[xbolva00]

Summary

Inspired by MSVC, which found some occurrences of this expression on our code base.

Fixes PR38950

[xbolva00]

/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

[lebedev.ri]

I don't think this makes much sense.

In D52137#1236011, @xbolva00 wrote:

/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]

lib/Sema/SemaExpr.cpp

12651 ↗

(On Diff #165651)

Ok, I will switch to warning.

[lebedev.ri]

In D52137#1236011, @xbolva00 wrote:

/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]

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.

[xbolva00]

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

[lebedev.ri]

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]

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]

1. Error -> Warning

More tests need to be adjusted...

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

[rsmith]

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.

[• Quuxplusone]

In D52137#1236053, @rsmith wrote:

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]

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

[xbolva00]

This comment was removed by xbolva00.

[xbolva00]

Added fixit hints

[xbolva00]

Hm, isUnsignedIntegerType seems to ignore uintN_t types?

int f(void) {

uint8_t u8 = 1;
uint8_t b = -u8;

}

No warning now :/

[joerg]

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?

[xbolva00]

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

[rsmith]

In D52137#1236065, @Quuxplusone wrote:

In D52137#1236053, @rsmith wrote:

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.

[xbolva00]

"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]

Suggestions implemented

[rsmith]

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

[xbolva00]

• Addressed comments, added tests

[rsmith]

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]

• tests merged

[xbolva00]

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

1 ↗

(On Diff #167861)

ok, thanks!

include/clang/Basic/DiagnosticSemaKinds.td

def warn_impcast_integer_sign : Warning<
   "implicit conversion changes signedness: %0 to %1">,
   InGroup<SignConversion>, DefaultIgnore;
 def warn_impcast_integer_sign_conditional : Warning<
   "operand of ? changes signedness: %0 to %1">,
   InGroup<SignConversion>, DefaultIgnore;
 def warn_impcast_integer_precision : Warning<
   "implicit conversion loses integer precision: %0 to %1">,
   InGroup<Conversion>, DefaultIgnore;
+def warn_impcast_high_order_zero_bits : Warning<
+  "higher order bits are zeroes after implicit conversion">,
+  InGroup<Conversion>, DefaultIgnore;
+def warn_impcast_nonnegative_result : Warning<
+  "the resulting value is always non-negative after implicit conversion">,
+  InGroup<SignConversion>, DefaultIgnore;
 def warn_impcast_integer_64_32 : Warning<
   "implicit conversion loses integer precision: %0 to %1">,
   InGroup<Shorten64To32>, DefaultIgnore;
 def warn_impcast_integer_precision_constant : Warning<
   "implicit conversion from %2 to %3 changes value from %0 to %1">,
   InGroup<ConstantConversion>;
 def warn_impcast_bitfield_precision_constant : Warning<
   "implicit truncation from %2 to bit-field changes value from %0 to %1">,

lib/Sema/SemaChecking.cpp

if (S.SourceMgr.isInSystemMacro(CC))
       return;
 
     if (TargetRange.Width == 32 && S.Context.getIntWidth(E->getType()) == 64)
       return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_integer_64_32,
                              /* pruneControlFlow */ true);
     return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_integer_precision);
   }
 
+  if (TargetRange.Width > SourceRange.Width) {
+    if (auto *UO = dyn_cast<UnaryOperator>(E))
+      if (UO->getOpcode() == UO_Minus)
+        if (Source->isUnsignedIntegerType()) {
+          if (Target->isUnsignedIntegerType())
+            return DiagnoseImpCast(S, E, T, CC,
+                                   diag::warn_impcast_high_order_zero_bits);
+          if (Target->isSignedIntegerType())
+            return DiagnoseImpCast(S, E, T, CC,
+                                   diag::warn_impcast_nonnegative_result);
+        }
+  }
+
   if (TargetRange.Width == SourceRange.Width && !TargetRange.NonNegative &&
       SourceRange.NonNegative && Source->isSignedIntegerType()) {
     // Warn when doing a signed to signed conversion, warn if the positive
     // source value is exactly the width of the target type, which will
     // cause a negative value to be stored.
 
     llvm::APSInt Value;
     if (E->EvaluateAsInt(Value, S.Context, Expr::SE_AllowSideEffects) &&

test/Sema/unary-minus-integer-impcast.c

+// RUN: %clang_cc1 %s -verify -Wconversion -fsyntax-only -triple x86_64-pc-linux-gnu
+// RUN: %clang_cc1 %s -verify -Wconversion -fsyntax-only -triple i386-pc-linux-gnu
+
+void test(void) {
+  unsigned int a = 1;
+
+  unsigned long long b = -a; // expected-warning {{higher order bits are zeroes after implicit conversion}}
+  long long c = -a;          // expected-warning {{the resulting value is always non-negative after implicit conversion}}
+
+  unsigned long b2 = -a;
+#ifdef __x86_64__
+// expected-warning@-2 {{higher order bits are zeroes after implicit conversion}}
+#endif
+  long c2 = -a;
+#ifdef __x86_64__
+// expected-warning@-2 {{the resulting value is always non-negative after implicit conversion}}
+#else
+// expected-warning@-4 {{implicit conversion changes signedness: 'unsigned int' to 'long'}}
+#endif
+}