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

Why is this an error?

[xbolva00]

lib/Sema/SemaExpr.cpp

12651

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

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

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

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

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

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 ↗

(On Diff #167784)

Add braces to these outer ifs.

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

1–8 ↗

(On Diff #167784)

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 ext_typecheck_addrof_temporary : ExtWarn<
   "taking the address of a temporary object of type %0">,
   InGroup<AddressOfTemporary>, DefaultError;
 def err_typecheck_addrof_temporary : Error<
   "taking the address of a temporary object of type %0">;
 def err_typecheck_addrof_dtor : Error<
   "taking the address of a destructor">;
 def err_typecheck_unary_expr : Error<
   "invalid argument type %0 to unary expression">;
+def warn_unsignedtypecheck_unary_minus : Warning<
+  "unary minus operator applied to type %0, result value is still unsigned">,
+   InGroup<SignConversion>;
 def err_typecheck_indirection_requires_pointer : Error<
   "indirection requires pointer operand (%0 invalid)">;
 def ext_typecheck_indirection_through_void_pointer : ExtWarn<
   "ISO C++ does not allow indirection on operand of type %0">,
   InGroup<DiagGroup<"void-ptr-dereference">>;
 def warn_indirection_through_null : Warning<
   "indirection of non-volatile null pointer will be deleted, not trap">,
   InGroup<NullDereference>;

lib/Sema/SemaExpr.cpp

ConvertHalfVec =
         needsConversionOfHalfVec(true, Context, Input.get()->getType());
 
     // If the operand is a half vector, promote it to a float vector.
     if (ConvertHalfVec)
       Input = convertVector(Input.get(), Context.FloatTy, *this);
     resultType = Input.get()->getType();
     if (resultType->isDependentType())
       break;
-    if (resultType->isArithmeticType()) // C99 6.5.3.3p1
+    if (resultType->isArithmeticType()) { // C99 6.5.3.3p1
+      if (Opc == UO_Minus && resultType->isUnsignedIntegerType())
+        return Diag(OpLoc, diag::warn_unsignedtypecheck_unary_minus)
+               << resultType << FixItHint::CreateRemoval(OpLoc)
+               << FixItHint::CreateInsertion(InputExpr->getBeginLoc(), "0u - ")
+               << Input.get()->getSourceRange();
       break;
-    else if (resultType->isVectorType() &&
+    } else if (resultType->isVectorType() &&
                // The z vector extensions don't allow + or - with bool vectors.
                (!Context.getLangOpts().ZVector ||
                 resultType->getAs<VectorType>()->getVectorKind() !=
                     VectorType::AltiVecBool))
       break;
     else if (getLangOpts().CPlusPlus && // C++ [expr.unary.op]p6
              Opc == UO_Plus &&
              resultType->isPointerType())
       break;
 
     return ExprError(Diag(OpLoc, diag::err_typecheck_unary_expr)
       << resultType << Input.get()->getSourceRange());

test/CXX/drs/dr6xx.cpp

 
 namespace dr608 { // dr608: yes
   struct A { virtual void f(); };
   struct B : A {};
   struct C : A { void f(); };
   struct D : B, C {};
 }
 
-int dr610[-0u == 0u ? 1 : -1]; // dr610: yes
+int dr610[-0u == 0u ? // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
+  1 : -1]; // dr610: yes
 
 namespace dr611 { // dr611: yes
   int k;
   struct S { int &r; } s = { k ? k : k };
 }
 
 // dr612: na
 

test/OpenMP/teams_distribute_thread_limit_messages.cpp

#pragma omp teams distribute thread_limit(argc), thread_limit (argc+1) // expected-error {{directive '#pragma omp teams distribute' cannot contain more than one 'thread_limit' clause}}
   for (int j=0; j<100; j++) foo();
 #pragma omp target
 #pragma omp teams distribute thread_limit(S1) // expected-error {{'S1' does not refer to a value}}
   for (int j=0; j<100; j++) foo();
 #pragma omp target
 #pragma omp teams distribute thread_limit(-2) // expected-error {{argument to 'thread_limit' clause must be a strictly positive integer value}}
   for (int j=0; j<100; j++) foo();
 #pragma omp target
-#pragma omp teams distribute thread_limit(-10u)
+#pragma omp teams distribute thread_limit(-10u) // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
   for (int j=0; j<100; j++) foo();
 #pragma omp target
 #pragma omp teams distribute thread_limit(3.14) // expected-error 2 {{expression must have integral or unscoped enumeration type, not 'double'}}
   for (int j=0; j<100; j++) foo();
 
   return 0;
 }
 

 #pragma omp teams distribute thread_limit (S1) // expected-error {{'S1' does not refer to a value}}
   for (int j=0; j<100; j++) foo();
 
 #pragma omp target
 #pragma omp teams distribute thread_limit (-2) // expected-error {{argument to 'thread_limit' clause must be a strictly positive integer value}}
   for (int j=0; j<100; j++) foo();
 
 #pragma omp target
-#pragma omp teams distribute thread_limit (-10u)
+#pragma omp teams distribute thread_limit (-10u) // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
   for (int j=0; j<100; j++) foo();
 
 #pragma omp target
 #pragma omp teams distribute thread_limit (3.14) // expected-error {{expression must have integral or unscoped enumeration type, not 'double'}}
   for (int j=0; j<100; j++) foo();
 
   return tmain<int, 10>(argc); // expected-note {{in instantiation of function template specialization 'tmain<int, 10>' requested here}}
 }

test/OpenMP/teams_thread_limit_messages.cpp

#pragma omp teams thread_limit(argc), thread_limit (argc+1) // expected-error {{directive '#pragma omp teams' cannot contain more than one 'thread_limit' clause}}
   foo();
 #pragma omp target
 #pragma omp teams thread_limit(S1) // expected-error {{'S1' does not refer to a value}}
   foo();
 #pragma omp target
 #pragma omp teams thread_limit(-2) // expected-error {{argument to 'thread_limit' clause must be a strictly positive integer value}}
   foo();
 #pragma omp target
-#pragma omp teams thread_limit(-10u)
+#pragma omp teams thread_limit(-10u) // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
   foo();
 #pragma omp target
 #pragma omp teams thread_limit(3.14) // expected-error 2 {{expression must have integral or unscoped enumeration type, not 'double'}}
   foo();
 
   return 0;
 }
 

 #pragma omp teams thread_limit (S1) // expected-error {{'S1' does not refer to a value}}
   foo();
 
 #pragma omp target
 #pragma omp teams thread_limit (-2) // expected-error {{argument to 'thread_limit' clause must be a strictly positive integer value}}
   foo();
 
 #pragma omp target
