[Sema] add warning for tautological FP compare with literal

[spatel]

Summary

If we are comparing an FP literal with a value that is casted from a type where the literal can't be represented, that's probably a programmer error.

This is a fix for:
https://github.com/llvm/llvm-project/issues/54222

Note - I added the optimizer change with:
9397bdc67eb2
...and as discussed in the post-commit comments, that transform might be too dangerous without this warning in place (and we might want to revert it anyway until this patch has had some time to bake in the wild).

[xbolva00]

Thanks! Yeah, we need a warning like this one.

Some nits added.

clang/include/clang/Basic/DiagnosticSemaKinds.td

122

maybe even say why this is always false?

clang/test/Sema/floating-point-compare.c

24–62

// expected-warning {{result of floating-point comparison is always false}}

33

Same here.

[spatel]

clang/include/clang/Basic/DiagnosticSemaKinds.td

122

Sure. I'm not sure how wordy we want to be, but I can add the source type as a parameter, so that should make it clearer.

Then we would show something like this:

54222.c:4:12: warning: floating-point comparison is always false: constant cannot be represented exactly in type 'float' [-Wliteral-range]
  return f == 0.1;

[spatel]

Patch updated:

1. Improved warning message.
2. Check warning string more exactly in test file.

[xbolva00]

clang/include/clang/Basic/DiagnosticSemaKinds.td

122

Sounds good

[spatel]

No code changes, just updated test comments.

[spatel]

Ping.

[efriedma]

This check seems a little narrow, but fine, I guess.

clang/lib/Sema/SemaChecking.cpp

11471–11472

Fix comments?

11478

Maybe consider IgnoreParens().

11489

There's a helper for this pattern: SourceTy->getAs<BuiltinType>()

clang/test/Sema/floating-point-compare.c

24–62

Maybe also add a testcase with an explicit float->double cast?

[spatel]

Patch updated:

1. Updated function comment.
2. Look through optional parentheses.
3. Use getAs<BuiltinType>.
4. Add tests with explicit casts, parens.

[xbolva00]

clang/test/Sema/floating-point-compare.c

33

One test with macro?
#define CST 0.3

f == CST

[spatel]

Added test with macro.

[xbolva00]

Thanks. One last thing - small info in Clang release notes?

[spatel]

In D121306#3383771, @xbolva00 wrote:

Thanks. One last thing - small info in Clang release notes?

Sure - but if we're going to document this as a feature, I will add the != sibling warning now, so we don't advertise a half-implemented enhancement. :)

[xbolva00]

Yes, support for != would be fine.

[spatel]

Patch updated:

1. Implemented warning for != with selective text.
2. Added tests for != warning.
3. Mentioned improvement in release notes.

[efriedma]

I don't think "floating-point comparison is only true for NaN" is accurate? It should just be "floating-point comparison is always true ", I think.

[spatel]

In D121306#3387016, @efriedma wrote:

I don't think "floating-point comparison is only true for NaN" is accurate? It should just be "floating-point comparison is always true ", I think.

Ah, right. I mistakenly was thinking of C "!=" as "fcmp one" in IR, but the semantics are "fcmp une":
https://godbolt.org/z/TP7145Psv
https://alive2.llvm.org/ce/z/sbRVpA

[spatel]

Updated code comments and warning message for != case; that's always true.

[efriedma]

LGTM

[alexfh]

What's the right fix for this warning, using a literal of the same floating point type? Did you consider adding a fixit hint to the diagnostic?

[efriedma]

In D121306#3405824, @alexfh wrote:

What's the right fix for this warning, using a literal of the same floating point type?

Probably? Depends on why you're trying to use equality on floating-point numbers; "x == 0.3" is inherently a bit suspicious.

Did you consider adding a fixit hint to the diagnostic?

A fixit to do what? Replacing the comparison with a literal "true"/"false" would probably be unwelcome. And I don't really want a "fixit" to change the meaning of well-defined code.

clang/docs/ReleaseNotes.rst

 - ``CXXNewExpr::getArraySize()`` previously returned a ``llvm::Optional``
   wrapping a ``nullptr`` when the ``CXXNewExpr`` did not have an array
   size expression. This was fixed and ``::getArraySize()`` will now always
   either return ``None`` or a ``llvm::Optional`` wrapping a valid ``Expr*``.
   This fixes `Issue 53742 <https://github.com/llvm/llvm-project/issues/53742>`_.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+- ``-Wliteral-range`` will warn on floating-point equality comparisons with
+  constants that are not representable in a casted value. For example,
+  ``(float) f == 0.1`` is always false.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 New Compiler Flags
 ------------------
 
 Deprecated Compiler Flags

clang/include/clang/Basic/DiagnosticSemaKinds.td

def ext_predef_outside_function : Warning<
   "predefined identifier is only valid inside function">,
   InGroup<DiagGroup<"predefined-identifier-outside-function">>;
 def warn_float_overflow : Warning<
   "magnitude of floating-point constant too large for type %0; maximum is %1">,
    InGroup<LiteralRange>;
 def warn_float_underflow : Warning<
   "magnitude of floating-point constant too small for type %0; minimum is %1">,
   InGroup<LiteralRange>;
