[AST] Improve overflow diagnostics for fixed-point constant evaluation.

Summary:
Diagnostics for overflow were not being produced for fixed-point
evaluation. This patch refactors a bit of the evaluator and adds
a proper diagnostic for these cases.

Reviewers: rjmccall, leonardchan, bjope

Subscribers: cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D73188

Author: bevin-hansson

clang/include/clang/Basic/DiagnosticASTKinds.td

@@ -346,6 +346,9 @@ def err_experimental_clang_interp_failed : Error<
 def warn_integer_constant_overflow : Warning<
   "overflow in expression; result is %0 with type %1">,
   InGroup<DiagGroup<"integer-overflow">>;
+def warn_fixedpoint_constant_overflow : Warning<
+  "overflow in expression; result is %0 with type %1">,
+  InGroup<DiagGroup<"fixed-point-overflow">>;
 
 // This is a temporary diagnostic, and shall be removed once our
 // implementation is complete, and like the preceding constexpr notes belongs

clang/lib/AST/ExprConstant.cpp

@@ -12881,8 +12881,14 @@ bool FixedPointExprEvaluator::VisitCastExpr(const CastExpr *E) {
       return false;
     bool Overflowed;
     APFixedPoint Result = Src.convert(DestFXSema, &Overflowed);
-    if (Overflowed && !HandleOverflow(Info, E, Result, DestType))
-      return false;
+    if (Overflowed) {
+      if (Info.checkingForUndefinedBehavior())
+        Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
+                                         diag::warn_fixedpoint_constant_overflow)
+          << Result.toString() << E->getType();
+      else if (!HandleOverflow(Info, E, Result, E->getType()))
+        return false;
+    }
     return Success(Result, E);
   }
   case CK_IntegralToFixedPoint: {
@@ -12894,8 +12900,14 @@ bool FixedPointExprEvaluator::VisitCastExpr(const CastExpr *E) {
     APFixedPoint IntResult = APFixedPoint::getFromIntValue(
         Src, Info.Ctx.getFixedPointSemantics(DestType), &Overflowed);
 
-    if (Overflowed && !HandleOverflow(Info, E, IntResult, DestType))
-      return false;
+    if (Overflowed) {
+      if (Info.checkingForUndefinedBehavior())
+        Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
+                                         diag::warn_fixedpoint_constant_overflow)
+          << IntResult.toString() << E->getType();
+      else if (!HandleOverflow(Info, E, IntResult, E->getType()))
+        return false;
+    }
 
     return Success(IntResult, E);
   }
