Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 6 commits into from
Mar 5, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
83248eb
[Clang][CodeGen] Bail out on constexpr unknown values in ConstantEmitter
dtcxzyw Feb 17, 2025
ac2bd21
Address review comments. NFC.
dtcxzyw Feb 18, 2025
1cea7d4
[Clang][CodeGen] Address review comments. NFC.
dtcxzyw Feb 21, 2025
bc24bde
[Clang] Treat constexpr-unknown value as invalid in `evaluateValue`
dtcxzyw Feb 23, 2025
52c8159
[Clang][AST] Fix diag regression
dtcxzyw Feb 26, 2025
1f4618c
Remove comments. NFC.
dtcxzyw Feb 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions clang/lib/AST/ExprConstant.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}
return true;
}
Expand Down Expand Up @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

<< NewVD;
NoteLValueLocation(Info, Base);
return false;
}
}

return CheckConstantExpression(Info, DeclLoc, DeclTy, Value,
Expand Down
5 changes: 4 additions & 1 deletion clang/lib/CodeGen/CGExprConstant.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1883,8 +1883,11 @@ 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())
if (APValue *value = D.evaluateValue()) {
assert(!value->allowConstexprUnknown() &&
"Constexpr unknown values are not allowed in CodeGen");
return tryEmitPrivateForMemory(*value, destType);
}

return nullptr;
}
Expand Down
15 changes: 15 additions & 0 deletions clang/test/CodeGenCXX/cxx23-p2280r4.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
}