[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

// 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

11432

Fix comments?

11439

Maybe consider IgnoreParens().

11450

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

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

26–36

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<
+  "result of floating-point comparison is always false">,
+  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,
   if (Context.getTargetInfo().getTriple().isPPC64())
     CheckPPCMMAType(RetValExp->getType(), ReturnLoc);
 }
 
 //===--- CHECK: Floating-Point comparisons (-Wfloat-equal) ---------------===//
 
 /// 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) {
+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);
+    FPCast = dyn_cast<CastExpr>(R);
+    return FPLiteral && FPCast;
+  };
+
+  if (getCastAndLiteral(LHS, RHS) || getCastAndLiteral(RHS, LHS)) {
+    const Type *SourceTy =
+        Context.getCanonicalType(FPCast->getSubExpr()->getType()).getTypePtr();
+    const Type *TargetTy =
+        Context.getCanonicalType(FPLiteral->getType()).getTypePtr();
+    auto *SourceBT = dyn_cast<BuiltinType>(SourceTy);
+    auto *TargetBT = dyn_cast<BuiltinType>(TargetTy);
+
+    // If the literal cannot be represented in the casted source type, then
+    // this probably is not the intended comparison.
+    if (SourceBT && TargetBT && SourceBT->isFloatingPoint() &&
+        TargetBT->isFloatingPoint()) {
+      bool Lossy;
+      llvm::APFloat TruncC = FPLiteral->getValue();
+      TruncC.convert(Context.getFloatTypeSemantics(QualType(SourceBT, 0)),
+                     llvm::APFloat::rmNearestTiesToEven, &Lossy);
+      if (Lossy) {
+        if (Opcode == BO_EQ)
+          Diag(Loc, diag::warn_tautological_float_compare)
+              << LHS->getSourceRange() << RHS->getSourceRange();
+
+        // TODO: Add warning for != comparison. It's an !isnan() in disguise.
+        return;
+      }
+    }
+  }
+
   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())

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 exactly as a float.
 int f7(float x) {
-  return x == 3.14159; // expected-warning {{comparing}}
+  return x == 3.14159; // expected-warning {{always false}}
+}
+
+// The literal is a double that can be represented exactly 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 {{always false}}
 }