[Clang][Sema] Avoid pack expansion for expanded empty PackIndexingExprs #92385

zyn0217 · 2024-05-16T11:46:20Z

We previously doubled the id-expression expansion, even when the pack was expanded to empty. The previous condition for determining whether we should expand couldn't distinguish between cases where 'the expansion was previously postponed' and 'the expansion occurred but resulted in emptiness.'

In the latter scenario, we crash because we have not been examining the current lambda's parent local instantiation scope since D98068: Any Decls instantiated in the parent scope are not visible to the generic lambda, and thus any attempt of looking for instantiated Decls in the lambda is capped to the current Lambda's LIS.

Fixes #92230

We previously doubled the id-expression expansion, even when the pack was expanded to empty. The previous condition for determining whether we should expand couldn't distinguish between cases where 'the expansion was previously postponed' and 'the expansion occurred but resulted in emptiness.' In the latter scenario, we crash because we have not been examining the current lambda's parent local instantiation scope since D98068: Any Decls instantiated in the parent scope are not visible to the generic lambda. And thus any attempt of looking for instantiated Decls in the lambda is capped to the current Lambda's LIS. Fixes llvm#92230

llvmbot · 2024-05-16T11:46:42Z

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

We previously doubled the id-expression expansion, even when the pack was expanded to empty. The previous condition for determining whether we should expand couldn't distinguish between cases where 'the expansion was previously postponed' and 'the expansion occurred but resulted in emptiness.'

In the latter scenario, we crash because we have not been examining the current lambda's parent local instantiation scope since D98068: Any Decls instantiated in the parent scope are not visible to the generic lambda, and thus any attempt of looking for instantiated Decls in the lambda is capped to the current Lambda's LIS.

Fixes #92230

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

5 Files Affected:

(modified) clang/include/clang/AST/ExprCXX.h (+10-4)
(modified) clang/lib/AST/ExprCXX.cpp (+8-8)
(modified) clang/lib/Sema/SemaTemplateVariadic.cpp (+1-1)
(modified) clang/lib/Sema/TreeTransform.h (+2-4)
(modified) clang/test/SemaCXX/cxx2c-pack-indexing.cpp (+6-2)

diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
index fac65628ffede..2617cd36d0df9 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -4381,11 +4381,13 @@ class PackIndexingExpr final
 
   PackIndexingExpr(QualType Type, SourceLocation EllipsisLoc,
                    SourceLocation RSquareLoc, Expr *PackIdExpr, Expr *IndexExpr,
-                   ArrayRef<Expr *> SubstitutedExprs = {})
+                   ArrayRef<Expr *> SubstitutedExprs = {},
+                   bool EmptyPack = false)
       : Expr(PackIndexingExprClass, Type, VK_LValue, OK_Ordinary),
         EllipsisLoc(EllipsisLoc), RSquareLoc(RSquareLoc),
         SubExprs{PackIdExpr, IndexExpr},
-        TransformedExpressions(SubstitutedExprs.size()) {
+        TransformedExpressions(EmptyPack ? size_t(-1)
+                                         : SubstitutedExprs.size()) {
 
     auto *Exprs = getTrailingObjects<Expr *>();
     std::uninitialized_copy(SubstitutedExprs.begin(), SubstitutedExprs.end(),
@@ -4408,10 +4410,13 @@ class PackIndexingExpr final
                                   SourceLocation EllipsisLoc,
                                   SourceLocation RSquareLoc, Expr *PackIdExpr,
                                   Expr *IndexExpr, std::optional<int64_t> Index,
-                                  ArrayRef<Expr *> SubstitutedExprs = {});
+                                  ArrayRef<Expr *> SubstitutedExprs = {},
+                                  bool EmptyPack = false);
   static PackIndexingExpr *CreateDeserialized(ASTContext &Context,
                                               unsigned NumTransformedExprs);
 
+  bool isEmptyPack() const { return TransformedExpressions == size_t(-1); }
+
   /// Determine the location of the 'sizeof' keyword.
   SourceLocation getEllipsisLoc() const { return EllipsisLoc; }
 
@@ -4446,7 +4451,8 @@ class PackIndexingExpr final
   }
 
   ArrayRef<Expr *> getExpressions() const {
-    return {getTrailingObjects<Expr *>(), TransformedExpressions};
+    return {getTrailingObjects<Expr *>(),
+            isEmptyPack() ? 0 : TransformedExpressions};
   }
 
   static bool classof(const Stmt *T) {
diff --git a/clang/lib/AST/ExprCXX.cpp b/clang/lib/AST/ExprCXX.cpp
index 7e9343271ac3c..01cdd2709b472 100644
--- a/clang/lib/AST/ExprCXX.cpp
+++ b/clang/lib/AST/ExprCXX.cpp
@@ -1665,12 +1665,11 @@ NonTypeTemplateParmDecl *SubstNonTypeTemplateParmExpr::getParameter() const {
       getReplacedTemplateParameterList(getAssociatedDecl())->asArray()[Index]);
 }
 
