[Clang] Preserve CXXParenListInitExpr in TreeTransform. #138518
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We were converting a CXXParenListInitExpr to a ParenListExpr in Treetransform. However, ParenListExpr is typeless so clang could not rebuild the correct initialization sequence in some contexts. Fixes 72880
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: cor3ntin (cor3ntin) ChangesWe were converting a CXXParenListInitExpr to a ParenListExpr in TreeTransform. However, ParenListExpr is typeless, so Clang could not rebuild the correct initialization sequence in some contexts. #Fixes 72880 Full diff: https://github.com/llvm/llvm-project/pull/138518.diff 6 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 4bd9d904e1ea9..55a46fb6f947e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -642,6 +642,7 @@ Bug Fixes to C++ Support
(#GH136432), (#GH137014), (#GH138018)
- Fixed an assertion when trying to constant-fold various builtins when the argument
referred to a reference to an incomplete type. (#GH129397)
+- Fixed a crash when a cast involved a parenthesized aggregate initialization in dependent context. (#GH72880)
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
index 844f6dd90ae1d..04d08b022c562 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -5124,8 +5124,8 @@ class CXXParenListInitExpr final
void updateDependence() { setDependence(computeDependence(this)); }
- ArrayRef<Expr *> getInitExprs() {
- return ArrayRef(getTrailingObjects<Expr *>(), NumExprs);
+ MutableArrayRef<Expr *> getInitExprs() {
+ return MutableArrayRef(getTrailingObjects<Expr *>(), NumExprs);
}
const ArrayRef<Expr *> getInitExprs() const {
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 19343eb0af092..741951cb9ea0e 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -7167,6 +7167,11 @@ class Sema final : public SemaBase {
ExprResult ActOnParenExpr(SourceLocation L, SourceLocation R, Expr *E);
ExprResult ActOnParenListExpr(SourceLocation L, SourceLocation R,
MultiExprArg Val);
+ ExprResult ActOnCXXParenListInitExpr(ArrayRef<Expr *> Args, QualType T,
+ unsigned NumUserSpecifiedExprs,
+ SourceLocation InitLoc,
+ SourceLocation LParenLoc,
+ SourceLocation RParenLoc);
/// ActOnStringLiteral - The specified tokens were lexed as pasted string
/// fragments (e.g. "foo" "bar" L"baz"). The result string has to handle
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index d72d97addfac2..787b07c4080ea 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -7985,6 +7985,15 @@ ExprResult Sema::ActOnParenListExpr(SourceLocation L,
return ParenListExpr::Create(Context, L, Val, R);
}
+ExprResult Sema::ActOnCXXParenListInitExpr(ArrayRef<Expr *> Args, QualType T,
+ unsigned NumUserSpecifiedExprs,
+ SourceLocation InitLoc,
+ SourceLocation LParenLoc,
+ SourceLocation RParenLoc) {
+ return CXXParenListInitExpr::Create(Context, Args, T, NumUserSpecifiedExprs,
+ InitLoc, LParenLoc, RParenLoc);
+}
+
bool Sema::DiagnoseConditionalForNull(const Expr *LHSExpr, const Expr *RHSExpr,
SourceLocation QuestionLoc) {
const Expr *NullExpr = LHSExpr;
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index aed00e0ff06cd..a3120f61f0d9c 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -3099,6 +3099,15 @@ class TreeTransform {
return getSema().ActOnParenListExpr(LParenLoc, RParenLoc, SubExprs);
}
+ ExprResult RebuildCXXParenListInitExpr(ArrayRef<Expr *> Args, QualType T,
+ unsigned NumUserSpecifiedExprs,
+ SourceLocation InitLoc,
+ SourceLocation LParenLoc,
+ SourceLocation RParenLoc) {
+ return getSema().ActOnCXXParenListInitExpr(Args, T, NumUserSpecifiedExprs,
+ InitLoc, LParenLoc, RParenLoc);
+ }
+
/// Build a new address-of-label expression.
///
/// By default, performs semantic analysis, using the name of the label
@@ -3315,6 +3324,11 @@ class TreeTransform {
return getSema().BuildCXXTypeConstructExpr(
TInfo, LParenLoc, MultiExprArg(PLE->getExprs(), PLE->getNumExprs()),
RParenLoc, ListInitialization);
+
+ if (auto *PLE = dyn_cast<CXXParenListInitExpr>(Sub))
+ return getSema().BuildCXXTypeConstructExpr(
+ TInfo, LParenLoc, PLE->getInitExprs(), RParenLoc, ListInitialization);
+
return getSema().BuildCXXTypeConstructExpr(TInfo, LParenLoc,
MultiExprArg(&Sub, 1), RParenLoc,
ListInitialization);
@@ -16487,12 +16501,22 @@ ExprResult
TreeTransform<Derived>::TransformCXXParenListInitExpr(CXXParenListInitExpr *E) {
SmallVector<Expr *, 4> TransformedInits;
ArrayRef<Expr *> InitExprs = E->getInitExprs();
- if (TransformExprs(InitExprs.data(), InitExprs.size(), true,
- TransformedInits))
+
+ QualType T = getDerived().TransformType(E->getType());
+
+ bool ArgChanged = false;
+ ;
+
+ if (getDerived().TransformExprs(InitExprs.data(), InitExprs.size(), true,
+ TransformedInits, &ArgChanged))
return ExprError();
- return getDerived().RebuildParenListExpr(E->getBeginLoc(), TransformedInits,
- E->getEndLoc());
+ if (!ArgChanged && T == E->getType())
+ return E;
+
+ return getDerived().RebuildCXXParenListInitExpr(
+ TransformedInits, T, E->getUserSpecifiedInitExprs().size(),
+ E->getInitLoc(), E->getBeginLoc(), E->getEndLoc());
}
template<typename Derived>
diff --git a/clang/test/SemaCXX/paren-list-agg-init.cpp b/clang/test/SemaCXX/paren-list-agg-init.cpp
index b2d0f2564bb59..ba7dffdc1af9f 100644
--- a/clang/test/SemaCXX/paren-list-agg-init.cpp
+++ b/clang/test/SemaCXX/paren-list-agg-init.cpp
@@ -83,7 +83,7 @@ template <typename T, char CH>
void bar() {
T t = 0;
A a(CH, 1.1); // OK; C++ paren list constructors are supported in semantic tree transformations.
- // beforecxx20-warning@-1 2{{aggregate initialization of type 'A' from a parenthesized list of values is a C++20 extension}}
+ // beforecxx20-warning@-1 {{aggregate initialization of type 'A' from a parenthesized list of values is a C++20 extension}}
}
template <class T, class... Args>
@@ -157,9 +157,6 @@ void foo(int n) { // expected-note {{declared here}}
constexpr F f2(1, 1); // OK: f2.b is initialized by a constant expression.
// beforecxx20-warning@-1 {{aggregate initialization of type 'const F' from a parenthesized list of values is a C++20 extension}}
- bar<int, 'a'>();
- // beforecxx20-note@-1 {{in instantiation of function template specialization 'bar<int, 'a'>' requested here}}
-
G<char> g('b', 'b');
// beforecxx20-warning@-1 {{aggregate initialization of type 'G<char>' from a parenthesized list of values is a C++20 extension}}
@@ -354,7 +351,7 @@ using Td = int[]; Td d(42,43);
// beforecxx20-warning@-1 {{aggregate initialization of type 'int[2]' from a parenthesized list of values is a C++20 extension}}
template<typename T, int Sz> using ThroughAlias = T[Sz];
ThroughAlias<int, 1> e(42);
-// beforecxx20-warning@-1 {{aggregate initialization of type 'ThroughAlias<int, 1>' (aka 'int[1]') from a parenthesized list of values is a C++20 extension}}
+// beforecxx20-warning@-1 {{aggregate initialization of type 'ThroughAlias<int, 1>' (aka 'int[1]') from a parenthesized list of values is a C++20 extension}}
}
@@ -376,3 +373,33 @@ static_assert(S(1, 2) == S(3, 4));
// beforecxx20-warning@-1 2{{C++20 extension}}
}
+
+namespace GH72880 {
+struct Base {};
+struct Derived : Base {
+ int count = 42;
+};
+
+template <typename T>
+struct BaseTpl {};
+template <typename T>
+struct DerivedTpl : BaseTpl<T> {
+ int count = 43;
+};
+template <typename T> struct S {
+ void f() {
+ Derived a = static_cast<Derived>(Base());
+ // beforecxx20-warning@-1 {{C++20 extension}}
+ DerivedTpl b = static_cast<DerivedTpl<T>>(BaseTpl<T>());
+ // beforecxx20-warning@-1 {{C++20 extension}}
+ static_assert(static_cast<Derived>(Base()).count == 42);
+ // beforecxx20-warning@-1 {{C++20 extension}}
+ static_assert(static_cast<DerivedTpl<T>>(BaseTpl<T>()).count == 43);
+ // beforecxx20-warning@-1 {{C++20 extension}}
+ }
+};
+
+void test() {
+ S<int>{}.f(); // beforecxx20-note {{requested here}}
+}
+}
|
erichkeane
approved these changes
May 5, 2025
clang/lib/Sema/TreeTransform.h
Outdated
| QualType T = getDerived().TransformType(E->getType()); | ||
|
|
||
| bool ArgChanged = false; | ||
| ; |
There was a problem hiding this comment.
extra semicolon for no reason?
zyn0217
approved these changes
May 5, 2025
clang/lib/Sema/TreeTransform.h
Outdated
Comment on lines
16514
to
16515
| if (!ArgChanged && T == E->getType()) | ||
| return E; |
There was a problem hiding this comment.
Do we want to handle AlwaysRebuild?
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
We were converting a CXXParenListInitExpr to a ParenListExpr in TreeTransform. However, ParenListExpr is typeless, so Clang could not rebuild the correct initialization sequence in some contexts. Fixes llvm#72880
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
We were converting a CXXParenListInitExpr to a ParenListExpr in TreeTransform. However, ParenListExpr is typeless, so Clang could not rebuild the correct initialization sequence in some contexts. Fixes llvm#72880
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
We were converting a CXXParenListInitExpr to a ParenListExpr in TreeTransform. However, ParenListExpr is typeless, so Clang could not rebuild the correct initialization sequence in some contexts. Fixes llvm#72880
GeorgeARM
pushed a commit
to GeorgeARM/llvm-project
that referenced
this pull request
May 7, 2025
We were converting a CXXParenListInitExpr to a ParenListExpr in TreeTransform. However, ParenListExpr is typeless, so Clang could not rebuild the correct initialization sequence in some contexts. Fixes llvm#72880
|
@cor3ntin this commit causes an assertion failure: https://gcc.godbolt.org/z/7hW4ssrfq Actually, I can reproduce the assertion failure in Clang before this commit using a different example, but I haven't yet reduced it. It might be a distinct problem though despite the same assertion failure. |
|
@cor3ntin yep, the second test case I have doesn't seem to be directly related. It causes crashes to Clang since version 7: https://gcc.godbolt.org/z/6M13c4q3T |
|
I've filed #141242 for the second report. However #138518 (comment) is new and still needs to be addressed. |
|
Apart from the assertion failure in CodeGen, we have two distinct test cases that started failing after this commit. The code in both cases is sanitizer-clean and looks valid. The failures reproduce with -O0, so it's not about optimizations. Looks like Clang codegen is broken for these cases. Reducing the test cases is not as easy as with Clang assertion failures, but I'll try to produce a test case early next week. So far I just managed to compare x86 assembly produced by clang -O0 without this commit and with it. The diff boils down to a few instances of In IR the only diffs are (--- before +++ after this commit): |
|
@cor3ntin ^^ |
|
Reduced test case (hopefully, no important bits were lost during automated reduction): |
cor3ntin
added a commit
to cor3ntin/llvm-project
that referenced
this pull request
May 24, 2025
We did not handle the case where a variable could be initialized by a CXXParenListInitExpr.
cor3ntin
added a commit
to cor3ntin/llvm-project
that referenced
this pull request
May 24, 2025
We did not handle the case where a variable could be initialized by a CXXParenListInitExpr.
cor3ntin
added a commit
that referenced
this pull request
May 28, 2025
We did not handle the case where a variable could be initialized by a CXXParenListInitExpr. --------- Co-authored-by: Shafik Yaghmour <[email protected]>
|
@cor3ntin thank you for the fix for the assertion failure! The problem described in #138518 (comment) still remains though. |
|
@cor3ntin friendly ping. Please take a look at the remaining issue here. Thanks! |
|
@alexfh I don't have the resources to look into this issue. Please revert if you need to. |
This has been in trunk for a long time, a revert here would be HIGHLY unfortunate. I would vastly prefer someone spend time trying to figure out why we are confused with the variable init here instead. So:
Seems to be the line that isn't producing the same value. We're initializing it with an int instead of a 64-bit- IN a non-template version (i removed the template head itself), we get: In the primary template we have: SO something about instantiating the expressions lost their cast? I'm not sure how that happens? |
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
We did not handle the case where a variable could be initialized by a CXXParenListInitExpr. --------- Co-authored-by: Shafik Yaghmour <[email protected]>
It would be really unfortunate indeed, if we have to revert this. But that's a clearly incorrect codegen, so the only alternative is to get a fix. I looked a bit into this and came up with a slightly more compact test case: https://gcc.godbolt.org/z/Pcj8954Ye Similarly to #138518 (comment), AST dump already shows the problem (https://godbolt.org/z/PYeG1v5Yz) - I'm trying to figure out how this commit affects the relevant template instantiation logic, but no ideas so far. |
|
Before this change the cast used to be added here: Now this doesn't happen: (probably because the argument of a In both cases this is happening here: |
|
Okay, I think, I have a fix. |
alexfh
added a commit
to alexfh/llvm-project
that referenced
this pull request
Jun 5, 2025
alexfh
added a commit
that referenced
this pull request
Jun 5, 2025
CXXParenListInitExpr arguments would lose casts leading to incorrect types being used (e.g. only 32 bits of a 64 bit value being initialized). See #138518 (comment) and #138518 (comment) for details and context.
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jun 5, 2025
CXXParenListInitExpr arguments would lose casts leading to incorrect types being used (e.g. only 32 bits of a 64 bit value being initialized). See llvm/llvm-project#138518 (comment) and llvm/llvm-project#138518 (comment) for details and context.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We were converting a CXXParenListInitExpr to a ParenListExpr in TreeTransform.
However, ParenListExpr is typeless, so Clang could not rebuild the correct initialization sequence in some contexts.
Fixes #72880