@@ -12920,47 +12932,41 @@ bool FixedPointExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {
   if (!EvaluateFixedPointOrInteger(RHS, RHSFX, Info))
     return false;
 
+  bool OpOverflow = false, ConversionOverflow = false;
+  APFixedPoint Result(LHSFX.getSemantics());
   switch (E->getOpcode()) {
   case BO_Add: {
-    bool AddOverflow, ConversionOverflow;
-    APFixedPoint Result = LHSFX.add(RHSFX, &AddOverflow)
-                               .convert(ResultFXSema, &ConversionOverflow);
-    if ((AddOverflow || ConversionOverflow) &&
-        !HandleOverflow(Info, E, Result, E->getType()))
-      return false;
-    return Success(Result, E);
+    Result = LHSFX.add(RHSFX, &OpOverflow)
+                  .convert(ResultFXSema, &ConversionOverflow);
+    break;
   }
   case BO_Sub: {
-    bool AddOverflow, ConversionOverflow;
-    APFixedPoint Result = LHSFX.sub(RHSFX, &AddOverflow)
-                               .convert(ResultFXSema, &ConversionOverflow);
-    if ((AddOverflow || ConversionOverflow) &&
-        !HandleOverflow(Info, E, Result, E->getType()))
-      return false;
-    return Success(Result, E);
+    Result = LHSFX.sub(RHSFX, &OpOverflow)
+                  .convert(ResultFXSema, &ConversionOverflow);
+    break;
   }
   case BO_Mul: {
-    bool AddOverflow, ConversionOverflow;
-    APFixedPoint Result = LHSFX.mul(RHSFX, &AddOverflow)
-                               .convert(ResultFXSema, &ConversionOverflow);
-    if ((AddOverflow || ConversionOverflow) &&
-        !HandleOverflow(Info, E, Result, E->getType()))
-      return false;
-    return Success(Result, E);
+    Result = LHSFX.mul(RHSFX, &OpOverflow)
+                  .convert(ResultFXSema, &ConversionOverflow);
+    break;
   }
   case BO_Div: {
-    bool AddOverflow, ConversionOverflow;
-    APFixedPoint Result = LHSFX.div(RHSFX, &AddOverflow)
-                               .convert(ResultFXSema, &ConversionOverflow);
-    if ((AddOverflow || ConversionOverflow) &&
-        !HandleOverflow(Info, E, Result, E->getType()))
-      return false;
-    return Success(Result, E);
+    Result = LHSFX.div(RHSFX, &OpOverflow)
+                  .convert(ResultFXSema, &ConversionOverflow);
+    break;
   }
   default:
     return false;
   }
-  llvm_unreachable("Should've exited before this");
+  if (OpOverflow || ConversionOverflow) {
+    if (Info.checkingForUndefinedBehavior())
+      Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
+                                       diag::warn_fixedpoint_constant_overflow)
+        << Result.toString() << E->getType();
+    else if (!HandleOverflow(Info, E, Result, E->getType()))
+      return false;
+  }
+  return Success(Result, E);
 }
 
 //===----------------------------------------------------------------------===//

clang/test/Frontend/fixed_point_errors.c

@@ -250,3 +250,17 @@ unsigned u_const = -2.5hk;                // expected-warning{{implicit conversi
 char c_const = 256.0uk;                   // expected-warning{{implicit conversion from 256.0 cannot fit within the range of values for 'char'}}
 short _Accum sa_const5 = 256;             // expected-warning{{implicit conversion from 256 cannot fit within the range of values for 'short _Accum'}}
 unsigned short _Accum usa_const2 = -2;    // expected-warning{{implicit conversion from -2 cannot fit within the range of values for 'unsigned short _Accum'}}
+
+short _Accum add_ovf1 = 255.0hk + 20.0hk;                     // expected-warning {{overflow in expression; result is -237.0 with type 'short _Accum'}}
+short _Accum add_ovf2 = 10 + 0.5hr;                           // expected-warning {{overflow in expression; result is 0.5 with type 'short _Fract'}}
+short _Accum sub_ovf1 = 16.0uhk - 32.0uhk;                    // expected-warning {{overflow in expression; result is 240.0 with type 'unsigned short _Accum'}}
+short _Accum sub_ovf2 = -255.0hk - 20;                        // expected-warning {{overflow in expression; result is 237.0 with type 'short _Accum'}}
+short _Accum mul_ovf1 = 200.0uhk * 10.0uhk;                   // expected-warning {{overflow in expression; result is 208.0 with type 'unsigned short _Accum'}}
+short _Accum mul_ovf2 = (-0.5hr - 0.5hr) * (-0.5hr - 0.5hr);  // expected-warning {{overflow in expression; result is -1.0 with type 'short _Fract'}}
+short _Accum div_ovf1 = 255.0hk / 0.5hk;                      // expected-warning {{overflow in expression; result is -2.0 with type 'short _Accum'}}
+
+// No warnings for saturation
+short _Fract add_sat  = (_Sat short _Fract)0.5hr + 0.5hr;
+short _Accum sub_sat  = (_Sat short _Accum)-200.0hk - 80.0hk;
+short _Accum mul_sat  = (_Sat short _Accum)80.0hk * 10.0hk;
+short _Fract div_sat  = (_Sat short _Fract)0.9hr / 0.1hr;