Skip to content

[CLANG] Add warning when INF or NAN are used in a binary operation or as function argument in fast math mode. #76873

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Jan 22, 2024
Merged
4 changes: 4 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,10 @@ Improvements to Clang's diagnostics
- Clang now diagnoses narrowing conversions involving const references.
(`#63151: <https://github.com/llvm/llvm-project/issues/63151>`_).
- Clang now diagnoses unexpanded packs within the template argument lists of function template specializations.
- Clang's ``-Wtautological-logical-constant-compare`` flag now diagnoses
when ``INFINITY`` or ``NAN`` are used in arithmetic operations or function
arguments in floating-points mode where ``INFINITY`` or ``NAN`` don't have the
expected values.
- Clang now diagnoses attempts to bind a bitfield to an NTTP of a reference type as erroneous
converted constant expression and not as a reference to subobject.

Expand Down
6 changes: 6 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -6784,6 +6784,12 @@ def warn_pointer_sub_null_ptr : Warning<
def warn_floatingpoint_eq : Warning<
"comparing floating point with == or != is unsafe">,
InGroup<DiagGroup<"float-equal">>, DefaultIgnore;
def warn_fast_floatingpoint_eq : Warning<
"%select{explicit comparison with|use of}0 %select{infinity|nan}1 "
"%select{will always return 'false'|as a function argument will not always be interpreted as such}0 "
"because %select{infinity|nan}1 will not be produced according to the "
"currently enabled floating-point options">,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, now that I better understand the scope of the functionality, I think we should go with:

use of %select{infinity|NaN}0 results in undefined behavior due to the currently enabled floating-point options

and move the diagnostic out of the tautological compare warning group. You should also update ExprConstant.cpp to reject use of inf/nan in a constant expression with the options disabled.

InGroup<TautologicalConstantCompare>;