+def warn_float_compare_literal : Warning<
+  "floating-point comparison is always %select{true|false}0; "
+  "constant cannot be represented exactly in type %1">,
+  InGroup<LiteralRange>;
 def warn_double_const_requires_fp64 : Warning<
   "double precision constant requires %select{cl_khr_fp64|cl_khr_fp64 and __opencl_c_fp64}0, "
   "casting to single precision">;
 def err_half_const_requires_fp16 : Error<
   "half precision constant requires cl_khr_fp16">;
 
 // C99 variable-length arrays
 def ext_vla : Extension<"variable length arrays are a C99 feature">,

clang/include/clang/Sema/Sema.h

private:
 
   void CheckReturnValExpr(Expr *RetValExp, QualType lhsType,
                           SourceLocation ReturnLoc,
                           bool isObjCMethod = false,
                           const AttrVec *Attrs = nullptr,
                           const FunctionDecl *FD = nullptr);
 
 public:
-  void CheckFloatComparison(SourceLocation Loc, Expr *LHS, Expr *RHS);
+  void CheckFloatComparison(SourceLocation Loc, Expr *LHS, Expr *RHS,
+                            BinaryOperatorKind Opcode);
 
 private:
   void CheckImplicitConversions(Expr *E, SourceLocation CC = SourceLocation());
   void CheckBoolLikeConversion(Expr *E, SourceLocation CC);
   void CheckForIntOverflow(Expr *E);
   void CheckUnsequencedOperations(const Expr *E);
 
   /// Perform semantic checks on a completed expression. This will either

clang/lib/Sema/SemaChecking.cpp

Sema::CheckReturnValExpr(Expr *RetValExp, QualType lhsType,
   }
 
   // PPC MMA non-pointer types are not allowed as return type. Checking the type
   // here prevent the user from using a PPC MMA type as trailing return type.
   if (Context.getTargetInfo().getTriple().isPPC64())
     CheckPPCMMAType(RetValExp->getType(), ReturnLoc);
 }
 
