Skip to content
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

Jump to bottom

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

Merged
merged 4 commits into from
May 21, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7acbb1d
[Clang][Sema] Avoid pack expansion for expanded empty PackIndexingExprs
zyn0217 May 16, 2024
afa1de4
Address feedback. Bit fields & some correction
zyn0217 May 17, 2024
e95fe54
Say 'ExpandedToEmptyPack'
zyn0217 May 20, 2024
56a62cf
Add a case to test/PCH/pack_indexing.cpp
zyn0217 May 20, 2024
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
18 changes: 14 additions & 4 deletions clang/include/clang/AST/ExprCXX.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


LLVM_PREFERRED_TYPE(bool)
unsigned EmptyPack : 1;
zyn0217 marked this conversation as resolved.
Show resolved Hide resolved

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(SubstitutedExprs.size()), EmptyPack(EmptyPack) {

auto *Exprs = getTrailingObjects<Expr *>();
std::uninitialized_copy(SubstitutedExprs.begin(), SubstitutedExprs.end(),
Expand All @@ -4408,10 +4413,14 @@ 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);

/// Determine if the expression was expanded to empty.
bool isEmptyPack() const { return EmptyPack; }

/// Determine the location of the 'sizeof' keyword.
SourceLocation getEllipsisLoc() const { return EllipsisLoc; }

Expand Down Expand Up @@ -4445,6 +4454,7 @@ class PackIndexingExpr final
return getTrailingObjects<Expr *>()[*Index];
}

/// Return the trailing expressions, regardless of the expansion.
ArrayRef<Expr *> getExpressions() const {
return {getTrailingObjects<Expr *>(), TransformedExpressions};
}
Expand Down
16 changes: 8 additions & 8 deletions clang/lib/AST/ExprCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Sema/SemaTemplateVariadic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
6 changes: 2 additions & 4 deletions clang/lib/Sema/TreeTransform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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))
Expand Down
1 change: 1 addition & 0 deletions clang/lib/Serialization/ASTReaderStmt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2177,6 +2177,7 @@ void ASTStmtReader::VisitSizeOfPackExpr(SizeOfPackExpr *E) {
void ASTStmtReader::VisitPackIndexingExpr(PackIndexingExpr *E) {
VisitExpr(E);
E->TransformedExpressions = Record.readInt();
E->EmptyPack = Record.readInt();
E->EllipsisLoc = readSourceLocation();
E->RSquareLoc = readSourceLocation();
E->SubExprs[0] = Record.readStmt();
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Serialization/ASTWriterStmt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2157,11 +2157,11 @@ void ASTStmtWriter::VisitSizeOfPackExpr(SizeOfPackExpr *E) {
void ASTStmtWriter::VisitPackIndexingExpr(PackIndexingExpr *E) {
VisitExpr(E);
Record.push_back(E->TransformedExpressions);
Record.push_back(E->EmptyPack);
Record.AddSourceLocation(E->getEllipsisLoc());
Record.AddSourceLocation(E->getRSquareLoc());
Record.AddStmt(E->getPackIdExpression());
Record.AddStmt(E->getIndexExpr());
Record.push_back(E->TransformedExpressions);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Weird that this did not cause any issues...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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>();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

for (Expr *Sub : E->getExpressions())
Record.AddStmt(Sub);
Code = serialization::EXPR_PACK_INDEXING;
Expand Down
8 changes: 6 additions & 2 deletions clang/test/SemaCXX/cxx2c-pack-indexing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}}
}


Expand Down
Loading