-PackIndexingExpr *PackIndexingExpr::Create(ASTContext &Context,
-                                           SourceLocation EllipsisLoc,
-                                           SourceLocation RSquareLoc,
-                                           Expr *PackIdExpr, Expr *IndexExpr,
-                                           std::optional<int64_t> Index,
-                                           ArrayRef<Expr *> SubstitutedExprs) {
+PackIndexingExpr *
+PackIndexingExpr::Create(ASTContext &Context, SourceLocation EllipsisLoc,
+                         SourceLocation RSquareLoc, Expr *PackIdExpr,
+                         Expr *IndexExpr, std::optional<int64_t> Index,
+                         ArrayRef<Expr *> SubstitutedExprs, bool EmptyPack) {
   QualType Type;
   if (Index && !SubstitutedExprs.empty())
     Type = SubstitutedExprs[*Index]->getType();
@@ -1679,8 +1678,9 @@ PackIndexingExpr *PackIndexingExpr::Create(ASTContext &Context,
 
   void *Storage =
       Context.Allocate(totalSizeToAlloc<Expr *>(SubstitutedExprs.size()));
-  return new (Storage) PackIndexingExpr(
-      Type, EllipsisLoc, RSquareLoc, PackIdExpr, IndexExpr, SubstitutedExprs);
+  return new (Storage)
+      PackIndexingExpr(Type, EllipsisLoc, RSquareLoc, PackIdExpr, IndexExpr,
+                       SubstitutedExprs, EmptyPack);
 }
 
 NamedDecl *PackIndexingExpr::getPackDecl() const {
diff --git a/clang/lib/Sema/SemaTemplateVariadic.cpp b/clang/lib/Sema/SemaTemplateVariadic.cpp
index a4b681ae4f008..0b20604665068 100644
--- a/clang/lib/Sema/SemaTemplateVariadic.cpp
+++ b/clang/lib/Sema/SemaTemplateVariadic.cpp
@@ -1128,7 +1128,7 @@ Sema::BuildPackIndexingExpr(Expr *PackExpression, SourceLocation EllipsisLoc,
 
   return PackIndexingExpr::Create(getASTContext(), EllipsisLoc, RSquareLoc,
                                   PackExpression, IndexExpr, Index,
-                                  ExpandedExprs);
+                                  ExpandedExprs, EmptyPack);
 }
 
 TemplateArgumentLoc Sema::getTemplateArgumentPackExpansionPattern(
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index c039b95293af2..7a560d1cbb32a 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -14975,7 +14975,7 @@ TreeTransform<Derived>::TransformPackIndexingExpr(PackIndexingExpr *E) {
     return ExprError();
 
   SmallVector<Expr *, 5> ExpandedExprs;
-  if (E->getExpressions().empty()) {
+  if (!E->isEmptyPack() && E->getExpressions().empty()) {
     Expr *Pattern = E->getPackIdExpression();
     SmallVector<UnexpandedParameterPack, 2> Unexpanded;
     getSema().collectUnexpandedParameterPacks(E->getPackIdExpression(),
@@ -15029,9 +15029,7 @@ TreeTransform<Derived>::TransformPackIndexingExpr(PackIndexingExpr *E) {
         return true;
       ExpandedExprs.push_back(Out.get());
     }
-  }
-
-  else {
+  } else if (!E->isEmptyPack()) {
     if (getDerived().TransformExprs(E->getExpressions().data(),
                                     E->getExpressions().size(), false,
                                     ExpandedExprs))
diff --git a/clang/test/SemaCXX/cxx2c-pack-indexing.cpp b/clang/test/SemaCXX/cxx2c-pack-indexing.cpp
index 0ac85b5bcc14b..28b9765127f4e 100644
--- a/clang/test/SemaCXX/cxx2c-pack-indexing.cpp
+++ b/clang/test/SemaCXX/cxx2c-pack-indexing.cpp
@@ -206,13 +206,17 @@ void test(auto...args){
 template<int... args>
 void test2(){
   [&]<int idx>(){
-    using R = decltype( args...[idx] ) ;
-  }.template operator()<0>();
+    using R = decltype( args...[idx] ) ; // #test2-R
+  }.template operator()<0>(); // #test2-call
 }
 
 void f( ) {
   test(1);
   test2<1>();
+  test2();
+  // expected-error@#test2-R {{invalid index 0 for pack args of size 0}}
+  // expected-note@#test2-call {{requested here}}
+  // expected-note@-3 {{requested here}}
 }

clang/include/clang/AST/ExprCXX.h

cor3ntin · 2024-05-16T12:54:04Z

Thanks for working on this. The general direction looks good, but i had a comment

zyn0217

@cor3ntin PTAL. Thanks!

zyn0217 · 2024-05-17T08:02:13Z

clang/include/clang/AST/ExprCXX.h

@@ -4377,15 +4377,20 @@ class PackIndexingExpr final
  // The pack being indexed, followed by the index
  Stmt *SubExprs[2];

-  size_t TransformedExpressions;
+  // The size of the trailing expressions.
+  unsigned TransformedExpressions : 31;


I changed it to unsigned because numTrailingObjects is designed to return an unsigned number.

Aside: I'm not spelling it as : sizeof(unsigned) - 1 because we seem to write down the number 31 directly elsewhere. So I think i'd better respect the convention.

zyn0217 · 2024-05-17T08:04:12Z

clang/lib/Serialization/ASTWriterStmt.cpp

  Record.AddSourceLocation(E->getEllipsisLoc());
  Record.AddSourceLocation(E->getRSquareLoc());
  Record.AddStmt(E->getPackIdExpression());
  Record.AddStmt(E->getIndexExpr());
-  Record.push_back(E->TransformedExpressions);


This appears to be a copy-paste error of line 2160... So I removed it.

Weird that this did not cause any issues...

Weird that this did not cause any issues...

I think that's just because we didn't test it (nor does anyone... use it at scale). Anyhow, I contrived an example that would previously crash in clang/test/PCH/pack_indexing.cpp:

template <int I, auto...V> decltype(V...[I]) foo() { return V...[I]; } void fn1() { foo<1, 2, 3, 4>(); }

(And I didn't receive any notification from GH for this comment... sorry, I just saw it.)

clang/include/clang/AST/ExprCXX.h

cor3ntin

LGTM, Thanks

zyn0217 added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 16, 2024

zyn0217 requested a review from cor3ntin May 16, 2024 11:46

cor3ntin reviewed May 16, 2024

View reviewed changes

clang/include/clang/AST/ExprCXX.h Outdated Show resolved Hide resolved

Address feedback. Bit fields & some correction

afa1de4

llvmbot added the clang:modules C++20 modules and Clang Header Modules label May 17, 2024

zyn0217 commented May 17, 2024

View reviewed changes

cor3ntin reviewed May 20, 2024

View reviewed changes

clang/include/clang/AST/ExprCXX.h Outdated Show resolved Hide resolved

Say 'ExpandedToEmptyPack'

e95fe54

zyn0217 requested a review from cor3ntin May 20, 2024 14:42

cor3ntin approved these changes May 20, 2024

View reviewed changes

Add a case to test/PCH/pack_indexing.cpp

56a62cf

zyn0217 merged commit 8ce2045 into llvm:main May 21, 2024
4 checks passed

zyn0217 deleted the issue-92230 branch May 21, 2024 01:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang][Sema] Avoid pack expansion for expanded empty PackIndexingExprs #92385

[Clang][Sema] Avoid pack expansion for expanded empty PackIndexingExprs #92385

zyn0217 commented May 16, 2024

llvmbot commented May 16, 2024 •

edited

Loading

cor3ntin commented May 16, 2024

zyn0217 left a comment

zyn0217 May 17, 2024

zyn0217 May 17, 2024

zyn0217 May 17, 2024

cor3ntin May 17, 2024

zyn0217 May 20, 2024

zyn0217 May 20, 2024

cor3ntin left a comment

[Clang][Sema] Avoid pack expansion for expanded empty PackIndexingExprs #92385

[Clang][Sema] Avoid pack expansion for expanded empty PackIndexingExprs #92385

Conversation

zyn0217 commented May 16, 2024

llvmbot commented May 16, 2024 • edited Loading

cor3ntin commented May 16, 2024

zyn0217 left a comment

Choose a reason for hiding this comment

zyn0217 May 17, 2024

Choose a reason for hiding this comment

zyn0217 May 17, 2024

Choose a reason for hiding this comment

zyn0217 May 17, 2024

Choose a reason for hiding this comment

cor3ntin May 17, 2024

Choose a reason for hiding this comment

zyn0217 May 20, 2024

Choose a reason for hiding this comment

zyn0217 May 20, 2024

Choose a reason for hiding this comment

cor3ntin left a comment

Choose a reason for hiding this comment

llvmbot commented May 16, 2024 •

edited

Loading