[Clang] Add machinery to catch overflow in unary minus outside of a constant expression context

[shafik]

Summary

We provide several diagnostics for various undefined behaviors due to signed integer overflow outside of a constant expression context. We were missing the machinery to catch overflows due to unary minus.

Fixes: https://github.com/llvm/llvm-project/issues/31643

[aaron.ballman]

LGTM, though needs a release note.

clang/lib/AST/ExprConstant.cpp

13503–13513

[shafik]

• Add release note.

[hans]

Amusingly this fired on some code that used negation with the intention of getting fewer warnings: https://github.com/KhronosGroup/glslang/commit/866f67140ed98865350b53312bba1b654b59f937 :-)
Fixed in https://github.com/KhronosGroup/glslang/commit/7341a21b345e7aea1d2791db0f2d36866c434c03

[eaeltsin]

The warning now fires even if overflow is prevented with if constexpr:

if constexpr (width <= 64) {
  if constexpr (width == 64) {
    return 1;
  }
  return -static_cast<int64_t>(uint64_t{1} << (width - 1));
}

https://godbolt.org/z/M3xdcKd3M

[shafik]

In D142867#4108079, @eaeltsin wrote:

The warning now fires even if overflow is prevented with if constexpr:

if constexpr (width <= 64) {
  if constexpr (width == 64) {
    return 1;
  }
  return -static_cast<int64_t>(uint64_t{1} << (width - 1));
}

https://godbolt.org/z/M3xdcKd3M

I believe this is due to issue: https://github.com/llvm/llvm-project/issues/59448

[hubert.reinterpretcast]

In D142867#4108079, @eaeltsin wrote:

The warning now fires even if overflow is prevented with if constexpr:

if constexpr (width <= 64) {
  if constexpr (width == 64) {
    return 1;
  }
  return -static_cast<int64_t>(uint64_t{1} << (width - 1));
}

https://godbolt.org/z/M3xdcKd3M

For reference, actually placing the second return into an else block (thus making it a discarded statement) does suppress the diagnostic:
https://godbolt.org/z/Kb6Md5PrK

I also question the focus on if constexpr. Replacing with if (true) { return 1; } should be as effective in the non-else-block case in suppressing the warning in terms of QoI.

clang/docs/ReleaseNotes.rst

- Fix crash on invalid code when looking up a destructor in a templated class
   inside a namespace. This fixes
   `Issue 59446 <https://github.com/llvm/llvm-project/issues/59446>`_.
 - Fix crash when diagnosing incorrect usage of ``_Nullable`` involving alias
   templates. This fixes
   `Issue 60344 <https://github.com/llvm/llvm-project/issues/60344>`_.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+- We now generate a diagnostic for signed integer overflow due to unary minus
+  in a non-constant expression context. This fixes
+  `Issue 31643 <https://github.com/llvm/llvm-project/issues/31643>`_
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 New Compiler Flags
 ------------------
 
 Deprecated Compiler Flags

clang/lib/AST/ExprConstant.cpp