-//===--- CHECK: Floating-Point comparisons (-Wfloat-equal) ---------------===//
+/// Check for comparisons of floating-point values using == and !=. Issue a
+/// warning if the comparison is not likely to do what the programmer intended.
+void Sema::CheckFloatComparison(SourceLocation Loc, Expr *LHS, Expr *RHS,
+                                BinaryOperatorKind Opcode) {
+  // Match and capture subexpressions such as "(float) X == 0.1".
+  FloatingLiteral *FPLiteral;
+  CastExpr *FPCast;
+  auto getCastAndLiteral = [&FPLiteral, &FPCast](Expr *L, Expr *R) {
+    FPLiteral = dyn_cast<FloatingLiteral>(L->IgnoreParens());
+    FPCast = dyn_cast<CastExpr>(R->IgnoreParens());
+    return FPLiteral && FPCast;
+  };
+
+  if (getCastAndLiteral(LHS, RHS) || getCastAndLiteral(RHS, LHS)) {
+    auto *SourceTy = FPCast->getSubExpr()->getType()->getAs<BuiltinType>();
+    auto *TargetTy = FPLiteral->getType()->getAs<BuiltinType>();
+    if (SourceTy && TargetTy && SourceTy->isFloatingPoint() &&
+        TargetTy->isFloatingPoint()) {
+      bool Lossy;
+      llvm::APFloat TargetC = FPLiteral->getValue();
+      TargetC.convert(Context.getFloatTypeSemantics(QualType(SourceTy, 0)),
+                      llvm::APFloat::rmNearestTiesToEven, &Lossy);
+      if (Lossy) {
+        // If the literal cannot be represented in the source type, then a
+        // check for == is always false and check for != is always true.
+        Diag(Loc, diag::warn_float_compare_literal)
+            << (Opcode == BO_EQ) << QualType(SourceTy, 0)
+            << LHS->getSourceRange() << RHS->getSourceRange();
+        return;
+      }
+    }
+  }
 
-/// Check for comparisons of floating point operands using != and ==.
-/// Issue a warning if these are no self-comparisons, as they are not likely
-/// to do what the programmer intended.
-void Sema::CheckFloatComparison(SourceLocation Loc, Expr* LHS, Expr *RHS) {
+  // Match a more general floating-point equality comparison (-Wfloat-equal).
   Expr* LeftExprSansParen = LHS->IgnoreParenImpCasts();
   Expr* RightExprSansParen = RHS->IgnoreParenImpCasts();
 
   // Special case: check for x == x (which is OK).
   // Do not emit warnings for such cases.
   if (DeclRefExpr* DRL = dyn_cast<DeclRefExpr>(LeftExprSansParen))
     if (DeclRefExpr* DRR = dyn_cast<DeclRefExpr>(RightExprSansParen))
       if (DRL->getDecl() == DRR->getDecl())
         return;
 
   // Special case: check for comparisons against literals that can be exactly
   //  represented by APFloat.  In such cases, do not emit a warning.  This
   //  is a heuristic: often comparison against such literals are used to
   //  detect if a value in a variable has not changed.  This clearly can
   //  lead to false negatives.
   if (FloatingLiteral* FLL = dyn_cast<FloatingLiteral>(LeftExprSansParen)) {
     if (FLL->isExact())
       return;
   } else
     if (FloatingLiteral* FLR = dyn_cast<FloatingLiteral>(RightExprSansParen))
       if (FLR->isExact())
         return;
 
   // Check for comparisons with builtin types.
   if (CallExpr* CL = dyn_cast<CallExpr>(LeftExprSansParen))

clang/lib/Sema/SemaExpr.cpp

if (Type.isNull())
     return S.InvalidOperands(Loc, LHS, RHS);
   assert(Type->isArithmeticType() || Type->isEnumeralType());
 
   if (Type->isAnyComplexType() && BinaryOperator::isRelationalOp(Opc))
     return S.InvalidOperands(Loc, LHS, RHS);
 
   // Check for comparisons of floating point operands using != and ==.
   if (Type->hasFloatingRepresentation() && BinaryOperator::isEqualityOp(Opc))
-    S.CheckFloatComparison(Loc, LHS.get(), RHS.get());
+    S.CheckFloatComparison(Loc, LHS.get(), RHS.get(), Opc);
 
   // The result of comparisons is 'bool' in C++, 'int' in C.
   return S.Context.getLogicalOperationType();
 }
 
 void Sema::CheckPtrComparisonWithNullChar(ExprResult &E, ExprResult &NullE) {
   if (!NullE.get()->getType()->isAnyPointerType())
     return;
QualType Sema::CheckVectorCompareOperands(ExprResult &LHS, ExprResult &RHS,
   // x == x, x != x, x < x, etc.  These always evaluate to a constant, and
   // often indicate logic errors in the program.
   diagnoseTautologicalComparison(*this, Loc, LHS.get(), RHS.get(), Opc);
 
   // Check for comparisons of floating point operands using != and ==.
   if (BinaryOperator::isEqualityOp(Opc) &&
       LHSType->hasFloatingRepresentation()) {
     assert(RHS.get()->getType()->hasFloatingRepresentation());
-    CheckFloatComparison(Loc, LHS.get(), RHS.get());
+    CheckFloatComparison(Loc, LHS.get(), RHS.get(), Opc);
   }
 
   // Return a signed type for the vector.
   return GetSignedVectorType(vType);
 }
 
 static void diagnoseXorMisusedAsPow(Sema &S, const ExprResult &XorLHS,
                                     const ExprResult &XorRHS,

clang/test/Sema/floating-point-compare.c

 // RUN: %clang_cc1 -fsyntax-only -Wfloat-equal -verify %s
 
 int f1(float x, float y) {
   return x == y; // expected-warning {{comparing floating point with ==}}
 } 
 
 int f2(float x, float y) {
   return x != y; // expected-warning {{comparing floating point with ==}}
 }
 
 int f3(float x) {
   return x == x; // no-warning
 }
 
+// 0.0 can be represented exactly, so don't warn.
 int f4(float x) {
   return x == 0.0; // no-warning {{comparing}}
 }
 
 int f5(float x) {
   return x == __builtin_inf(); // no-warning
 }
 
-int f7(float x) {
-  return x == 3.14159; // expected-warning {{comparing}}
+// The literal is a double that can't be represented losslessly as a float.
+int tautological_FP_compare(float x) {
+  return x == 3.14159; // expected-warning {{floating-point comparison is always false}}
+}
+
+int tautological_FP_compare_inverse(float x) {
+  return x != 3.14159; // expected-warning {{floating-point comparison is always true}}
+}
+
+// The literal is a double that can be represented losslessly as a long double,
+// but this might not be the comparison what was intended.
+int not_tautological_FP_compare(long double f) {
+  return f == 0.1; // expected-warning {{comparing floating point with ==}}
+}
+
+int tautological_FP_compare_commute(float f) {
+  return 0.3 == f; // expected-warning {{floating-point comparison is always false}}
+}
+
+int tautological_FP_compare_commute_inverse(float f) {
+  return 0.3 != f; // expected-warning {{floating-point comparison is always true}}
+}
+
+int tautological_FP_compare_explicit_upcast(float f) {
+  return 0.3 == (double) f; // expected-warning {{floating-point comparison is always false}}
+}
+
+int tautological_FP_compare_explicit_downcast(double d) {
+  return 0.3 == (float) d; // expected-warning {{floating-point comparison is always false}}
+}
+
+int tautological_FP_compare_ignore_parens(float f) {
+  return f == (0.3); // expected-warning {{floating-point comparison is always false}}
+}
+
+#define CST 0.3
+
+int tautological_FP_compare_macro(float f) {
+  return f == CST; // expected-warning {{floating-point comparison is always false}}
 }