-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[clang][C23] Allow NaN in constant evaluation #167892
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: yronglin <yronglin777@gmail.com>
|
@llvm/pr-subscribers-clang Author: None (yronglin) ChangesAllow NaN in constant evaluation even if it's undefined behavior in C mode. Fixes #161806. Full diff: https://github.com/llvm/llvm-project/pull/167892.diff 4 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 6b396e7ba63f3..d19cc041b5082 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -222,6 +222,8 @@ C23 Feature Support
the same translation unit but from different types.
- ``-MG`` now silences the "file not found" errors with ``#embed`` when
scanning for dependencies and encountering an unknown file. #GH165632
+- Allow NaN in constant expression evaluation to maintain consistency with
+ GCC in behavior, even though it's an undefined behavior. #GH161806
Non-comprehensive list of changes in this release
-------------------------------------------------
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 8579e51e45697..53ed180f377a1 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -2605,11 +2605,18 @@ APValue *VarDecl::evaluateValueImpl(SmallVectorImpl<PartialDiagnosticAt> &Notes,
// a constant initializer if we produced notes. In that case, we can't keep
// the result, because it may only be correct under the assumption that the
// initializer is a constant context.
- if (IsConstantInitialization &&
- (Ctx.getLangOpts().CPlusPlus ||
- (isConstexpr() && Ctx.getLangOpts().C23)) &&
- !Notes.empty())
- Result = false;
+ if (IsConstantInitialization && !Notes.empty()) {
+ if (getLangOpts().CPlusPlus)
+ Result = false;
+
+ // Even though hitting an NaN during constant evaluation is an undefined
+ // behavior, we expect to maintain consistency with GCC in behavior, that
+ // is, allow NaN to appear in constant evaluation.
+ bool isNaN =
+ Eval->Evaluated.isFloat() && Eval->Evaluated.getFloat().isNaN();
+ if (getLangOpts().C23 && !isNaN)
+ Result = false;
+ }
// Ensure the computed APValue is cleaned up later if evaluation succeeded,
// or that it's empty (so that there's nothing to clean up) if evaluation
@@ -2676,8 +2683,14 @@ bool VarDecl::checkForConstantInitialization(
assert(!getInit()->isValueDependent());
// Evaluate the initializer to check whether it's a constant expression.
- Eval->HasConstantInitialization =
- evaluateValueImpl(Notes, true) && Notes.empty();
+ auto *Result = evaluateValueImpl(Notes, true);
+
+ // Even though hitting an NaN during constant evaluation is an undefined
+ // behavior, we expect to maintain consistency with GCC in behavior, that is,
+ // allow NaN to appear in constant evaluation.
+ bool isNaN = Result && Result->isFloat() && Result->getFloat().isNaN();
+ bool AllowNaN = getLangOpts().C23 && isNaN;
+ Eval->HasConstantInitialization = Result && (Notes.empty() || AllowNaN);
// If evaluation as a constant initializer failed, allow re-evaluation as a
// non-constant initializer if we later find we want the value.
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 972d9fe3b5e4f..bd62fbb2edce4 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -3056,7 +3056,8 @@ static bool handleFloatFloatBinOp(EvalInfo &Info, const BinaryOperator *E,
// FIXME: C++ rules require us to not conform to IEEE 754 here.
if (LHS.isNaN()) {
Info.CCEDiag(E, diag::note_constexpr_float_arithmetic) << LHS.isNaN();
- return Info.noteUndefinedBehavior();
+ bool keepEvaluatingAfterUB = Info.noteUndefinedBehavior();
+ return Info.Ctx.getLangOpts().C23 || keepEvaluatingAfterUB;
}
return checkFloatingPointResult(Info, E, St);
@@ -19707,7 +19708,8 @@ bool Expr::EvaluateAsInitializer(APValue &Value, const ASTContext &Ctx,
EvalInfo Info(Ctx, EStatus,
(IsConstantInitialization &&
- (Ctx.getLangOpts().CPlusPlus || Ctx.getLangOpts().C23))
+ (Ctx.getLangOpts().CPlusPlus ||
+ (Ctx.getLangOpts().C23 && VD->isConstexpr())))
? EvaluationMode::ConstantExpression
: EvaluationMode::ConstantFold);
Info.setEvaluatingDecl(VD, Value);
diff --git a/clang/test/AST/const-nan.c b/clang/test/AST/const-nan.c
new file mode 100644
index 0000000000000..c732abac4a90d
--- /dev/null
+++ b/clang/test/AST/const-nan.c
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -std=c23 -triple i386-linux %s -fsyntax-only -verify
+// RUN: %clang_cc1 -std=c23 -emit-llvm -triple i386-linux %s -o - | FileCheck %s
+
+// expected-no-diagnostics
+
+// CHECK: @[[CONST:.*]] = private unnamed_addr constant [1 x float] [float 0x7FF8000000000000], align 4
+// CHECK: @[[F_X:.*]] = internal global float 0x7FF8000000000000, align 4
+#pragma STDC FENV_ACCESS ON
+void f(void)
+{
+ // CHECK: %[[V:.*]] = alloca double, align 8
+ // CHECK: %[[W:.*]] = alloca [1 x float], align 4
+ // CHECK: %[[Y:.*]] = alloca float, align 4
+ // CHECK: %[[Z:.*]] = alloca double, align 8
+
+ // CHECK: store double 0x7FF8000000000000, ptr %[[V]], align 8
+ constexpr double v = 0.0/0.0; // does not raise an exception
+
+ // CHECK: call void @llvm.memcpy.p0.p0.i32(ptr align 4 %[[W]], ptr align 4 @[[CONST]], i32 4, i1 false)
+ float w[] = { 0.0f/0.0f }; // raises an exception
+
+ // F_X
+ static float x = 0.0f/0.0f; // does not raise an exception
+
+ // CHECK: %[[DIV:.*]] = call float @llvm.experimental.constrained.fdiv.f32(float 0.000000e+00, float 0.000000e+00, metadata !"round.dynamic", metadata !"fpexcept.strict")
+ // CHECK: store float %[[DIV]], ptr %[[Y]], align 4
+ float y = 0.0f/0.0f; // raises an exception
+
+ // CHECK: %[[DIV1:.*]] = call double @llvm.experimental.constrained.fdiv.f64(double 0.000000e+00, double 0.000000e+00, metadata !"round.dynamic", metadata !"fpexcept.strict")
+ // CHECK: store double %[[DIV1]], ptr %[[Z]], align 8
+ double z = 0.0/0.0; // raises an exception
+}
|
tbaederr
left a comment
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.
Variables should start with an uppercase character.
As a side note, I hate this. It really shouldn't be that hard to decide whether producing a nan is UB or not. I don't think following GCC is that important in this case. But I'll let someone else decide.
| @@ -0,0 +1,32 @@ | |||
| // RUN: %clang_cc1 -std=c23 -triple i386-linux %s -fsyntax-only -verify | |||
| // RUN: %clang_cc1 -std=c23 -emit-llvm -triple i386-linux %s -o - | FileCheck %s | |||
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.
-fexperimental-new-constant-interpreter?
I think the important case to support is the static initialization case. e.g., |
| the same translation unit but from different types. | ||
| - ``-MG`` now silences the "file not found" errors with ``#embed`` when | ||
| scanning for dependencies and encountering an unknown file. #GH165632 | ||
| - Allow NaN in constant expression evaluation to maintain consistency with |
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.
I'm not convinced this is the direction we want to go. The original issue was about not being able to codegen the static initializer, but this PR goes beyond that and allows NaN in any constant expression, including ones nobody accepts: https://godbolt.org/z/9PPqn76a9
This is because it runs afoul of explicit requirements in the standard:
https://eel.is/c++draft/expr.const#10.8
https://eel.is/c++draft/expr.mul#4.sentence-2
I think we only want to fix up the codegen situation.
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.
Implementations are permitted to define behavior that the standard leaves undefined, and I would certainly say that floating point division by zero falls into that category. This is UB solely because the C standard preceded the IEEE FP consensus, and C++ inherited that for complicated reasons.
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.
That said, if we're going to allow this, it admits NaN template arguments, and we need to test that template argument equality is based on the level 4 specification level of IEEE 754, i.e. that it uses APFloat::bitwiseIsEqual and not APFloat::compare. Fortunately, mangling is already based on that.
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.
C++ specifically says we can't treat divide by zero as a constant expression; there isn't any leeway in the rules.
C technically doesn't have a corresponding rule; it just says "Each constant expression shall evaluate to a constant that is in the range of representable values for its type." Which... arguably means we can define the meaning of anything that's otherwise legal in a constant expression. Like, we could accept constexpr int a[2] = {0}; constexpr int b = a[2]; static_assert(b==0);. (NOTE: edited to fix typo.) I'm wary of making C and C++ diverge here; constant expressions are confusing enough as it is. But we could, I guess.
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.
C++ specifically says we can't treat divide by zero as a constant expression; there isn't any leeway in the rules.
+1 to this, it's even called out in the footnote attached to the normative wording: https://eel.is/c++draft/expr.const#footnote-68
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.
Huh, okay. That's a terrible decision, but it's the committee's right to make it.
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.
Huh, okay. That's a terrible decision, but it's the committee's right to make it.
I think that's why the paper Hubert linked to is trying to change the status quo to match implementation behavior, so it's possible this situation will improve in the future.
Does that mean we want to hold off on C++ changes here until the committee has an opinion on that paper?
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.
Is there a belief that the committee will not extend the design principles it took for the library calls to the core language and essentially match the GCC behaviour column in https://web.archive.org/web/20251117163349/https://isocpp.org/files/papers/D3899R0.html#implementation-divergence?
I am not aware of any alternative proposals being championed.
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.
I don't know what to expect out of the committee, but allowing (quiet) NaN in constant expressions makes a lot of sense to me, at least for IEEE math. The whole point to a quiet NaN (AIUI) is so that you can let them flow through a mathematical calculation to the end result. And there's no reason to believe that end result is unexpected, either; if it was the case, then there would be no push for constexpr support for the library functions which produce those values.
Signalling NaN is the one where you want to be alerted as soon as the calculation ends up with that result. That one makes a lot of sense to me to fail immediately in constant expressions.
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.
I agree with Aaron about what the semantics should be for platforms using IEEE FP. It is an explicit goal of IEEE FP that arithmetic be well-defined, consistent, and portable, and it out of keeping with that for the compiler to act like FLT_MAX + FLT_MAX does not consistently yield +Inf or that +Inf + -Inf does not consistently yield NaN. It might be reasonable to warn about non-trivial, non-dependent expressions that always produce an infinity or NaN, but C++ constexpr left that kind of purely local constraint behind a long time ago.
I understand that the committee doesn't want to mandate IEEE FP, but the committee should not be in the business of blocking reasonable behavior for the predominant FP design. Someone porting FP code to a non-IEEE platform is going to face much worse problems than a few constexprs potentially breaking.
Allowing it to be implementation-defined would give implementations leeway to vary based on things like the active FP exceptions mode.
| // behavior, we expect to maintain consistency with GCC in behavior, that | ||
| // is, allow NaN to appear in constant evaluation. | ||
| bool isNaN = | ||
| Eval->Evaluated.isFloat() && Eval->Evaluated.getFloat().isNaN(); |
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.
We don't want to special-case NaN here. Whether the end result of an evaluation is NaN isn't really closely related to whether it has any divide by zero operations.
| Info.CCEDiag(E, diag::note_constexpr_float_arithmetic) << LHS.isNaN(); | ||
| return Info.noteUndefinedBehavior(); | ||
| bool keepEvaluatingAfterUB = Info.noteUndefinedBehavior(); | ||
| return Info.Ctx.getLangOpts().C23 || keepEvaluatingAfterUB; |
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.
We probably just want to delete the call to noteUndefinedBehavior here. The CCEDiag should be sufficient to trigger appropriate error messages without treating the expression as unevaluatable.
I think we really should go to WG14/WG21/SC22 before even considering anything in this space. It has a lot of ramifications:
|
Allow NaN in constant evaluation even if it's undefined behavior in C mode.
Fixes #161806.