-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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.
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
[Clang] prevent setting default lexical access specifier for missing primary declarations #112424
base: main
Are you sure you want to change the base?
Conversation
…primary declarations
@llvm/pr-subscribers-clang Author: Oleksandr T. (a-tarasyuk) ChangesFixes #112208 Full diff: https://github.com/llvm/llvm-project/pull/112424.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 817e3abef8d566..64ffdcde045a3a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -517,6 +517,8 @@ Bug Fixes to C++ Support
certain situations. (#GH47400), (#GH90896)
- Fix erroneous templated array size calculation leading to crashes in generated code. (#GH41441)
- During the lookup for a base class name, non-type names are ignored. (#GH16855)
+- Fixed an assertion failure when the default lexical access specifier was set for missing
+ primary declarations. (#GH112208)
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaAccess.cpp b/clang/lib/Sema/SemaAccess.cpp
index df6edb21a50dee..8b4a5b70669d84 100644
--- a/clang/lib/Sema/SemaAccess.cpp
+++ b/clang/lib/Sema/SemaAccess.cpp
@@ -39,7 +39,8 @@ bool Sema::SetMemberAccessSpecifier(NamedDecl *MemberDecl,
AccessSpecifier LexicalAS) {
if (!PrevMemberDecl) {
// Use the lexical access specifier.
- MemberDecl->setAccess(LexicalAS);
+ if (LexicalAS != AS_none)
+ MemberDecl->setAccess(LexicalAS);
return false;
}
diff --git a/clang/test/SemaCXX/enum.cpp b/clang/test/SemaCXX/enum.cpp
index 9c398cc8da886c..44042d8bf5cfc8 100644
--- a/clang/test/SemaCXX/enum.cpp
+++ b/clang/test/SemaCXX/enum.cpp
@@ -143,3 +143,11 @@ struct PR28903 {
})
};
};
+
+namespace GH112208 {
+class C {
+ enum E { e = 0 };
+ void f(int, enum E;); // expected-error {{ISO C++ forbids forward references to 'enum' types}} \
+ // expected-error {{unexpected ';' before ')'}}
+};
+}
|
clang/lib/Sema/SemaAccess.cpp
Outdated
@@ -39,7 +39,8 @@ bool Sema::SetMemberAccessSpecifier(NamedDecl *MemberDecl, | |||
AccessSpecifier LexicalAS) { | |||
if (!PrevMemberDecl) { | |||
// Use the lexical access specifier. | |||
MemberDecl->setAccess(LexicalAS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shafik #112208 (comment) when PrevMemberDecl
exists but isn't equal to LexicalAS
— with AS_none
excluded
if (LexicalAS != AS_none && LexicalAS != PrevMemberDecl->getAccess()) { |
I suppose AS_none
should be excluded when PrevMemberDecl
is unknown. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we should be catching this in Sema::ActOnTag
here:
llvm-project/clang/lib/Sema/SemaDecl.cpp
Lines 17938 to 17951 in b497010
if ((Name || Kind == TagTypeKind::Enum) && | |
getNonFieldDeclScope(S)->isFunctionPrototypeScope()) { | |
if (getLangOpts().CPlusPlus) { | |
// C++ [dcl.fct]p6: | |
// Types shall not be defined in return or parameter types. | |
if (TUK == TagUseKind::Definition && !IsTypeSpecifier) { | |
Diag(Loc, diag::err_type_defined_in_param_type) | |
<< Name; | |
Invalid = true; | |
} | |
} else if (!PrevDecl) { | |
Diag(Loc, diag::warn_decl_in_param_list) << Context.getTagDeclType(New); | |
} | |
} |
It looks like we are not b/c TUK
is TUK_Declataion
but we should not be forward declaring here either.
The current diagnostic does not fit this case though and maybe it has to be tweaked some more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add more details to the summary, it is helpful to have some idea what you think the underlying problem is and why this fix is correct before reviewing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current diagnostic does not fit this case though and maybe it has to be tweaked some more.
@shafik Thanks for the feedback! I initially considered marking the new declaration invalid based on TUK_Declaration
, but I wasn’t entirely sure because of its relation to the new declaration
llvm-project/clang/lib/Sema/SemaDecl.cpp
Lines 17953 to 17954 in 7be1dc0
if (Invalid) | |
New->setInvalidDecl(); |
I’ve made changes to handle this case based on TUK_Declaration. If we need to add a diagnostic message (something like a general message saying "enum" cannot appear here
), just let me know and I’ll add it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shafik could you take a look at the latest changes? thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these changes make sense, the diagnostic on forward reference is not great but that feels like a seperate already existing problem and not directly related to the crash.
Please update the summary. I also had one question on why we are checking Name
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shafik thanks for the feedback. I've checked, and it seems safe to omit Name
here, so I’ve removed it.
clang/lib/Sema/SemaAccess.cpp
Outdated
@@ -39,7 +39,8 @@ bool Sema::SetMemberAccessSpecifier(NamedDecl *MemberDecl, | |||
AccessSpecifier LexicalAS) { | |||
if (!PrevMemberDecl) { | |||
// Use the lexical access specifier. | |||
MemberDecl->setAccess(LexicalAS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these changes make sense, the diagnostic on forward reference is not great but that feels like a seperate already existing problem and not directly related to the crash.
Please update the summary. I also had one question on why we are checking Name
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming it passes the test LGTM
@shafik thanks for the review. It seems the |
Fixes #112208
This PR resolves a crash triggered by a forward reference to an enum type in a function parameter list. The fix includes setting
Invalid
whenTagUseKind
isDeclaration
to ensure correct error handling.