bool IntExprEvaluator::VisitUnaryOperator(const UnaryOperator *E) {
   case UO_Plus:
     // The result is just the value.
     return Visit(E->getSubExpr());
   case UO_Minus: {
     if (!Visit(E->getSubExpr()))
       return false;
     if (!Result.isInt()) return Error(E);
     const APSInt &Value = Result.getInt();
-    if (Value.isSigned() && Value.isMinSignedValue() && E->canOverflow() &&
-        !HandleOverflow(Info, E, -Value.extend(Value.getBitWidth() + 1),
+    if (Value.isSigned() && Value.isMinSignedValue() && E->canOverflow()) {
+      if (Info.checkingForUndefinedBehavior())
+        Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
+                                         diag::warn_integer_constant_overflow)
+            << toString(Value, 10) << E->getType();
+
+      if (!HandleOverflow(Info, E, -Value.extend(Value.getBitWidth() + 1),
                           E->getType()))
         return false;
+    }
     return Success(-Value, E);
   }
   case UO_Not: {
     if (!Visit(E->getSubExpr()))
       return false;
     if (!Result.isInt()) return Error(E);
     return Success(~Result.getInt(), E);
   }
   case UO_LNot: {

clang/lib/Sema/SemaChecking.cpp

 void Sema::CheckForIntOverflow (Expr *E) {
   // Use a work list to deal with nested struct initializers.
   SmallVector<Expr *, 2> Exprs(1, E);
 
   do {
     Expr *OriginalE = Exprs.pop_back_val();
     Expr *E = OriginalE->IgnoreParenCasts();
 
-    if (isa<BinaryOperator>(E)) {
+    if (isa<BinaryOperator, UnaryOperator>(E)) {
       E->EvaluateForOverflow(Context);
       continue;
     }
 
     if (auto InitList = dyn_cast<InitListExpr>(OriginalE))
       Exprs.append(InitList->inits().begin(), InitList->inits().end());
     else if (isa<ObjCBoxedExpr>(OriginalE))
       E->EvaluateForOverflow(Context);

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

void f(int n) {
     case (int)((float)1e37 / 1e30): // ok
     case (int)(__fp16)65536: // expected-error {{constant expression}} expected-note {{value +Inf is outside the range of representable values of type 'int'}}
       break;
     }
   }
 
   constexpr int int_min = ~0x7fffffff;
   constexpr int minus_int_min = -int_min; // expected-error {{constant expression}} expected-note {{value 2147483648 is outside the range}}
+  constexpr int minus_int_min_from_shift = -(1<<31); // expected-error {{constant expression}} expected-note {{value 2147483648 is outside the range}}
   constexpr int div0 = 3 / 0; // expected-error {{constant expression}} expected-note {{division by zero}}
   constexpr int mod0 = 3 % 0; // expected-error {{constant expression}} expected-note {{division by zero}}
   constexpr int int_min_div_minus_1 = int_min / -1; // expected-error {{constant expression}} expected-note {{value 2147483648 is outside the range}}
   constexpr int int_min_mod_minus_1 = int_min % -1; // expected-error {{constant expression}} expected-note {{value 2147483648 is outside the range}}
 
   constexpr int shl_m1 = 0 << -1; // expected-error {{constant expression}} expected-note {{negative shift count -1}}
   constexpr int shl_0 = 0 << 0; // ok
   constexpr int shl_31 = 0 << 31; // ok

clang/test/SemaCXX/integer-overflow.cpp

namespace VirtualCallWithVbase {
     };
     int d;
     bool e(C c) {
       if (c.f(&d, d)) {}
       return true;
     }
   }
 }
+
+namespace GH31643 {
+void f() {
+  int a = -(1<<31); // expected-warning {{overflow in expression; result is -2147483648 with type 'int'}}
+}
+}

clang/unittests/Support/TimeProfilerTest.cpp

constexpr int slow_init_list[] = {1, 1, 2, 3, 5, 8, 13, 21}; // 25th line
   setupProfiler();
   ASSERT_TRUE(compileFromString(Code, "-std=c++20", "test.cc"));
   std::string Json = teardownProfiler();
   std::string TraceGraph = buildTraceGraph(Json);
   ASSERT_TRUE(TraceGraph == R"(
 Frontend
 | EvaluateAsRValue (<test.cc:8:21>)
 | EvaluateForOverflow (<test.cc:8:21, col:25>)
+| EvaluateForOverflow (<test.cc:8:30, col:32>)
 | EvaluateAsRValue (<test.cc:9:14>)
 | EvaluateForOverflow (<test.cc:9:9, col:14>)
 | isPotentialConstantExpr (slow_namespace::slow_func)
 | EvaluateAsBooleanCondition (<test.cc:8:21, col:25>)
 | | EvaluateAsRValue (<test.cc:8:21, col:25>)
 | EvaluateAsBooleanCondition (<test.cc:8:21, col:25>)
 | | EvaluateAsRValue (<test.cc:8:21, col:25>)
 | EvaluateAsInitializer (slow_value)