-#pragma omp teams thread_limit (-10u)
+#pragma omp teams thread_limit (-10u) // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
   foo();
 
 #pragma omp target
 #pragma omp teams thread_limit (3.14) // expected-error {{expression must have integral or unscoped enumeration type, not 'double'}}
   foo();
 
   return tmain<int, 10>(argc); // expected-note {{in instantiation of function template specialization 'tmain<int, 10>' requested here}}
 }

test/Sema/unary-minus-unsigned.c

+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+
+void test1(void) {
+  unsigned int a = 1;
+  unsigned long b = 1;
+  unsigned long long c = 1;
+
+  unsigned int a2 = -a;       // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
+  unsigned long b2 = -b;      // expected-warning {{unary minus operator applied to type 'unsigned long', result value is still unsigned}}
+  unsigned long long c2 = -c; // expected-warning {{unary minus operator applied to type 'unsigned long long', result value is still unsigned}}
+
+  int a3 = -a;       // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
+  long b3 = -b;      // expected-warning {{unary minus operator applied to type 'unsigned long', result value is still unsigned}}
+  long long c3 = -c; // expected-warning {{unary minus operator applied to type 'unsigned long long', result value is still unsigned}}
+
+  unsigned int a4 = -1u;         // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
+  unsigned long b4 = -1ul;       // expected-warning {{unary minus operator applied to type 'unsigned long', result value is still unsigned}}
+  unsigned long long c4 = -1ull; // expected-warning {{unary minus operator applied to type 'unsigned long long', result value is still unsigned}}
+}