[Clang] Treat constexpr-unknown value as invalid in `EvaluateAsInitializer` #128409

dtcxzyw · 2025-02-23T09:07:09Z

It is an alternative to #127525.
Close #127475.

clang/test/SemaCXX/constant-expression-cxx11.cpp

efriedma-quic · 2025-02-24T21:32:36Z

clang/lib/AST/ExprConstant.cpp

    if (!Result)
      Result = &Info.CurrentCall->createConstexprUnknownAPValues(VD, Base);
-    else
-      Result->setConstexprUnknown();


Deleting this makes a lot of sense; I was really confused trying to parse what this line in the original patch was supposed to do.

clang/lib/AST/Decl.cpp

dtcxzyw · 2025-02-26T16:11:52Z

clang/lib/AST/ExprConstant.cpp

+      const auto *NewVD = Base.dyn_cast<const ValueDecl *>();
+      if (!NewVD)
+        NewVD = VD;
+      Info.FFDiag(getExprLoc(), diag::note_constexpr_var_init_non_constant, 1)


Ideally, we should emit a note here:

llvm-project/clang/lib/AST/ExprConstant.cpp

Lines 3581 to 3584 in a955426

if (AllowConstexprUnknown) {

Result = &Info.CurrentCall->createConstexprUnknownAPValues(VD, Base);

return true;

}

But I don't know how to add a "lazy" note (i.e., these notes will be shown if an error occurs).

I'm not sure what you mean by a "lazy" note; the whole point of constexpr-unknown is that it's not an error at the point it's constructed.

If you need more information to generate a good diagnostic, you could pass that information to createConstexprUnknownAPValues, and store it in the APValue itself, I guess. Computing the whole diagnostic in advance seems like it would be too expensive, though.

I don't think it's a big deal if we generate slightly different diagnostics depending on whether we're in C++23 mode, as long as we generate some explanation for what's happening.

If you need more information to generate a good diagnostic, you could pass that information to createConstexprUnknownAPValues, and store it in the APValue itself, I guess. Computing the whole diagnostic in advance seems like it would be too expensive, though.

That is what the current implementation does.

llvmbot · 2025-02-26T16:13:15Z

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Yingwei Zheng (dtcxzyw)

Changes

It is an alternative to #127525.
Close #127475.

Full diff: https://github.com/llvm/llvm-project/pull/128409.diff

3 Files Affected:

(modified) clang/lib/AST/ExprConstant.cpp (+12-2)
(modified) clang/lib/CodeGen/CGExprConstant.cpp (+5-1)
(added) clang/test/CodeGenCXX/cxx23-p2280r4.cpp (+15)

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 6ccb6e23f8d2f..42801dc4e18fa 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -3628,8 +3628,6 @@ static bool evaluateVarDeclInit(EvalInfo &Info, const Expr *E,
   if (AllowConstexprUnknown) {
     if (!Result)
       Result = &Info.CurrentCall->createConstexprUnknownAPValues(VD, Base);
-    else
-      Result->setConstexprUnknown();
   }
   return true;
 }
