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

kadircet · 2025-04-24T11:52:13Z

Previously we would defer evaluation of CLEs until LValue to RValue
conversions, which would result in creating values within wrong scope
and triggering use-after-frees.

This patch instead eagerly evaluates CLEs, within the scope requiring
them. This requires storing an extra pointer for CLE expressions with
static storage.

Fixes #137165

llvmbot · 2025-04-24T11:52:49Z

@llvm/pr-subscribers-clang

Author: kadir çetinkaya (kadircet)

Changes

Previously we would defer evaluation of CLEs until LValue to RValue
conversions, which would result in creating values within wrong scope
and triggering use-after-frees.

This patch instead eagerly evaluates CLEs, within the scope requiring
them. This requires storing an extra pointer for CLE expressions with
static storage.

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

4 Files Affected:

(modified) clang/include/clang/AST/Expr.h (+12)
(modified) clang/lib/AST/Expr.cpp (+9)
(modified) clang/lib/AST/ExprConstant.cpp (+25-8)
(added) clang/test/AST/static-compound-literals.cpp (+12)

diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index a83320a7ddec2..95c0f910c22f8 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -3489,6 +3489,11 @@ 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. Used only for
+  /// constant folding as CompoundLiteralExpr is not an ICE.
+  mutable APValue *StaticValue = nullptr;
+
 public:
   CompoundLiteralExpr(SourceLocation lparenloc, TypeSourceInfo *tinfo,
                       QualType T, ExprValueKind VK, Expr *init, bool fileScope)
@@ -3518,6 +3523,13 @@ class CompoundLiteralExpr : public Expr {
     TInfoAndScope.setPointer(tinfo);
   }
 
+  bool hasStaticStorage() const { return isFileScope() && isGLValue(); }
+  APValue *getOrCreateStaticValue(ASTContext& Ctx) const;
+  APValue &getStaticValue() const {
+    assert(StaticValue);
+    return *StaticValue;
+  }
+
   SourceLocation getBeginLoc() const LLVM_READONLY {
     // FIXME: Init should never be null.
     if (!Init)
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 59c0e47c7c195..442e85b892a51 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -5467,3 +5467,12 @@ 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;
+}
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 7c933f47bf7f0..2379e78c1631a 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -4596,10 +4596,6 @@ handleLValueToRValueConversion(EvalInfo &Info, const Expr *Conv, QualType Type,
         return false;
       }
 
-      APValue Lit;
-      if (!Evaluate(Lit, Info, CLE->getInitializer()))
-        return false;
-
       // According to GCC info page:
       //
       // 6.28 Compound Literals
@@ -4622,7 +4618,12 @@ handleLValueToRValueConversion(EvalInfo &Info, const Expr *Conv, QualType Type,
         }
       }
 
-      CompleteObject LitObj(LVal.Base, &Lit, Base->getType());
+      APValue *Lit =
+          CLE->hasStaticStorage()
+              ? &CLE->getStaticValue()
+              : Info.CurrentCall->getTemporary(Base, LVal.Base.getVersion());
+
+      CompleteObject LitObj(LVal.Base, Lit, Base->getType());
       return extractSubobject(Info, Conv, LitObj, LVal.Designator, RVal, AK);
     } else if (isa<StringLiteral>(Base) || isa<PredefinedExpr>(Base)) {
       // Special-case character extraction so we don't have to construct an
@@ -9125,9 +9126,25 @@ 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();
+  } else {
+    Lit = &Info.CurrentCall->createTemporary(E, E->getInitializer()->getType(),
+                                             ScopeKind::FullExpression, Result);
+  }
+  if (!EvaluateInPlace(*Lit, Info, Result, E->getInitializer())) {
+    *Lit = APValue();
+    return false;
+  }
+  return true;
 }
 
 bool LValueExprEvaluator::VisitCXXTypeidExpr(const CXXTypeidExpr *E) {
diff --git a/clang/test/AST/static-compound-literals.cpp b/clang/test/AST/static-compound-literals.cpp
new file mode 100644
index 0000000000000..ceb8b985ab9dc
--- /dev/null
+++ b/clang/test/AST/static-compound-literals.cpp
@@ -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;
+}

github-actions · 2025-04-24T11:54:35Z

✅ With the latest revision this PR passed the C/C++ code formatter.

kadircet · 2025-04-24T11:55:25Z

Followup for #118480

AaronBallman · 2025-04-24T14:13:05Z

clang/include/clang/AST/Expr.h

@@ -3489,6 +3489,11 @@ 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. Used only for
+  /// constant folding as CompoundLiteralExpr is not an ICE.


A compound literal expression is an ICE in C though?

An integer constant expression116) shall have integer type and shall only have operands that are
integer constants, named and compound literal constants of integer type, character constants,
sizeof expressions whose results are integer constants, alignof expressions, and floating, named,
or compound literal constants of arithmetic type that are the immediate operands of casts. Cast
operators in an integer constant expression shall only convert arithmetic types to integer types,
except as part of an operand to the typeof operators, sizeof operator, or alignof operator.

Ah, but that's probably from an old comment that confused us (at least me):
LValueExprEvaluator used to say:

// only see this when folding in C, so there's no standard to follow here

