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

[ebevhan]

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.

[leonardchan]

Could you also add a few test lines showing this warning doesn't appear for operations where the result is a saturated type?

[ebevhan]

Added tests for saturating types. Changed the warning group name for consistency.

[ebevhan]

Rebased.

[rjmccall]

LGTM. I'm somewhat surprised that this is the implementation design for integer overflow diagnostics, but since it is, I see no reason not to adopt it for fixed-point as well.

clang/include/clang/Basic/DiagnosticASTKinds.td

def note_constexpr_memory_leak : Note<
   "allocation performed here was not deallocated"
   "%plural{0:|: (along with %0 other memory leak%s0)}0">;
 def err_experimental_clang_interp_failed : Error<
   "the experimental clang interpreter failed to evaluate an expression">;
 
 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
 // in Sema.
 def note_unimplemented_constexpr_lambda_feature_ast : Note<
     "unimplemented constexpr lambda feature: %0 (coming soon!)">;
 
 def warn_is_constant_evaluated_always_true_constexpr : Warning<

clang/lib/AST/ExprConstant.cpp

 //===--- ExprConstant.cpp - Expression Constant Evaluator -----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
bool FixedPointExprEvaluator::VisitCastExpr(const CastExpr *E) {
 
   switch (E->getCastKind()) {
   case CK_FixedPointCast: {
     APFixedPoint Src(Info.Ctx.getFixedPointSemantics(SubExpr->getType()));
     if (!EvaluateFixedPoint(SubExpr, Src, Info))
       return false;
     bool Overflowed;
     APFixedPoint Result = Src.convert(DestFXSema, &Overflowed);
-    if (Overflowed && !HandleOverflow(Info, E, Result, DestType))
+    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: {
     APSInt Src;
     if (!EvaluateInteger(SubExpr, Src, Info))
       return false;
 
     bool Overflowed;
     APFixedPoint IntResult = APFixedPoint::getFromIntValue(
         Src, Info.Ctx.getFixedPointSemantics(DestType), &Overflowed);
 
-    if (Overflowed && !HandleOverflow(Info, E, IntResult, DestType))
+    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);
   }
   case CK_NoOp:
   case CK_LValueToRValue:
     return ExprEvaluatorBaseTy::VisitCastExpr(E);
   default:
     return Error(E);
   }
 }
 
 bool FixedPointExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {
   const Expr *LHS = E->getLHS();
   const Expr *RHS = E->getRHS();
   FixedPointSemantics ResultFXSema =
       Info.Ctx.getFixedPointSemantics(E->getType());
 
   APFixedPoint LHSFX(Info.Ctx.getFixedPointSemantics(LHS->getType()));
   if (!EvaluateFixedPointOrInteger(LHS, LHSFX, Info))
     return false;
   APFixedPoint RHSFX(Info.Ctx.getFixedPointSemantics(RHS->getType()));
   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)
+    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)
+    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)
+    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)
+    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);
 }
 
 //===----------------------------------------------------------------------===//
 // Float Evaluation
 //===----------------------------------------------------------------------===//
 
 namespace {
 class FloatExprEvaluator

clang/test/Frontend/fixed_point_errors.c

 short _Fract sf_const = 1.0hk;    // expected-warning{{implicit conversion from 1.0 cannot fit within the range of values for 'short _Fract'}}
 unsigned _Accum ua_const = -1.0k; // expected-warning{{implicit conversion from -1.0 cannot fit within the range of values for 'unsigned _Accum'}}
 short _Accum sa_const2 = 128.0k + 128.0k; // expected-warning{{implicit conversion from 256.0 cannot fit within the range of values for 'short _Accum'}}
 short s_const = 65536.0lk;                // expected-warning{{implicit conversion from 65536.0 cannot fit within the range of values for 'short'}}
 unsigned u_const = -2.5hk;                // expected-warning{{implicit conversion from -2.5 cannot fit within the range of values for 'unsigned int'}}
 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;