[PATCH] D156506: [clang][Interp] Check floating results for NaNs
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG4b5fe9c42d94: [clang][Interp] Check floating results for
NaNs (authored by tbaeder).
Changed prior to commit:
https://reviews.llvm.org/D156506?vs=545023&id=556407#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156506/new/
https://reviews.llvm.org/D156506
Files:
clang/lib/AST/Interp/Interp.cpp
clang/lib/AST/Interp/Interp.h
clang/test/AST/Interp/floats.cpp
Index: clang/test/AST/Interp/floats.cpp
===
--- clang/test/AST/Interp/floats.cpp
+++ clang/test/AST/Interp/floats.cpp
@@ -202,3 +202,18 @@
static_assert(!(inf < nan), "");
static_assert(!(inf > nan), "");
}
+
+namespace nan {
+ constexpr double nan = __builtin_nan("");
+ static_assert(nan);
+
+ constexpr double D1 = 1 + nan; // ref-error {{must be initialized by a constant expression}} \
+ // ref-note {{produces a NaN}} \
+ // expected-error {{must be initialized by a constant expression}} \
+ // expected-note {{produces a NaN}}
+
+ constexpr double D2 = __builtin_inf() / __builtin_inf(); // ref-error {{must be initialized by a constant expression}} \
+ // ref-note {{produces a NaN}} \
+ // expected-error {{must be initialized by a constant expression}} \
+ // expected-note {{produces a NaN}}
+}
Index: clang/lib/AST/Interp/Interp.h
===
--- clang/lib/AST/Interp/Interp.h
+++ clang/lib/AST/Interp/Interp.h
@@ -172,7 +172,8 @@
/// Checks if the result of a floating-point operation is valid
/// in the current context.
-bool CheckFloatResult(InterpState &S, CodePtr OpPC, APFloat::opStatus Status);
+bool CheckFloatResult(InterpState &S, CodePtr OpPC, const Floating &Result,
+ APFloat::opStatus Status);
/// Interpreter entry point.
bool Interpret(InterpState &S, APValue &Result);
@@ -304,7 +305,7 @@
Floating Result;
auto Status = Floating::add(LHS, RHS, RM, &Result);
S.Stk.push(Result);
- return CheckFloatResult(S, OpPC, Status);
+ return CheckFloatResult(S, OpPC, Result, Status);
}
template ::T>
@@ -322,7 +323,7 @@
Floating Result;
auto Status = Floating::sub(LHS, RHS, RM, &Result);
S.Stk.push(Result);
- return CheckFloatResult(S, OpPC, Status);
+ return CheckFloatResult(S, OpPC, Result, Status);
}
template ::T>
@@ -340,7 +341,7 @@
Floating Result;
auto Status = Floating::mul(LHS, RHS, RM, &Result);
S.Stk.push(Result);
- return CheckFloatResult(S, OpPC, Status);
+ return CheckFloatResult(S, OpPC, Result, Status);
}
/// 1) Pops the RHS from the stack.
/// 2) Pops the LHS from the stack.
@@ -443,7 +444,7 @@
Floating Result;
auto Status = Floating::div(LHS, RHS, RM, &Result);
S.Stk.push(Result);
- return CheckFloatResult(S, OpPC, Status);
+ return CheckFloatResult(S, OpPC, Result, Status);
}
//===--===//
@@ -622,7 +623,7 @@
Ptr.deref() = Result;
- return CheckFloatResult(S, OpPC, Status);
+ return CheckFloatResult(S, OpPC, Result, Status);
}
inline bool Incf(InterpState &S, CodePtr OpPC, llvm::RoundingMode RM) {
@@ -1525,7 +1526,7 @@
auto Status = Floating::fromIntegral(FromAP, *Sem, RM, Result);
S.Stk.push(Result);
- return CheckFloatResult(S, OpPC, Status);
+ return CheckFloatResult(S, OpPC, Result, Status);
}
template ::T>
@@ -1550,7 +1551,7 @@
}
S.Stk.push(T(Result));
-return CheckFloatResult(S, OpPC, Status);
+return CheckFloatResult(S, OpPC, F, Status);
}
}
Index: clang/lib/AST/Interp/Interp.cpp
===
--- clang/lib/AST/Interp/Interp.cpp
+++ clang/lib/AST/Interp/Interp.cpp
@@ -495,13 +495,25 @@
return false;
}
-bool CheckFloatResult(InterpState &S, CodePtr OpPC, APFloat::opStatus Status) {
+bool CheckFloatResult(InterpState &S, CodePtr OpPC, const Floating &Result,
+ APFloat::opStatus Status) {
+ const SourceInfo &E = S.Current->getSource(OpPC);
+
+ // [expr.pre]p4:
+ // If during the evaluation of an expression, the result is not
+ // mathematically defined [...], the behavior is undefined.
+ // FIXME: C++ rules require us to not conform to IEEE 754 here.
+ if (Result.isNan()) {
+S.CCEDiag(E, diag::note_constexpr_float_arithmetic)
+<< /*NaN=*/true << S.Current->getRange(OpPC);
+return S.noteUndefinedBehavior();
+ }
+
// In a constant context, assume that any dynamic rounding mode or FP
// exception state matches the default fl
[PATCH] D156506: [clang][Interp] Check floating results for NaNs
tbaeder added a comment. I thought this review wasn't clear at all on what's supposed to happen but since https://reviews.llvm.org/D157072 is going nowhere, I'll just push this one in the next few days. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156506/new/ https://reviews.llvm.org/D156506 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156506: [clang][Interp] Check floating results for NaNs
jcranmer-intel added inline comments.
Comment at: clang/lib/AST/Interp/Interp.cpp:503
+ // If during the evaluation of an expression, the result is not
+ // mathematically defined [...], the behavior is undefined.
+ // FIXME: C++ rules require us to not conform to IEEE 754 here.
tbaeder wrote:
> @jcranmer-intel Doesn't this comment (which I've coped from
> `ExprConstant.cpp`) contradict what you said about not checking the result?
Immediately following that in the specification is this:
> [Note 3: Treatment of division by zero, forming a remainder using a zero
> divisor, and all floating-point exceptions varies among machines, and is
> sometimes adjustable by a library function. — end note]
The current C++ specification is rather clear about its unclarity on
floating-point. Also note that IEEE 754 defines floating-point data as
consisting of {-inf, ..., -0} union {+0, ..., +inf} union {NaN}. So NaN is
arguable to be a well-defined mathematical result, if you consider that
floating-point types don't model real numbers but an approximation of real
numbers (just as unsigned integers model not integers but integers mod 2^N).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156506/new/
https://reviews.llvm.org/D156506
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156506: [clang][Interp] Check floating results for NaNs
tbaeder added inline comments. Comment at: clang/lib/AST/Interp/Interp.cpp:503 + // If during the evaluation of an expression, the result is not + // mathematically defined [...], the behavior is undefined. + // FIXME: C++ rules require us to not conform to IEEE 754 here. @jcranmer-intel Doesn't this comment (which I've coped from `ExprConstant.cpp`) contradict what you said about not checking the result? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156506/new/ https://reviews.llvm.org/D156506 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156506: [clang][Interp] Check floating results for NaNs
jcranmer-intel added a comment.
You definitely don't want these rules to apply to all qNaNs. It's when an input
operand is an sNaN for many operations. Note that the result of an operation
with an sNaN as input (and FP result type) is a qNaN output, and the only times
that you get an sNaN output are the times when you never signal (I think), so
checking the result type is incorrect.
Comment at: clang/lib/AST/Interp/Interp.cpp:534-539
if ((Status & APFloat::opStatus::opInvalidOp) &&
FPO.getExceptionMode() != LangOptions::FPE_Ignore) {
// There is no usefully definable result.
S.FFDiag(E);
return false;
}
A further note is that sNaNs signal `FE_INVALID` when used, so sNaN should
generally fall into this if statement in particular.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156506/new/
https://reviews.llvm.org/D156506
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156506: [clang][Interp] Check floating results for NaNs
aaron.ballman accepted this revision. aaron.ballman added a subscriber: jcranmer-intel. aaron.ballman added a comment. This revision is now accepted and ready to land. > This does not handle the builtin functions yet, since I'm not sure if I > should check for all nans or only signaling ones yet. I think it's only signaling ones, but CC @jcranmer-intel for a better-informed answer. The changes here LGTM though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156506/new/ https://reviews.llvm.org/D156506 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156506: [clang][Interp] Check floating results for NaNs
tbaeder created this revision.
tbaeder added reviewers: aaron.ballman, erichkeane, shafik, cor3ntin.
Herald added a project: All.
tbaeder requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
This does not handle the builtin functions yet, since I'm not sure if I should
check for all nans or only signaling ones yet.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D156506
Files:
clang/lib/AST/Interp/Interp.cpp
clang/lib/AST/Interp/Interp.h
clang/test/AST/Interp/floats.cpp
Index: clang/test/AST/Interp/floats.cpp
===
--- clang/test/AST/Interp/floats.cpp
+++ clang/test/AST/Interp/floats.cpp
@@ -161,3 +161,18 @@
static_assert(!(inf < nan), "");
static_assert(!(inf > nan), "");
}
+
+namespace nan {
+ constexpr double nan = __builtin_nan("");
+ static_assert(nan);
+
+ constexpr double D1 = 1 + nan; // ref-error {{must be initialized by a constant expression}} \
+ // ref-note {{produces a NaN}} \
+ // expected-error {{must be initialized by a constant expression}} \
+ // expected-note {{produces a NaN}}
+
+ constexpr double D2 = __builtin_inf() / __builtin_inf(); // ref-error {{must be initialized by a constant expression}} \
+ // ref-note {{produces a NaN}} \
+ // expected-error {{must be initialized by a constant expression}} \
+ // expected-note {{produces a NaN}}
+}
Index: clang/lib/AST/Interp/Interp.h
===
--- clang/lib/AST/Interp/Interp.h
+++ clang/lib/AST/Interp/Interp.h
@@ -171,7 +171,8 @@
/// Checks if the result is a floating-point operation is valid
/// in the current context.
-bool CheckFloatResult(InterpState &S, CodePtr OpPC, APFloat::opStatus Status);
+bool CheckFloatResult(InterpState &S, CodePtr OpPC, const Floating &Result,
+ APFloat::opStatus Status);
bool CheckBitcast(InterpState &S, CodePtr OpPC, unsigned IndeterminateBits,
bool TargetIsUCharOrByte);
@@ -316,7 +317,7 @@
Floating Result;
auto Status = Floating::add(LHS, RHS, RM, &Result);
S.Stk.push(Result);
- return CheckFloatResult(S, OpPC, Status);
+ return CheckFloatResult(S, OpPC, Result, Status);
}
template ::T>
@@ -334,7 +335,7 @@
Floating Result;
auto Status = Floating::sub(LHS, RHS, RM, &Result);
S.Stk.push(Result);
- return CheckFloatResult(S, OpPC, Status);
+ return CheckFloatResult(S, OpPC, Result, Status);
}
template ::T>
@@ -352,7 +353,7 @@
Floating Result;
auto Status = Floating::mul(LHS, RHS, RM, &Result);
S.Stk.push(Result);
- return CheckFloatResult(S, OpPC, Status);
+ return CheckFloatResult(S, OpPC, Result, Status);
}
/// 1) Pops the RHS from the stack.
/// 2) Pops the LHS from the stack.
@@ -455,7 +456,7 @@
Floating Result;
auto Status = Floating::div(LHS, RHS, RM, &Result);
S.Stk.push(Result);
- return CheckFloatResult(S, OpPC, Status);
+ return CheckFloatResult(S, OpPC, Result, Status);
}
//===--===//
@@ -632,7 +633,7 @@
Ptr.deref() = Result;
- return CheckFloatResult(S, OpPC, Status);
+ return CheckFloatResult(S, OpPC, Result, Status);
}
inline bool Incf(InterpState &S, CodePtr OpPC, llvm::RoundingMode RM) {
@@ -1586,7 +1587,7 @@
auto Status = Floating::fromIntegral(FromAP, *Sem, RM, Result);
S.Stk.push(Result);
- return CheckFloatResult(S, OpPC, Status);
+ return CheckFloatResult(S, OpPC, Result, Status);
}
template ::T>
@@ -1611,7 +1612,7 @@
}
S.Stk.push(T(Result));
-return CheckFloatResult(S, OpPC, Status);
+return CheckFloatResult(S, OpPC, F, Status);
}
}
Index: clang/lib/AST/Interp/Interp.cpp
===
--- clang/lib/AST/Interp/Interp.cpp
+++ clang/lib/AST/Interp/Interp.cpp
@@ -494,13 +494,25 @@
return false;
}
-bool CheckFloatResult(InterpState &S, CodePtr OpPC, APFloat::opStatus Status) {
+bool CheckFloatResult(InterpState &S, CodePtr OpPC, const Floating &Result,
+ APFloat::opStatus Status) {
+ const SourceInfo &E = S.Current->getSource(OpPC);
+
+ // [expr.pre]p4:
+ // If during the evaluation of an expression, the result is not
+ // mathematically defined [...], the behavior is undefined.
+ // FIXME: C++ rules require us to not conform to IEEE 754 here.
+ if (Result.isNan()) {
+S.CCEDiag(E, diag::note_constexpr_float_arithmetic)
+<< /*NaN=*/true << S.Current->getRange(OpPC);
+return S.noteUndefinedBehavior();
+ }
+
// In a constant context, assume that any dynamic rounding mode or FP
// exception
