[Sema] Warn when shifting a negative value

[davide]

Summary

While I think there's no need to not mirror gcc warning, probably the message can be reworded in a slightly different way.
I tried, but suggestions are more than welcome. Fixes PR24026

% ./clang -Wshift-negative-value emit.c
emit.c:3:14: warning: shifting a negative signed value is undefined [-Wshift-negative-value]

int a = -1 << 3;
        ~~ ^

1 warning generated.

[rsmith]

LGTM

lib/Sema/SemaExpr.cpp

7940–7941

While you're here, please reverse the order of these two lines. It's much cheaper to check for an unsigned type than to evaluate the expression.

test/Sema/shift.c

42

Please also add a test that we even warn for shifting a negative value by zero.

[davide]

Now in tree, r241478. Thank you for the review, Richard.

include/clang/Basic/DiagnosticSemaKinds.td

 def warn_floatingpoint_eq : Warning<
   "comparing floating point with == or != is unsafe">,
   InGroup<DiagGroup<"float-equal">>, DefaultIgnore;
 
 def warn_division_by_zero : Warning<"division by zero is undefined">,
   InGroup<DivZero>;
 def warn_remainder_by_zero : Warning<"remainder by zero is undefined">,
   InGroup<DivZero>;
+def warn_shift_lhs_negative : Warning<"shifting a negative signed value is undefined">,
+  InGroup<DiagGroup<"shift-negative-value">>;
 def warn_shift_negative : Warning<"shift count is negative">,
   InGroup<DiagGroup<"shift-count-negative">>;
 def warn_shift_gt_typewidth : Warning<"shift count >= width of type">,
   InGroup<DiagGroup<"shift-count-overflow">>;
 def warn_shift_result_gt_typewidth : Warning<
   "signed shift result (%0) requires %1 bits to represent, but %2 only has "
   "%3 bits">, InGroup<DiagGroup<"shift-overflow">>;
 def warn_shift_result_sets_sign_bit : Warning<

lib/Sema/SemaExpr.cpp

     return;
 
   // When left shifting an ICE which is signed, we can check for overflow which
   // according to C++ has undefined behavior ([expr.shift] 5.8/2). Unsigned
   // integers have defined behavior modulo one more than the maximum value
   // representable in the result type, so never warn for those.
   llvm::APSInt Left;
   if (LHS.get()->isValueDependent() ||
-      !LHS.get()->isIntegerConstantExpr(Left, S.Context) ||
+      !LHS.get()->EvaluateAsInt(Left, S.Context) ||
       LHSType->hasUnsignedIntegerRepresentation())
     return;
+
+  // If LHS does not have a signed type and non-negative value
+  // then, the behavior is undefined. Warn about it.
+  if (Left.isNegative()) {
+    S.DiagRuntimeBehavior(Loc, LHS.get(),
+                          S.PDiag(diag::warn_shift_lhs_negative)
+                            << LHS.get()->getSourceRange());
+    return;
+  }
+
   llvm::APInt ResultBits =
       static_cast<llvm::APInt&>(Right) + Left.getMinSignedBits();
   if (LeftBits.uge(ResultBits))
     return;
   llvm::APSInt Result = Left.extend(ResultBits.getLimitedValue());
   Result = Result.shl(Right);
 
   // Print the bit representation of the signed integer as an unsigned

test/CXX/expr/expr.const/p2-0x.cpp

namespace UndefinedBehavior {
 
   constexpr int shl_m1 = 0 << -1; // expected-error {{constant expression}} expected-note {{negative shift count -1}} expected-warning {{negative}}
   constexpr int shl_0 = 0 << 0; // ok
   constexpr int shl_31 = 0 << 31; // ok
   constexpr int shl_32 = 0 << 32; // expected-error {{constant expression}} expected-note {{shift count 32 >= width of type 'int' (32}} expected-warning {{>= width of type}}
   constexpr int shl_unsigned_negative = unsigned(-3) << 1; // ok
   constexpr int shl_unsigned_into_sign = 1u << 31; // ok
   constexpr int shl_unsigned_overflow = 1024u << 31; // ok
-  constexpr int shl_signed_negative = (-3) << 1; // expected-error {{constant expression}} expected-note {{left shift of negative value -3}}
+  constexpr int shl_signed_negative = (-3) << 1; // expected-warning {{shifting a negative signed value is undefined}} // expected-error {{constant expression}} expected-note {{left shift of negative value -3}}
   constexpr int shl_signed_ok = 1 << 30; // ok
   constexpr int shl_signed_into_sign = 1 << 31; // ok (DR1457)
   constexpr int shl_signed_into_sign_2 = 0x7fffffff << 1; // ok (DR1457)
   constexpr int shl_signed_off_end = 2 << 31; // expected-error {{constant expression}} expected-note {{signed left shift discards bits}} expected-warning {{signed shift result (0x100000000) requires 34 bits to represent, but 'int' only has 32 bits}}
   constexpr int shl_signed_off_end_2 = 0x7fffffff << 2; // expected-error {{constant expression}} expected-note {{signed left shift discards bits}} expected-warning {{signed shift result (0x1FFFFFFFC) requires 34 bits to represent, but 'int' only has 32 bits}}
   constexpr int shl_signed_overflow = 1024 << 31; // expected-error {{constant expression}} expected-note {{signed left shift discards bits}} expected-warning {{requires 43 bits to represent}}
   constexpr int shl_signed_ok2 = 1024 << 20; // ok
 

test/Sema/shift.c

void test() {
   c <<= CHAR_BIT+1; // expected-warning {{shift count >= width of type}}
   c >>= CHAR_BIT+1; // expected-warning {{shift count >= width of type}}
   (void)((long)c << CHAR_BIT);
 
   int i;
   i = 1 << (WORD_BIT - 2);
   i = 2 << (WORD_BIT - 1); // expected-warning {{bits to represent, but 'int' only has}}
   i = 1 << (WORD_BIT - 1); // expected-warning {{sets the sign bit of the shift expression}}
-  i = -1 << (WORD_BIT - 1);
+  i = -1 << (WORD_BIT - 1); // expected-warning {{shifting a negative signed value is undefined}}
   i = 0 << (WORD_BIT - 1);
   i = (char)1 << (WORD_BIT - 2);
 
   unsigned u;
   u = 1U << (WORD_BIT - 1);
   u = 5U << (WORD_BIT - 1);
 
   long long int lli;
-  lli = INT_MIN << 2; // expected-warning {{bits to represent, but 'int' only has}}
+  lli = INT_MIN << 2; // expected-warning {{shifting a negative signed value is undefined}}
   lli = 1LL << (sizeof(long long) * CHAR_BIT - 2);
 }
 
 #define a 0
 #define ashift 8
 enum { b = (a << ashift) };
 
 // Don't warn for negative shifts in code that is unreachable.