Skip to content

[clang] Fix a crash issue that caused by handling of fields with initializers in nested anonymous unions #113049

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

Merged
merged 7 commits into from
Dec 10, 2024
Merged

[clang] Fix a crash issue that caused by handling of fields with initializers in nested anonymous unions #113049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
804b103
[clang] Fix a crash issue that caused by handling of fields with init…
yronglin Nov 20, 2024
de4a9d5
Revert "[clang] Fix a crash issue that caused by handling of fields w…
yronglin Nov 23, 2024
882fd4e
Recovery from the invalid in-class-initializer
yronglin Nov 24, 2024
cf5a038
Add test for ast-dump
yronglin Nov 25, 2024
ac30f8a
Try to recover when in-class-initializer had errors
yronglin Dec 4, 2024
c0be803
Move the error check into BuildCXXDefaultInit
yronglin Dec 5, 2024
5be9331
Build invalid default-member-init in BuildCXXDefaultInitExpr
yronglin Dec 6, 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
2 changes: 1 addition & 1 deletion clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5287,7 +5287,7 @@ class Sema final : public SemaBase {
/// is complete.
void ActOnFinishCXXInClassMemberInitializer(Decl *VarDecl,
SourceLocation EqualLoc,
Expr *Init);
ExprResult Init);

