-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[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
Changes from 6 commits
7dbaf03
f4aa379
948ace7
d82f569
b603724
51db96f
0b2cec2
71431de
c874335
c0bd9e5
3506625
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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., | ||
|
@@ -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: | ||
|
@@ -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: | ||
|
@@ -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); | ||
|
@@ -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); | ||
|
||
|
@@ -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. | ||
|
@@ -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) { | ||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line is 81 characters long. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's 79 chars long. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Then the tautological comparison component to this could be implemented by looking for calls to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have actually tried that but I noticed that There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be done in CheckFloatComparison()? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
There was a problem hiding this comment.
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.