@@ -17000,6 +16998,18 @@ bool Expr::EvaluateAsInitializer(APValue &Value, const ASTContext &Ctx,
 
     if (!Info.discardCleanups())
       llvm_unreachable("Unhandled cleanup; missing full expression marker?");
+
+    if (Value.allowConstexprUnknown()) {
+      assert(Value.isLValue() && "Expected an lvalue");
+      auto Base = Value.getLValueBase();
+      const auto *NewVD = Base.dyn_cast<const ValueDecl *>();
+      if (!NewVD)
+        NewVD = VD;
+      Info.FFDiag(getExprLoc(), diag::note_constexpr_var_init_non_constant, 1)
+          << NewVD;
+      NoteLValueLocation(Info, Base);
+      return false;
+    }
   }
 
   return CheckConstantExpression(Info, DeclLoc, DeclTy, Value,
diff --git a/clang/lib/CodeGen/CGExprConstant.cpp b/clang/lib/CodeGen/CGExprConstant.cpp
index ee5874b26f534..0295f703b9061 100644
--- a/clang/lib/CodeGen/CGExprConstant.cpp
+++ b/clang/lib/CodeGen/CGExprConstant.cpp
@@ -1883,8 +1883,12 @@ llvm::Constant *ConstantEmitter::tryEmitPrivateForVarInit(const VarDecl &D) {
 
   // Try to emit the initializer.  Note that this can allow some things that
   // are not allowed by tryEmitPrivateForMemory alone.
-  if (APValue *value = D.evaluateValue())
+  // Bail out on constexpr-unknown values since they are invalid in CodeGen.
+  if (APValue *value = D.evaluateValue()) {
+    assert(!value->allowConstexprUnknown() &&
+           "Constexpr unknown values are not allowed in CodeGen");
     return tryEmitPrivateForMemory(*value, destType);
+  }
 
   return nullptr;
 }
diff --git a/clang/test/CodeGenCXX/cxx23-p2280r4.cpp b/clang/test/CodeGenCXX/cxx23-p2280r4.cpp
new file mode 100644
index 0000000000000..d5409be451df0
--- /dev/null
+++ b/clang/test/CodeGenCXX/cxx23-p2280r4.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++23 %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++17 %s -emit-llvm -o - | FileCheck %s
+
+extern int& s;
+
+// CHECK: @_Z4testv()
+// CHECK-NEXT: entry:
+// CHECK-NEXT: [[I:%.*]] = alloca ptr, align {{.*}}
+// CHECK-NEXT: [[X:%.*]] = load ptr, ptr @s, align {{.*}}
+// CHECK-NEXT: store ptr [[X]], ptr [[I]], align {{.*}}
+int& test() {
+  auto &i = s;
+  return i;
+}

efriedma-quic

Should the other Expr::Evaluate* entry points have the same bailout? Do you want to do that in this patch, or a followup?

clang/lib/CodeGen/CGExprConstant.cpp

efriedma-quic · 2025-02-26T23:28:03Z

clang/lib/AST/ExprConstant.cpp

+      const auto *NewVD = Base.dyn_cast<const ValueDecl *>();
+      if (!NewVD)
+        NewVD = VD;
+      Info.FFDiag(getExprLoc(), diag::note_constexpr_var_init_non_constant, 1)


I'm not sure what you mean by a "lazy" note; the whole point of constexpr-unknown is that it's not an error at the point it's constructed.

If you need more information to generate a good diagnostic, you could pass that information to createConstexprUnknownAPValues, and store it in the APValue itself, I guess. Computing the whole diagnostic in advance seems like it would be too expensive, though.

I don't think it's a big deal if we generate slightly different diagnostics depending on whether we're in C++23 mode, as long as we generate some explanation for what's happening.

dtcxzyw · 2025-02-27T06:08:05Z

Should the other Expr::Evaluate* entry points have the same bailout? Do you want to do that in this patch, or a followup?

Sorry I don't have enough bandwidth to work on other potential constexpr-unknown-related issues. I just want to close #127475 as soon as possible.

efriedma-quic

LGTM

shafik

I get that we are allowing constexpr unknown values into codegen and that is a mistake but I don't totally follow the fix. Can you go into more details?

If I am reading the code correctly there should be some constexpr cases that generate a diagnostic, can we verify that with a test?

efriedma-quic · 2025-03-01T01:16:05Z

Here's a testcase that's should generate a diagnostic, but doesn't without this patch:

int &ff(); int &x = ff(); constinit int &z = x;

Some related testcases are a little weird... consider the following:

extern int &x; int &z = x;

There's a gap between checking for constant expressions, and codegen, due to the following diagnostic; it triggers in constexpr, but allows evaluating for the value because a CCEDiag doesn't stop the evaluation:

llvm-project/clang/lib/AST/ExprConstant.cpp

Line 3607 in 52c8159

Info.CCEDiag(E);

.

dtcxzyw · 2025-03-01T09:16:56Z

If I am reading the code correctly there should be some constexpr cases that generate a diagnostic, can we verify that with a test?

It has been tested by the following case:

extern int &Recurse1;
int &Recurse2 = Recurse1;
int &Recurse1 = Recurse2;
constexpr int &Recurse3 = Recurse2;

Without diagnostic-related code in this patch, we will lose some notes on Recurse2.

Mishura4 · 2025-03-04T19:08:43Z

So it looks like 20.1.0 is planned to release tomorrow as there are "no critical issues"... https://discourse.llvm.org/t/20-1-0-release-update-releasing-1-week-early-tomorrow/84932
I'm not an LLVM maintainer so I don't know if it's really my place to say this, but I think you're about to see a lot more GitHub issues filed related to this 😭 I don't think extern references are that uncommon

shafik

I am happy to see a fix here but I would still like some more context on whether we think this still leave some cases uncaught or not.

frederick-vs-ja · 2025-03-05T06:22:02Z

/cherry-pick 27757fb

llvmbot · 2025-03-05T06:27:38Z

/pull-request #129836

efriedma-quic · 2025-03-05T07:46:52Z

Opened #129844 with a case that still fails. Other entry-points to constant evaluation need similar treatment to EvaluateAsInitializer.

efriedma-quic · 2025-03-05T07:57:27Z

And the check for allowConstexprUnknown() isn't catching all the relevant cases; opened #129845.

…consistently. Perform the check for constexpr-unknown values in the same place we perform checks for other values which don't count as constant expressions. While I'm here, also fix a rejects-valid with a reference that doesn't have an initializer. This diagnostic was also covering up some of the bugs here. The existing behavior with -fexperimental-new-constant-interpreter seems to be correct, but the diagnostics are slightly different; it would be helpful if someone could check on that as a followup. Followup to llvm#128409. Fixes llvm#129844. Fixes llvm#129845.

…consistently (#129952) Perform the check for constexpr-unknown values in the same place we perform checks for other values which don't count as constant expressions. While I'm here, also fix a rejects-valid with a reference that doesn't have an initializer. This diagnostic was also covering up some of the bugs here. The existing behavior with -fexperimental-new-constant-interpreter seems to be correct, but the diagnostics are slightly different; it would be helpful if someone could check on that as a followup. Followup to #128409. Fixes #129844. Fixes #129845.

…lizer` (llvm#128409) It is an alternative to llvm#127525. Close llvm#127475.

…consistently (llvm#129952) Perform the check for constexpr-unknown values in the same place we perform checks for other values which don't count as constant expressions. While I'm here, also fix a rejects-valid with a reference that doesn't have an initializer. This diagnostic was also covering up some of the bugs here. The existing behavior with -fexperimental-new-constant-interpreter seems to be correct, but the diagnostics are slightly different; it would be helpful if someone could check on that as a followup. Followup to llvm#128409. Fixes llvm#129844. Fixes llvm#129845.

…lizer` (llvm#128409) It is an alternative to llvm#127525. Close llvm#127475.

…consistently (llvm#129952) Perform the check for constexpr-unknown values in the same place we perform checks for other values which don't count as constant expressions. While I'm here, also fix a rejects-valid with a reference that doesn't have an initializer. This diagnostic was also covering up some of the bugs here. The existing behavior with -fexperimental-new-constant-interpreter seems to be correct, but the diagnostics are slightly different; it would be helpful if someone could check on that as a followup. Followup to llvm#128409. Fixes llvm#129844. Fixes llvm#129845.

dtcxzyw added 4 commits February 23, 2025 16:32

[Clang][CodeGen] Bail out on constexpr unknown values in ConstantEmitter

83248eb

Address review comments. NFC.

ac2bd21

[Clang][CodeGen] Address review comments. NFC.

1cea7d4

[Clang] Treat constexpr-unknown value as invalid in evaluateValue

bc24bde

dtcxzyw requested review from rjmccall, cor3ntin, AaronBallman, shafik, frederick-vs-ja, tbaederr and efriedma-quic February 23, 2025 09:07

dtcxzyw commented Feb 23, 2025

View reviewed changes

clang/test/SemaCXX/constant-expression-cxx11.cpp Outdated Show resolved Hide resolved

dtcxzyw mentioned this pull request Feb 23, 2025

[Clang][CodeGen] Bail out on constexpr unknown values in ConstantEmitter #127525

Closed

efriedma-quic reviewed Feb 24, 2025

View reviewed changes

[Clang][AST] Fix diag regression

52c8159

dtcxzyw commented Feb 26, 2025

View reviewed changes

dtcxzyw changed the title ~~[Clang] Treat constexpr-unknown value as invalid in evaluateValue~~ [Clang] Treat constexpr-unknown value as invalid in EvaluateAsInitializer Feb 26, 2025

dtcxzyw marked this pull request as ready for review February 26, 2025 16:12

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen IR generation bugs: mangling, exceptions, etc. labels Feb 26, 2025

efriedma-quic reviewed Feb 26, 2025

View reviewed changes

Remove comments. NFC.

1f4618c

efriedma-quic approved these changes Feb 27, 2025

View reviewed changes

shafik reviewed Feb 28, 2025

View reviewed changes

cor3ntin approved these changes Mar 4, 2025

View reviewed changes

shafik approved these changes Mar 4, 2025

View reviewed changes

dtcxzyw merged commit 27757fb into llvm:main Mar 5, 2025
11 checks passed

dtcxzyw deleted the p2280r4-reject-constexpr-unknown branch March 5, 2025 06:01

frederick-vs-ja added this to the LLVM 20.X Release milestone Mar 5, 2025

github-project-automation bot added this to LLVM Release Status Mar 5, 2025

github-project-automation bot moved this to Needs Triage in LLVM Release Status Mar 5, 2025

llvmbot moved this from Needs Triage to Done in LLVM Release Status Mar 5, 2025

efriedma-quic mentioned this pull request Mar 5, 2025

Miscompile with C++23 constexpr-unknown constant evaluation #129844

Closed

efriedma-quic mentioned this pull request Mar 5, 2025

[clang] Reject constexpr-unknown values as constant expressions more consistently #129952

Merged

shiltian mentioned this pull request Mar 10, 2025

[AMDGPU] Fix test failures when expensive checks are enabled #130644

Merged

efriedma-quic pushed a commit to efriedma-quic/llvm-project that referenced this pull request Mar 10, 2025

[Clang] Treat constexpr-unknown value as invalid in `EvaluateAsInitia…

fa5fbf2

…lizer` (llvm#128409) It is an alternative to llvm#127525. Close llvm#127475.

efriedma-quic mentioned this pull request Mar 10, 2025

release/20.x: [clang] Reject constexpr-unknown values as constant expressions more consistently #130658

Merged

swift-ci pushed a commit to swiftlang/llvm-project that referenced this pull request Mar 11, 2025

[Clang] Treat constexpr-unknown value as invalid in `EvaluateAsInitia…

9010db1

…lizer` (llvm#128409) It is an alternative to llvm#127525. Close llvm#127475.

shafik mentioned this pull request May 23, 2025

Unknown references and pointers related bad codegen #141114

Closed

	if (AllowConstexprUnknown) {
	Result = &Info.CurrentCall->createConstexprUnknownAPValues(VD, Base);
	return true;
	}

[Clang] Treat constexpr-unknown value as invalid in EvaluateAsInitializer #128409

[Clang] Treat constexpr-unknown value as invalid in EvaluateAsInitializer #128409

Uh oh!

Conversation

dtcxzyw commented Feb 23, 2025

Uh oh!

Uh oh!

Uh oh!

efriedma-quic Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dtcxzyw Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

efriedma-quic Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

dtcxzyw Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

llvmbot commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

efriedma-quic Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

dtcxzyw commented Feb 27, 2025

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

shafik left a comment

Choose a reason for hiding this comment

Uh oh!

efriedma-quic commented Mar 1, 2025

Uh oh!

dtcxzyw commented Mar 1, 2025

Uh oh!

Mishura4 commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shafik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

frederick-vs-ja commented Mar 5, 2025

Uh oh!

llvmbot commented Mar 5, 2025

Uh oh!

efriedma-quic commented Mar 5, 2025

Uh oh!

efriedma-quic commented Mar 5, 2025

Uh oh!

Uh oh!

[Clang] Treat constexpr-unknown value as invalid in `EvaluateAsInitializer` #128409

[Clang] Treat constexpr-unknown value as invalid in `EvaluateAsInitializer` #128409

llvmbot commented Feb 26, 2025 •

edited

Loading

Mishura4 commented Mar 4, 2025 •

edited

Loading