/// Handle a C++ member initializer using parentheses syntax.
MemInitResult
Expand Down
3 changes: 1 addition & 2 deletions clang/lib/Parse/ParseCXXInlineMethods.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,8 +722,7 @@ void Parser::ParseLexedMemberInitializer(LateParsedMemberInitializer &MI) {
ExprResult Init = ParseCXXMemberInitializer(MI.Field, /*IsFunction=*/false,
EqualLoc);

Actions.ActOnFinishCXXInClassMemberInitializer(MI.Field, EqualLoc,
Init.get());
Actions.ActOnFinishCXXInClassMemberInitializer(MI.Field, EqualLoc, Init);

// The next token should be our artificial terminating EOF token.
if (Tok.isNot(tok::eof)) {
Expand Down
24 changes: 14 additions & 10 deletions clang/lib/Sema/SemaDeclCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4080,24 +4080,28 @@ ExprResult Sema::ConvertMemberDefaultInitExpression(FieldDecl *FD,

void Sema::ActOnFinishCXXInClassMemberInitializer(Decl *D,
SourceLocation InitLoc,
Expr *InitExpr) {
ExprResult InitExpr) {
// Pop the notional constructor scope we created earlier.
PopFunctionScopeInfo(nullptr, D);

FieldDecl *FD = dyn_cast<FieldDecl>(D);
assert((isa<MSPropertyDecl>(D) || FD->getInClassInitStyle() != ICIS_NoInit) &&
"must set init style when field is created");

if (!InitExpr) {
// Microsoft C++'s property declaration cannot have a default member
// initializer.
if (isa<MSPropertyDecl>(D)) {
D->setInvalidDecl();
if (FD)
FD->removeInClassInitializer();
return;
}

if (DiagnoseUnexpandedParameterPack(InitExpr, UPPC_Initializer)) {
FieldDecl *FD = dyn_cast<FieldDecl>(D);
assert((FD && FD->getInClassInitStyle() != ICIS_NoInit) &&
"must set init style when field is created");

if (!InitExpr.isUsable() ||
DiagnoseUnexpandedParameterPack(InitExpr.get(), UPPC_Initializer)) {
FD->setInvalidDecl();
FD->removeInClassInitializer();
ExprResult RecoveryInit =
CreateRecoveryExpr(InitLoc, InitLoc, {}, FD->getType());
if (RecoveryInit.isUsable())
FD->setInClassInitializer(RecoveryInit.get());
return;
}

Expand Down
4 changes: 0 additions & 4 deletions clang/lib/Sema/SemaExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5563,10 +5563,6 @@ ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc,
ExprResult Sema::BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl *Field) {
assert(Field->hasInClassInitializer());

// If we might have already tried and failed to instantiate, don't try again.
if (Field->isInvalidDecl())
return ExprError();

CXXThisScopeRAII This(*this, Field->getParent(), Qualifiers());

auto *ParentRD = cast<CXXRecordDecl>(Field->getParent());
Expand Down
1 change: 1 addition & 0 deletions clang/lib/Sema/SemaInit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -750,6 +750,7 @@ void InitListChecker::FillInEmptyInitForField(unsigned Init, FieldDecl *Field,
if (Field->hasInClassInitializer()) {
if (VerifyOnly)
return;

ExprResult DIE;
{
// Enter a default initializer rebuild context, then we can support
Expand Down
34 changes: 34 additions & 0 deletions clang/test/AST/ast-dump-recovery.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,3 +460,37 @@ void RecoveryForStmtCond() {
// CHECK-NEXT: `-CompoundStmt {{.*}}
for (int i = 0; i < invalid; ++i) {}
}

// Fix crash issue https://github.com/llvm/llvm-project/issues/112560.
// Make sure clang compiles the following code without crashing:

// CHECK:NamespaceDecl {{.*}} GH112560
// CHECK-NEXT: |-CXXRecordDecl {{.*}} referenced union U definition
// CHECK-NEXT: | |-DefinitionData {{.*}}
// CHECK-NEXT: | | |-DefaultConstructor {{.*}}
// CHECK-NEXT: | | |-CopyConstructor {{.*}}
// CHECK-NEXT: | | |-MoveConstructor {{.*}}
// CHECK-NEXT: | | |-CopyAssignment {{.*}}
// CHECK-NEXT: | | |-MoveAssignment {{.*}}
// CHECK-NEXT: | | `-Destructor {{.*}}
// CHECK-NEXT: | |-CXXRecordDecl {{.*}} implicit union U
// CHECK-NEXT: | `-FieldDecl {{.*}} invalid f 'int'
// CHECK-NEXT: | `-RecoveryExpr {{.*}} 'int' contains-errors
// DISABLED-NOT: -RecoveryExpr {{.*}} contains-errors
namespace GH112560 {
union U {
int f = ;
};

// CHECK: FunctionDecl {{.*}} foo 'void ()'
// CHECK-NEXT: `-CompoundStmt {{.*}}
// CHECK-NEXT: `-DeclStmt {{.*}}
// CHECK-NEXT: `-VarDecl {{.*}} g 'U':'GH112560::U' listinit
// CHECK-NEXT: `-InitListExpr {{.*}} 'U':'GH112560::U' contains-errors field Field {{.*}} 'f' 'int'
// CHECK-NEXT: `-CXXDefaultInitExpr {{.*}} 'int' contains-errors has rewritten init
// CHECK-NEXT: `-RecoveryExpr {{.*}} 'int' contains-errors
// DISABLED-NOT: -RecoveryExpr {{.*}} contains-errors
void foo() {
U g{};
}
} // namespace GH112560
11 changes: 11 additions & 0 deletions clang/test/SemaCXX/cxx1y-initializer-aggregates.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,14 @@ namespace nested_union {
// of Test3, or we should exclude f(Test3) as a candidate.
static_assert(f({1}) == 2, ""); // expected-error {{call to 'f' is ambiguous}}
}

// Fix crash issue https://github.com/llvm/llvm-project/issues/112560.
// Make sure clang compiles the following code without crashing:
namespace GH112560 {
union U {
int f = ; // expected-error {{expected expression}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I can tell, the reason the code in SemaInit is getting confused is that the union is marked "hasInClassInitializer" because of the initializer... but then there isn't actually an initializer anywhere because we failed to parse it.

I think hasInClassInitializer() on RecordDecl needs to be consistent with hasInClassInitializer() on FieldDecl. If hasInClassInitializer is true, there should be an initializer somewhere. Either we should pretend the user didn't write the "=", or we should construct a RecoveryExpr to represent the missing initializer. Adding extra handling to every bit of code that checks hasInClassInitializer spreads out error handling to places it shouldn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! Make sense, there should have a RecoveryExpr as the initializer. I'd like to give a try.

};
void foo() {
U g{};
}
} // namespace GH112560
Loading