def err_setting_eval_method_used_in_unsafe_context : Error <
"%select{'#pragma clang fp eval_method'|option 'ffp-eval-method'}0 cannot be used with "
Expand Down
9 changes: 7 additions & 2 deletions clang/include/clang/Sema/Sema.h
Original file line number Diff line number Diff line change
Expand Up @@ -13901,8 +13901,9 @@ class Sema final {

bool SemaBuiltinVAStart(unsigned BuiltinID, CallExpr *TheCall);
bool SemaBuiltinVAStartARMMicrosoft(CallExpr *Call);
bool SemaBuiltinUnorderedCompare(CallExpr *TheCall);
bool SemaBuiltinFPClassification(CallExpr *TheCall, unsigned NumArgs);
bool SemaBuiltinUnorderedCompare(CallExpr *TheCall, unsigned BuiltinID);
bool SemaBuiltinFPClassification(CallExpr *TheCall, unsigned NumArgs,
unsigned BuiltinID);
bool SemaBuiltinComplex(CallExpr *TheCall);
bool SemaBuiltinVSX(CallExpr *TheCall);
bool SemaBuiltinOSLogFormat(CallExpr *TheCall);
Expand Down Expand Up @@ -14006,6 +14007,8 @@ class Sema final {
SourceRange range,
llvm::SmallBitVector &CheckedVarArgs);

void CheckInfNaNFunction(const CallExpr *Call, const FunctionDecl *FDecl);

void CheckAbsoluteValueFunction(const CallExpr *Call,
const FunctionDecl *FDecl);

Expand All @@ -14032,6 +14035,8 @@ class Sema final {
public:
void CheckFloatComparison(SourceLocation Loc, Expr *LHS, Expr *RHS,
BinaryOperatorKind Opcode);
void CheckInfNaNFloatComparison(SourceLocation Loc, Expr *LHS, Expr *RHS,
BinaryOperatorKind Opcode);

private:
void CheckImplicitConversions(Expr *E, SourceLocation CC = SourceLocation());
Expand Down
78 changes: 72 additions & 6 deletions clang/lib/Sema/SemaChecking.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2169,6 +2169,7 @@ Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID,
ICEArguments &= ~(1 << ArgNo);
}

FPOptions FPO;
switch (BuiltinID) {
case Builtin::BI__builtin___CFStringMakeConstantString:
// CFStringMakeConstantString is currently not implemented for GOFF (i.e.,
Expand Down Expand Up @@ -2245,15 +2246,15 @@ Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID,
case Builtin::BI__builtin_islessequal:
case Builtin::BI__builtin_islessgreater:
case Builtin::BI__builtin_isunordered:
if (SemaBuiltinUnorderedCompare(TheCall))
if (SemaBuiltinUnorderedCompare(TheCall, BuiltinID))
return ExprError();
break;
case Builtin::BI__builtin_fpclassify:
if (SemaBuiltinFPClassification(TheCall, 6))
if (SemaBuiltinFPClassification(TheCall, 6, BuiltinID))
return ExprError();
break;
case Builtin::BI__builtin_isfpclass:
if (SemaBuiltinFPClassification(TheCall, 2))
if (SemaBuiltinFPClassification(TheCall, 2, BuiltinID))
return ExprError();
break;
case Builtin::BI__builtin_isfinite:
Expand All @@ -2267,7 +2268,7 @@ Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID,
case Builtin::BI__builtin_signbit:
case Builtin::BI__builtin_signbitf:
case Builtin::BI__builtin_signbitl:
if (SemaBuiltinFPClassification(TheCall, 1))
if (SemaBuiltinFPClassification(TheCall, 1, BuiltinID))
return ExprError();
break;
case Builtin::BI__builtin_shufflevector:
Expand Down Expand Up @@ -7636,6 +7637,7 @@ bool Sema::CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall,

CheckAbsoluteValueFunction(TheCall, FDecl);
CheckMaxUnsignedZero(TheCall, FDecl);
CheckInfNaNFunction(TheCall, FDecl);

if (getLangOpts().ObjC)
DiagnoseCStringFormatDirectiveInCFAPI(*this, FDecl, Args, NumArgs);
Expand Down Expand Up @@ -9105,10 +9107,15 @@ bool Sema::SemaBuiltinVAStartARMMicrosoft(CallExpr *Call) {

/// SemaBuiltinUnorderedCompare - Handle functions like __builtin_isgreater and
/// friends. This is declared to take (...), so we have to check everything.
bool Sema::SemaBuiltinUnorderedCompare(CallExpr *TheCall) {
bool Sema::SemaBuiltinUnorderedCompare(CallExpr *TheCall, unsigned BuiltinID) {
if (checkArgCount(*this, TheCall, 2))
return true;

if (BuiltinID == Builtin::BI__builtin_isunordered &&
TheCall->getFPFeaturesInEffect(getLangOpts()).getNoHonorNaNs())
Diag(TheCall->getBeginLoc(), diag::warn_fast_floatingpoint_eq)
<< 0 << 1 << 0 << 1;

ExprResult OrigArg0 = TheCall->getArg(0);
ExprResult OrigArg1 = TheCall->getArg(1);

Expand Down Expand Up @@ -9143,10 +9150,22 @@ bool Sema::SemaBuiltinUnorderedCompare(CallExpr *TheCall) {
/// SemaBuiltinSemaBuiltinFPClassification - Handle functions like
/// __builtin_isnan and friends. This is declared to take (...), so we have
/// to check everything.
bool Sema::SemaBuiltinFPClassification(CallExpr *TheCall, unsigned NumArgs) {
bool Sema::SemaBuiltinFPClassification(CallExpr *TheCall, unsigned NumArgs,
unsigned BuiltinID) {
if (checkArgCount(*this, TheCall, NumArgs))
return true;

FPOptions FPO = TheCall->getFPFeaturesInEffect(getLangOpts());
if (FPO.getNoHonorInfs() && (BuiltinID == Builtin::BI__builtin_isfinite ||
BuiltinID == Builtin::BI__builtin_isinf ||
BuiltinID == Builtin::BI__builtin_isinf_sign))
Diag(TheCall->getBeginLoc(), diag::warn_fast_floatingpoint_eq)
<< 0 << 0 << 0 << 0;
if (FPO.getNoHonorNaNs() && (BuiltinID == Builtin::BI__builtin_isnan ||
BuiltinID == Builtin::BI__builtin_isunordered))
Diag(TheCall->getBeginLoc(), diag::warn_fast_floatingpoint_eq)
<< 1 << 1 << 1 << 1;

bool IsFPClass = NumArgs == 2;

// Find out position of floating-point argument.
Expand Down Expand Up @@ -12893,6 +12912,23 @@ static bool IsStdFunction(const FunctionDecl *FDecl,
return true;
}

void Sema::CheckInfNaNFunction(const CallExpr *Call,
const FunctionDecl *FDecl) {
FPOptions FPO = Call->getFPFeaturesInEffect(getLangOpts());
if ((IsStdFunction(FDecl, "isnan") || IsStdFunction(FDecl, "isunordered") ||
(Call->getBuiltinCallee() == Builtin::BI__builtin_nanf)) &&
FPO.getNoHonorNaNs())
Diag(Call->getBeginLoc(), diag::warn_fast_floatingpoint_eq)
<< 1 << 1 << 1 << 1;
else if ((IsStdFunction(FDecl, "isinf") ||
(IsStdFunction(FDecl, "isfinite") ||
(Call->getBuiltinCallee() == Builtin::BI__builtin_inff)) ||
(FDecl->getIdentifier() && FDecl->getName() == "infinity")) &&
FPO.getNoHonorInfs())
Diag(Call->getBeginLoc(), diag::warn_fast_floatingpoint_eq)
<< 1 << 0 << 1 << 0;
}

// Warn when using the wrong abs() function.
void Sema::CheckAbsoluteValueFunction(const CallExpr *Call,
const FunctionDecl *FDecl) {
Expand Down Expand Up @@ -13861,6 +13897,36 @@ Sema::CheckReturnValExpr(Expr *RetValExp, QualType lhsType,
CheckPPCMMAType(RetValExp->getType(), ReturnLoc);
}

/// Diagnose comparison to NAN or INFINITY in fast math modes.
/// The comparison to NaN or INFINITY is always false in
/// fast modes: float evaluation will not result in inf or nan.
void Sema::CheckInfNaNFloatComparison(SourceLocation Loc, Expr *LHS, Expr *RHS,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is 81 characters long.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's 79 chars long.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry. I must have copied the "+ " when I checked it. It just looked longer than the surrounding text.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The basic idea I have is that it's UB to generate a constant infinity or NaN if infinity/NaN is disabled. Instead of performing special logic for float comparisons, I think the logic should live in Sema::ActOnNumericConstant() so that we capture any time a float or infinity constant is used. e.g. inf + 1 should also diagnose.

Then the tautological comparison component to this could be implemented by looking for calls to __builtin_isinf or __builtin_isnan and claim those are tautologically false (as a follow-up patch). WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have actually tried that but I noticed that getSpellingOfSingleCharacterNumericConstant of inf/nan will both return "0" so I will have no way to distinguish between inf + 1 and 0 + 1 and both will generate a warning. Unless there is other way of finding out that the token is nan/inf?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we on that path in the first place? The token length shouldn't be one character long for either inf or nan right? I was thinking you should be able to call NumericLiteralParser::GetFloatValue() within the } else if (Literal.isFloatingLiteral()) { block to get an APFloat that you can call isInfinity() or isNaN() on and then diagnose at that point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AaronBallman Thanks.

BinaryOperatorKind Opcode) {
Expr *LeftExprSansParen = LHS->IgnoreParenImpCasts();
Expr *RightExprSansParen = RHS->IgnoreParenImpCasts();

FPOptions FPO = LHS->getFPFeaturesInEffect(getLangOpts());
bool NoHonorNaNs = FPO.getNoHonorNaNs();
bool NoHonorInfs = FPO.getNoHonorInfs();
llvm::APFloat Value(0.0);

auto IsConstant = [&Value](Expr *E, Expr *ESansParen, ASTContext &Context) {
return !E->isValueDependent() &&
ESansParen->EvaluateAsFloat(Value, Context,
Expr::SE_AllowSideEffects);
};

if (IsConstant(LHS, LeftExprSansParen, Context) &&
((NoHonorNaNs && Value.isNaN()) || (NoHonorInfs && Value.isInfinity())))
Diag(Loc, diag::warn_fast_floatingpoint_eq)
<< 0 << (Value.isNaN() ? 1 : 0) << 0 << (Value.isNaN() ? 1 : 0);

if (IsConstant(RHS, RightExprSansParen, Context) &&
((NoHonorNaNs && Value.isNaN()) || (NoHonorInfs && Value.isInfinity())))
Diag(Loc, diag::warn_fast_floatingpoint_eq)
<< 0 << (Value.isNaN() ? 1 : 0) << 0 << (Value.isNaN() ? 1 : 0);
}

/// 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,
Expand Down
7 changes: 5 additions & 2 deletions clang/lib/Sema/SemaExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13044,9 +13044,12 @@ static QualType checkArithmeticOrEnumeralCompare(Sema &S, ExprResult &LHS,
if (Type->isAnyComplexType() && BinaryOperator::isRelationalOp(Opc))
return S.InvalidOperands(Loc, LHS, RHS);

// Check for comparisons of floating point operands using != and ==.
if (Type->hasFloatingRepresentation())
if (Type->hasFloatingRepresentation()) {
// Check for comparisons to NAN or INFINITY in fast math mode.
S.CheckInfNaNFloatComparison(Loc, LHS.get(), RHS.get(), Opc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be done in CheckFloatComparison()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// Check for comparisons of floating point operands using != and ==.
S.CheckFloatComparison(Loc, LHS.get(), RHS.get(), Opc);
}

// The result of comparisons is 'bool' in C++, 'int' in C.
return S.Context.getLogicalOperationType();
Expand Down
Loading