Skip to content

[clang][CompundLiteralExpr] Don't defer evaluation for CLEs #137163

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[clang][CompundLiteralExpr] Don't defer evaluation for CLEs #137163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
99fa9a1
[clang][CompundLiteralExpr] Don't defer evaluation for CLEs
kadircet Apr 24, 2025
e341332
Address comments
kadircet Apr 25, 2025
ee8e0f8
Get rid of redundant volatile check
kadircet Apr 28, 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
8 changes: 8 additions & 0 deletions clang/include/clang/AST/Expr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3489,6 +3489,10 @@ class CompoundLiteralExpr : public Expr {
/// The int part of the pair stores whether this expr is file scope.
llvm::PointerIntPair<TypeSourceInfo *, 1, bool> TInfoAndScope;
Stmt *Init;

/// Value of constant literals with static storage duration.
mutable APValue *StaticValue = nullptr;

public:
CompoundLiteralExpr(SourceLocation lparenloc, TypeSourceInfo *tinfo,
QualType T, ExprValueKind VK, Expr *init, bool fileScope)
Expand Down Expand Up @@ -3518,6 +3522,10 @@ class CompoundLiteralExpr : public Expr {
TInfoAndScope.setPointer(tinfo);
}

bool hasStaticStorage() const { return isFileScope() && isGLValue(); }
APValue &getOrCreateStaticValue(ASTContext &Ctx) const;
APValue &getStaticValue() const;

SourceLocation getBeginLoc() const LLVM_READONLY {
// FIXME: Init should never be null.
if (!Init)
Expand Down
14 changes: 14 additions & 0 deletions clang/lib/AST/Expr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5467,3 +5467,17 @@ ConvertVectorExpr *ConvertVectorExpr::Create(
return new (Mem) ConvertVectorExpr(SrcExpr, TI, DstType, VK, OK, BuiltinLoc,
RParenLoc, FPFeatures);
}

APValue &CompoundLiteralExpr::getOrCreateStaticValue(ASTContext &Ctx) const {
assert(hasStaticStorage());
if (!StaticValue) {
StaticValue = new (Ctx) APValue;
Ctx.addDestruction(StaticValue);
}
return *StaticValue;
}

APValue &CompoundLiteralExpr::getStaticValue() const {
assert(StaticValue);
return *StaticValue;
}
87 changes: 46 additions & 41 deletions clang/lib/AST/ExprConstant.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
#include "clang/AST/OptionalDiagnostic.h"
#include "clang/AST/RecordLayout.h"
#include "clang/AST/StmtVisitor.h"
#include "clang/AST/Type.h"
#include "clang/AST/TypeLoc.h"
#include "clang/Basic/Builtins.h"
#include "clang/Basic/DiagnosticSema.h"
Expand Down Expand Up @@ -4522,6 +4523,30 @@ static CompleteObject findCompleteObject(EvalInfo &Info, const Expr *E,

BaseVal = MTE->getOrCreateValue(false);
assert(BaseVal && "got reference to unevaluated temporary");
} else if (const CompoundLiteralExpr *CLE =
dyn_cast_or_null<CompoundLiteralExpr>(Base)) {
// According to GCC info page:
//
// 6.28 Compound Literals
//
// As an optimization, G++ sometimes gives array compound literals
// longer lifetimes: when the array either appears outside a function or
// has a const-qualified type. If foo and its initializer had elements
// of type char *const rather than char *, or if foo were a global
// variable, the array would have static storage duration. But it is
// probably safest just to avoid the use of array compound literals in
// C++ code.
//
// Obey that rule by checking constness for converted array types.
if (QualType CLETy = CLE->getType(); CLETy->isArrayType() &&
!LValType->isArrayType() &&
!CLETy.isConstant(Info.Ctx)) {
Info.FFDiag(E);
Info.Note(CLE->getExprLoc(), diag::note_declared_at);
return CompleteObject();
}

BaseVal = &CLE->getStaticValue();
} else {
if (!IsAccess)
return CompleteObject(LVal.getLValueBase(), nullptr, BaseType);
Expand Down Expand Up @@ -4587,44 +4612,7 @@ handleLValueToRValueConversion(EvalInfo &Info, const Expr *Conv, QualType Type,
WantObjectRepresentation ? AK_ReadObjectRepresentation : AK_Read;

if (Base && !LVal.getLValueCallIndex() && !Type.isVolatileQualified()) {
if (const CompoundLiteralExpr *CLE = dyn_cast<CompoundLiteralExpr>(Base)) {
// In C99, a CompoundLiteralExpr is an lvalue, and we defer evaluating the
// initializer until now for such expressions. Such an expression can't be
// an ICE in C, so this only matters for fold.
if (Type.isVolatileQualified()) {
Info.FFDiag(Conv);
return false;
}

APValue Lit;
if (!Evaluate(Lit, Info, CLE->getInitializer()))
return false;

// According to GCC info page:
//
// 6.28 Compound Literals
//
// As an optimization, G++ sometimes gives array compound literals longer
// lifetimes: when the array either appears outside a function or has a
// const-qualified type. If foo and its initializer had elements of type
// char *const rather than char *, or if foo were a global variable, the
// array would have static storage duration. But it is probably safest
// just to avoid the use of array compound literals in C++ code.
//
// Obey that rule by checking constness for converted array types.

QualType CLETy = CLE->getType();
if (CLETy->isArrayType() && !Type->isArrayType()) {
if (!CLETy.isConstant(Info.Ctx)) {
Info.FFDiag(Conv);
Info.Note(CLE->getExprLoc(), diag::note_declared_at);
return false;
}
}

CompleteObject LitObj(LVal.Base, &Lit, Base->getType());
return extractSubobject(Info, Conv, LitObj, LVal.Designator, RVal, AK);
} else if (isa<StringLiteral>(Base) || isa<PredefinedExpr>(Base)) {
if (isa<StringLiteral>(Base) || isa<PredefinedExpr>(Base)) {
// Special-case character extraction so we don't have to construct an
// APValue for the whole string.
assert(LVal.Designator.Entries.size() <= 1 &&
Expand Down Expand Up @@ -9125,9 +9113,26 @@ bool
LValueExprEvaluator::VisitCompoundLiteralExpr(const CompoundLiteralExpr *E) {
assert((!Info.getLangOpts().CPlusPlus || E->isFileScope()) &&
"lvalue compound literal in c++?");
// Defer visiting the literal until the lvalue-to-rvalue conversion. We can
// only see this when folding in C, so there's no standard to follow here.
return Success(E);
APValue *Lit;
// If CompountLiteral has static storage, its value can be used outside
// this expression. So evaluate it once and store it in ASTContext.
if (E->hasStaticStorage()) {
Lit = &E->getOrCreateStaticValue(Info.Ctx);
Result.set(E);
// Reset any previously evaluated state, otherwise evaluation below might
// fail.
// FIXME: Should we just re-use the previously evaluated value instead?
*Lit = APValue();
Copy link
Collaborator

Choose a reason for hiding this comment

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

The evaluation should produce the same result every time, so it doesn't really do any harm to do it twice. But if you do it right, it's also very easy to avoid evaluating it twice.

I suspect using EvaluateInPlace will cause state to leak into the evaluation when it shouldn't (like, we accidentally accept a use of a temporary inside the initializer).

Copy link
Member Author

Choose a reason for hiding this comment

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

Since I don't fully understand the implications here, I'd rather keep the evaluation behavior similar to the previous version (e.g. evaluate every-time we need it).

Happy to evaluate once if you think that should be safe ~always and feel strongly about this.

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 more concerned about the fact that you're using EvaluateInPlace, as opposed to using EvaluateAsInitializer. Which... I'm not sure it's actually possible to observe a difference at the moment due to the rule that static compound literals are required to have a constant initializer, but it's fragile.

On a sort of related note, I was experimenting with some testcases, and the following crashed:

struct A {int x[1]; };
A f();
typedef int *t[];
consteval int* f(int* x) { return x; }
int ** x = (t){f(f().x)};

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually think of the opposite. The code here mimics what MaterializeTemporaryExpr does and I think it's a good thing because MaterializeTemporaryExpr should be the most battle-tested code for similar cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also it seems to EvaluateAsInitializer requires a VarDecl, I am not sure if that makes sense in LValueExprEvaluator.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking about it as a different thing from MaterializeTemporaryExpr... but I guess it's similar enough to a lifetime-extended temporary that we can just adopt the same semantic rules. Which might not be what a user would expect, but I guess it's at least self-consistent.

Do we need the Info.EvalMode == EvalInfo::EM_ConstantFold check from MaterializeTemporaryExpr?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need the Info.EvalMode == EvalInfo::EM_ConstantFold check from MaterializeTemporaryExpr?

Considering the previous comments in the code, I think we need to perform folding also for CLEs with static duration. There are also many test failures if we don't fold these in static initializers, https://github.com/llvm/llvm-project/blob/main/clang/test/AST/ByteCode/c.c#L155-L174 is an example (but I am also surprised to see that hold for MaterializeTemporaryExprs).

Copy link
Member Author

Choose a reason for hiding this comment

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

ping here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Dug into the EvalMode thing... I remember why we added it now. See https://reviews.llvm.org/D151587 for context. Roughly, the issue is that, if you constant-evaluate the expression in isolation, you get a different result from what you'd get evaluating the whole expression. So we can't redo the evaluation in ConstantFold mode: if you do, you'll corrupt the precomputed value, for cases like clang/test/CodeGenCXX/const-init-cxx1y.cpp.

I think compound literals run into similar issues if you allow ConstantFold mode to overwrite the evaluated value.

Maybe we can add some workaround specifically for cases where we immediately do an lvalue-to-rvalue conversion.

} else {
assert(!Info.getLangOpts().CPlusPlus);
Lit = &Info.CurrentCall->createTemporary(E, E->getInitializer()->getType(),
ScopeKind::Block, Result);
}
if (!EvaluateInPlace(*Lit, Info, Result, E->getInitializer())) {
*Lit = APValue();
return false;
}
return true;
}

bool LValueExprEvaluator::VisitCXXTypeidExpr(const CXXTypeidExpr *E) {
Expand Down
12 changes: 12 additions & 0 deletions clang/test/AST/static-compound-literals.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Test that we can successfully compile this code, especially under ASAN.
// RUN: %clang_cc1 -verify -std=c++20 -fsyntax-only %s
// expected-no-diagnostics
struct Foo {
Foo* f;
operator bool() const { return true; }
};
constexpr Foo f((Foo[]){});
int foo() {
if (Foo(*f.f)) return 1;
return 0;
}