[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

26–48

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

35

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

11474–11475

Fix comments?

11481

Maybe consider IgnoreParens().

11492

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

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

26–48

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

35

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/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_tautological_float_compare : Warning<
+  "floating-point comparison is always false: constant cannot be represented exactly in type %0">,
+  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 *SourceBT = FPCast->getSubExpr()->getType()->getAs<BuiltinType>();
+    auto *TargetBT = FPLiteral->getType()->getAs<BuiltinType>();
+    if (SourceBT && TargetBT && SourceBT->isFloatingPoint() &&
+        TargetBT->isFloatingPoint()) {
+      bool Lossy;
+      llvm::APFloat TargetC = FPLiteral->getValue();
+      TargetC.convert(Context.getFloatTypeSemantics(QualType(SourceBT, 0)),
+                      llvm::APFloat::rmNearestTiesToEven, &Lossy);
+      if (Lossy) {
+        // If the literal cannot be represented in the source type, then a
+        // check for equality is always false.
+        if (Opcode == BO_EQ)
+          Diag(Loc, diag::warn_tautological_float_compare)
+              << FPCast->getSubExpr()->getType() << LHS->getSourceRange()
+              << RHS->getSourceRange();
+
+        // TODO: Add warning for != comparison. It's an !isnan() in disguise.
+        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
 }
 
+// The literal is a double that can't be represented losslessly as a float.
 int f7(float x) {
-  return x == 3.14159; // expected-warning {{comparing}}
+  return x == 3.14159; // expected-warning {{floating-point comparison is always false}}
+}
+
+// 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_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}}
 }