I think we can just remove this comment.

i am happy to drop that comment, but I was inclined to preserve it as well since I came across logic aligned with that in couple of other places too, e.g.

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

Line 17278 in 5bad5d8

case Expr::CompoundLiteralExprClass:

. There's likely more dragons lurking in here.

ilya-biryukov · 2025-04-24T15:59:48Z

clang/include/clang/AST/Expr.h

@@ -3489,6 +3489,11 @@ 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. Used only for
+  /// constant folding as CompoundLiteralExpr is not an ICE.


Ah, but that's probably from an old comment that confused us (at least me):
LValueExprEvaluator used to say:

// only see this when folding in C, so there's no standard to follow here

I think we can just remove this comment.

clang/include/clang/AST/Expr.h

clang/lib/AST/ExprConstant.cpp

kadircet · 2025-05-06T21:27:16Z

ping @AaronBallman @efriedma-quic if you don't have more concerns here, I'd like to move forward with this patch

kadircet

thanks for the review @efriedma-quic! LMK if you have more concerns, otherwise i'd like to move forward with the change.

clang/lib/AST/ExprConstant.cpp

efriedma-quic

LGTM

clang/lib/AST/ExprConstant.cpp

Previously we would defer evaluation of CLEs until LValue to RValue conversions, which would result in creating values within wrong scope and triggering use-after-frees. This patch instead eagerly evaluates CLEs, within the scope requiring them. This requires storing an extra pointer for CLE expressions with static storage.

kadircet · 2025-07-07T08:07:41Z

@AaronBallman @ilya-biryukov ping here, I am planning to merge this soon. please LMK if you still have any concerns.

ilya-biryukov

LGTM

dstenb · 2025-07-08T16:45:51Z

Hi, @kadircet!

This commit broke one of our downstream tests where we mount the repository as read-only, as the static-compound-literals-crash.cpp test attempts to write the output to the test source directory:

(frontend): unable to open output file '/path/to/repo/clang/test/AST/static-compound-literals-crash.ll': 'Read-only file system'

It looks like the RUN line is missing -o or a redirection:

RUN: not --crash %clang_cc1 -verify -std=c++20 -emit-llvm %s

kadircet · 2025-07-09T07:25:28Z

oops, should be fixed with 16b8fb1

kadircet requested review from ilya-biryukov and AaronBallman April 24, 2025 11:52

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 24, 2025

kadircet force-pushed the static_compound_lits branch from c3efccd to 99fa9a1 Compare April 24, 2025 11:56

kadircet mentioned this pull request Apr 24, 2025

[clang] Fix a use-after-free in expression evaluation #118480

Closed

AaronBallman reviewed Apr 24, 2025

View reviewed changes

ilya-biryukov reviewed Apr 24, 2025

View reviewed changes

efriedma-quic reviewed Apr 24, 2025

View reviewed changes

clang/lib/AST/ExprConstant.cpp Show resolved Hide resolved

clang/lib/AST/ExprConstant.cpp Outdated Show resolved Hide resolved

kadircet requested review from ilya-biryukov, efriedma-quic and AaronBallman April 25, 2025 10:40

efriedma-quic reviewed Apr 25, 2025

View reviewed changes

clang/lib/AST/ExprConstant.cpp Outdated Show resolved Hide resolved

clang/lib/AST/ExprConstant.cpp Show resolved Hide resolved

clang/lib/AST/ExprConstant.cpp Show resolved Hide resolved

kadircet requested a review from efriedma-quic April 28, 2025 15:30

kadircet force-pushed the static_compound_lits branch from ee8e0f8 to 6ca1c58 Compare June 27, 2025 09:21

kadircet commented Jun 27, 2025

View reviewed changes

clang/lib/AST/ExprConstant.cpp Show resolved Hide resolved

efriedma-quic approved these changes Jul 2, 2025

View reviewed changes

clang/lib/AST/ExprConstant.cpp Show resolved Hide resolved

kadircet added 5 commits July 7, 2025 09:33

Address comments

bc2188c

Get rid of redundant volatile check

50865b1

add new test cases

f67d035

Add a fixme for using appropraite context

0caa8a5

kadircet force-pushed the static_compound_lits branch from 6ca1c58 to 0caa8a5 Compare July 7, 2025 07:41

ilya-biryukov approved these changes Jul 7, 2025

View reviewed changes

fix windows tests

81ad962

fix windows tests - take2

6572cb8

kadircet merged commit f72e53f into llvm:main Jul 8, 2025
9 checks passed

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

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

Uh oh!

Conversation

kadircet commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 24, 2025

Uh oh!

github-actions bot commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kadircet commented Apr 24, 2025

Uh oh!

AaronBallman Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

ilya-biryukov Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

kadircet Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

ilya-biryukov Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kadircet commented May 6, 2025

Uh oh!

kadircet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kadircet commented Jul 7, 2025

Uh oh!

ilya-biryukov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dstenb commented Jul 8, 2025

Uh oh!

kadircet commented Jul 9, 2025

Uh oh!

Uh oh!

kadircet commented Apr 24, 2025 •

edited

Loading

github-actions bot commented Apr 24, 2025 •

edited

Loading