[Clang] Remove unnecessary Decl transform & profiles for SizeOfPackExpr #124533
Conversation
We used to always transform the pattern declaration for SizeOfPackExpr to ensure the constraint expression's profile produced the desired result. However, this approach failed to handle pack expansions when the pack referred to function parameters. In such cases, the function parameters were previously expanded to 1 to avoid building Subst* nodes (see e6974da). That workaround caused us to transform a pack without a proper ArgumentPackSubstitutionIndex, leading to crashes when transforming the pattern. It turns out that profiling the pattern for partially substituted SizeOfPackExprs is unnecessary because their transformed forms are already profiled within the partial arguments. Moreover, we always end up with a partially transformed SizeOfPackExpr for constraint comparison.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Younan Zhang (zyn0217) ChangesWe used to always transform the pattern declaration for SizeOfPackExpr to ensure the constraint expression's profile produced the desired result. However, this approach failed to handle pack expansions when the pack referred to function parameters. In such cases, the function parameters were formerly expanded to 1 to avoid building Subst* nodes (see e6974da). That workaround caused us to transform a pack without a proper ArgumentPackSubstitutionIndex, leading to crashes when transforming the pattern. It turns out that profiling the pattern for partially substituted SizeOfPackExprs is unnecessary because their transformed forms are also profiled within the partial arguments. Fixes #124161 Full diff: https://github.com/llvm/llvm-project/pull/124533.diff 4 Files Affected:
diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
index 4cec89c979f775..04529fa616d703 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -4326,8 +4326,6 @@ class SizeOfPackExpr final
/// Retrieve the parameter pack.
NamedDecl *getPack() const { return Pack; }
- void setPack(NamedDecl *NewPack) { Pack = NewPack; }
-
/// Retrieve the length of the parameter pack.
///
/// This routine may only be invoked when the expression is not
diff --git a/clang/lib/AST/StmtProfile.cpp b/clang/lib/AST/StmtProfile.cpp
index 0f1ebc68a4f762..089d91d2736459 100644
--- a/clang/lib/AST/StmtProfile.cpp
+++ b/clang/lib/AST/StmtProfile.cpp
@@ -2266,13 +2266,13 @@ void StmtProfiler::VisitPackExpansionExpr(const PackExpansionExpr *S) {
void StmtProfiler::VisitSizeOfPackExpr(const SizeOfPackExpr *S) {
VisitExpr(S);
- VisitDecl(S->getPack());
if (S->isPartiallySubstituted()) {
auto Args = S->getPartialArguments();
ID.AddInteger(Args.size());
for (const auto &TA : Args)
VisitTemplateArgument(TA);
} else {
+ VisitDecl(S->getPack());
ID.AddInteger(0);
}
}
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 839c4e8a28220b..98bc630fb69cc5 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -1747,23 +1747,6 @@ namespace {
return inherited::TransformLambdaBody(E, Body);
}
- ExprResult TransformSizeOfPackExpr(SizeOfPackExpr *E) {
- ExprResult Transformed = inherited::TransformSizeOfPackExpr(E);
- if (!Transformed.isUsable())
- return Transformed;
- auto *TransformedExpr = cast<SizeOfPackExpr>(Transformed.get());
- if (SemaRef.CodeSynthesisContexts.back().Kind ==
- Sema::CodeSynthesisContext::ConstraintNormalization &&
- TransformedExpr->getPack() == E->getPack()) {
- Decl *NewPack =
- TransformDecl(E->getPackLoc(), TransformedExpr->getPack());
- if (!NewPack)
- return ExprError();
- TransformedExpr->setPack(cast<NamedDecl>(NewPack));
- }
- return TransformedExpr;
- }
-
ExprResult TransformRequiresExpr(RequiresExpr *E) {
LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true);
ExprResult TransReq = inherited::TransformRequiresExpr(E);
diff --git a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
index 7cb5cfc10b9a7b..16779ba184296d 100644
--- a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -720,6 +720,34 @@ template <typename... Ts> struct d {
template struct c<int>;
template struct d<int, int>;
+namespace Regression_123441 {
+
+struct buf {
+ constexpr buf(auto&&... initList) requires (sizeof...(initList) <= 8);
+};
+
+constexpr buf::buf(auto&&... initList) requires (sizeof...(initList) <= 8) {}
+
+template <class>
+struct buffer {
+ constexpr buffer(auto&&... initList) requires (sizeof...(initList) <= 8);
+};
+
+template <class T>
+constexpr buffer<T>::buffer(auto&&... initList) requires (sizeof...(initList) <= 8) {}
+
+template <class...>
+struct foo { // expected-note {{foo defined here}}
+ constexpr foo(auto&&... initList)
+ requires (sizeof...(initList) <= 8);
+};
+
+template <class... T>
+constexpr foo<T...>::foo(auto&&... initList) // expected-error {{does not match any declaration}}
+ requires (sizeof...(T) <= 8) {}
+
+}
+
} // namespace GH115098
namespace GH114685 {
|
There was a problem hiding this comment.
In my comment here, it really looks like a clang-17 regression: #124161 (comment)
From what I can tell you also address the simplified version, so I believe a release note is probably warrented.
Are there really multiple bugs here?
Yeah, this does appear to be a regression and so it warrants a release note. Reminder: please be sure to check back on issues to make sure we're addressing all of the comments (sometimes new information comes up in comments that's not reflected in the issue summary). |
|
@shafik @AaronBallman Here is the complete backstory: We were not performing well in out-of-line concept comparison, especially for expressions involving packs. I tried to address these issues around commit 04d20b1, but it quickly became evident that the fix was insufficient for #101735. To resolve that, we harnessed a feature that TreeTransform would expand function parameters if the parameters were instantiated as a pack, rather than keeping them in non-expanded forms. That solution (developed during the 20 cycle and backported to 19) worked but introduced a pitfall: if we needed to transform a pack declaration within an expression, we would also need a valid ArgumentPackSubstitutionIndex to expand the pack. Shortly after, I addressed #93099 (also during the 20 development cycle), where a SizeOfPackExpr was improperly profiled for constraint comparison due to its declaration being non-transformed. To that end, I added a declaration transformation step when normalizing constraints. Combined with the previously mentioned pitfall, this again crashed #124161 since no ArgumentPackSubstitutionIndex was set. Technically, this should have worked at some point, as @shafik observed, which I believe was prior to the fix for #93099. And as far as I can tell, the latest crash was introduced by the fix for #93099, so I didn’t add a release note as usual. |
|
Thank you for the explanation, that makes more sense to me. |
When profiling a pack indexing that has been partially substituted, we should profile the expansions, rather than the pattern itseld This is a better approach to llvm#139057 This mirrors the fix done for SizeOfPackExpr in llvm#124533 Fixes llvm#138255
We used to always transform the pattern declaration for SizeOfPackExpr to ensure the constraint expression's profile produced the desired result. However, this approach failed to handle pack expansions when the pack referred to function parameters. In such cases, the function parameters were formerly expanded to 1 to avoid building Subst* nodes (see e6974da). That workaround caused us to transform a pack without a proper ArgumentPackSubstitutionIndex, leading to crashes when transforming the pattern.
It turns out that profiling the pattern for partially substituted SizeOfPackExprs is unnecessary because their transformed forms are also profiled within the partial arguments.
(No release note because this is actually a trunk regression)
Fixes #124161