[C23] Handle type compatibility for enumerations better #150282
Conversation
An enumeration is compatible with its underlying type, which means that
code like the following should be accepted:
struct A { int h; };
void func() {
extern struct A x;
enum E : int { e };
struct A { enum E h; };
extern struct A x;
}
because the structures are declared in different scopes, the two
declarations of 'x' are both compatible.
Note, the structural equivalence checker does not take scope into
account, but that is something the C standard requires. This means we
are accepting code we should be rejecting per the standard, like:
void func() {
struct A { int h; };
extern struct A x;
enum E : int { e };
struct A { enum E h; };
extern struct A x;
}
Because the structures are declared in the same scope, the type
compatibility rule require the structures to use the same types, not
merely compatible ones.
Fixes llvm#149965
AaronBallman
requested review from
jyknight,
Fznamznon,
efriedma-quic and
Sirraide
AaronBallman
added
clang
|
@llvm/pr-subscribers-clang Author: Aaron Ballman (AaronBallman) ChangesAn enumeration is compatible with its underlying type, which means that code like the following should be accepted: struct A { int h; }; because the structures are declared in different scopes, the two declarations of 'x' are both compatible. Note, the structural equivalence checker does not take scope into account, but that is something the C standard requires. This means we are accepting code we should be rejecting per the standard, like: void func() { Because the structures are declared in the same scope, the type compatibility rule require the structures to use the same types, not merely compatible ones. Fixes #149965 Full diff: https://github.com/llvm/llvm-project/pull/150282.diff 2 Files Affected:
diff --git a/clang/lib/AST/ASTStructuralEquivalence.cpp b/clang/lib/AST/ASTStructuralEquivalence.cpp
index 0f2762d5c0f14..f113b32d6eb30 100644
--- a/clang/lib/AST/ASTStructuralEquivalence.cpp
+++ b/clang/lib/AST/ASTStructuralEquivalence.cpp
@@ -870,7 +870,29 @@ static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
else if (T1->getTypeClass() == Type::FunctionNoProto &&
T2->getTypeClass() == Type::FunctionProto)
TC = Type::FunctionNoProto;
- else
+ else if (Context.LangOpts.C23 && !Context.StrictTypeSpelling &&
+ (T1->getTypeClass() == Type::Enum ||
+ T2->getTypeClass() == Type::Enum)) {
+ // In C23, if not being strict about token equivalence, we need to handle
+ // the case where one type is an enumeration and the other type is an
+ // integral type.
+ //
+ // C23 6.7.3.3p16: The enumerated type is compatible with the underlying
+ // type of the enumeration.
+ //
+ // Treat the enumeration as its underlying type and use the builtin type
+ // class comparison.
+ if (T1->getTypeClass() == Type::Enum) {
+ T1 = T1->getAs<EnumType>()->getDecl()->getIntegerType();
+ if (!T2->isBuiltinType() || T1.isNull()) // Sanity check
+ return false;
+ } else if (T2->getTypeClass() == Type::Enum) {
+ T2 = T2->getAs<EnumType>()->getDecl()->getIntegerType();
+ if (!T1->isBuiltinType() || T2.isNull()) // Sanity check
+ return false;
+ }
+ TC = Type::Builtin;
+ } else
return false;
}
diff --git a/clang/test/C/C23/n3037.c b/clang/test/C/C23/n3037.c
index ce6f4c4ea7acf..54e8ab43ec271 100644
--- a/clang/test/C/C23/n3037.c
+++ b/clang/test/C/C23/n3037.c
@@ -401,3 +401,30 @@ _Static_assert(0 == _Generic(inner_anon_tagged.untagged, struct { int i; } : 1,
// unions and structures are both RecordDecl objects, whereas EnumDecl is not).
enum { E_Untagged1 } nontag_enum; // both-note {{previous definition is here}}
_Static_assert(0 == _Generic(nontag_enum, enum { E_Untagged1 } : 1, default : 0)); // both-error {{redefinition of enumerator 'E_Untagged1'}}
+
+// Test that enumerations are compatible with their underlying type, but still
+// diagnose when "same type" is required rather than merely "compatible type".
+struct GH149965_1 { int h; };
+struct GH149965_2 { int h; };
+void gh149965(void) {
+ extern struct GH149965_1 x1; // c17-note {{previous declaration is here}}
+ extern struct GH149965_2 x2; // c17-note {{previous declaration is here}}
+
+ enum E1 : int { e1 }; // Fixed underlying type
+ enum E2 { e2 }; // Unfixed underlying type, defaults to int in Clang (unsigned in GCC)
+
+ // Both the structure and the variable declarations are fine because only a
+ // compatible type is required, not the same type, because the structures are
+ // declared in different scopes.
+ struct GH149965_1 { enum E1 h; };
+ struct GH149965_2 { enum E2 h; };
+
+ extern struct GH149965_1 x1; // c17-error {{redeclaration of 'x1'}}
+ extern struct GH149965_2 x2; // c17-error {{redeclaration of 'x2'}}
+
+ // However, in the same scope, the same type is required, not just compatible
+ // types.
+ // FIXME: this should be an error in both C17 and C23 mode.
+ struct GH149965_3 { int h; }; // c17-note {{previous definition is here}}
+ struct GH149965_3 { enum E1 h; }; // c17-error {{redefinition of 'GH149965_3'}}
+}
|
|
There's no release note because this is covered by the existing one about N3037. Assuming this patch is landed in time, it would be cherry-picked to the 21.x branch (which also has the release note). If it's not cherry-picked, then I'll add a release note to the main branch. |
|
Ping (note #150946 is related but different) |
| if (!T2->isBuiltinType() || T1.isNull()) // Sanity check | ||
| return false; |
There was a problem hiding this comment.
Should the null check be an assert?
There was a problem hiding this comment.
Yeah, if the underlying type is invalid, I’d expect us to default to int, so it should probably never be null.
There was a problem hiding this comment.
In general, yes, but given that I want to put this on the release branch with little time for finding fallout, I'd prefer to not assert; we mostly recover by using int as a type, but that's not a guarantee.
How about this for an idea, leave it this way, land the changes, pick the changes to 21.x, once the pick is finished, make this an assert on main?
There was a problem hiding this comment.
In general, yes, but given that I want to put this on the release branch with little time for finding fallout, I'd prefer to not assert; we mostly recover by using
intas a type, but that's not a guarantee.How about this for an idea, leave it this way, land the changes, pick the changes to 21.x, once the pick is finished, make this an assert on main?
sgtm
There was a problem hiding this comment.
How about this for an idea, leave it this way, land the changes, pick the changes to 21.x, once the pick is finished, make this an assert on main?
Yes, please!
| if (!T2->isBuiltinType() || T1.isNull()) // Sanity check | ||
| return false; |
There was a problem hiding this comment.
Yeah, if the underlying type is invalid, I’d expect us to default to int, so it should probably never be null.
github-project-automation
bot
moved this from Needs Backport PR
to Needs Merge
in LLVM Release Status
|
@AaronBallman can you squash this? (release procedure is a bit different so we prefer to get it pre-squashed). |
Sure, but I'm not certain we should have a different merge strategy for main vs release branches so I'd like to understand more about what's driving this. Are we planning on switching all contributions to squashing first? Or is this a temporary thing for release branches? |
We can't use the github squash and merge workflow on the release branch, it makes it so the PR and the commit is not correctly linked. So we use a script to merge PR's from the release branch instead, this script does a straight rebase merge, so when we have multiple commits that are iterative work in a release pr we ask the backporters to squash them. I can also do it if you prefer. This limitation is only for the release branches and we hope that the bug we have with the github merge button will go away at some point. |
|
Oh never mind @AaronBallman I just noticed that this PR was targeting |
Thank you for the explanation! That's not a good situation. :-(
No worries! Now this is all making more sense to me. :-) |
github-project-automation
bot
moved this from Needs Merge
to Done
in LLVM Release Status
|
/cherry-pick 315e2e2 |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/24281 Here is the relevant piece of the build log for the reference |
|
/pull-request #151137 |
An enumeration is compatible with its underlying type, which means that
code like the following should be accepted:
struct A { int h; };
void func() {
extern struct A x;
enum E : int { e };
struct A { enum E h; };
extern struct A x;
}
because the structures are declared in different scopes, the two
declarations of 'x' are both compatible.
Note, the structural equivalence checker does not take scope into
account, but that is something the C standard requires. This means we
are accepting code we should be rejecting per the standard, like:
void func() {
struct A { int h; };
extern struct A x;
enum E : int { e };
struct A { enum E h; };
extern struct A x;
}
Because the structures are declared in the same scope, the type
compatibility rule require the structures to use the same types, not
merely compatible ones.
Fixes llvm#149965
(cherry picked from commit 315e2e2)
An enumeration is compatible with its underlying type, which means that code like the following should be accepted:
struct A { int h; };
void func() {
extern struct A x;
enum E : int { e };
struct A { enum E h; };
extern struct A x;
}
because the structures are declared in different scopes, the two declarations of 'x' are both compatible.
Note, the structural equivalence checker does not take scope into account, but that is something the C standard requires. This means we are accepting code we should be rejecting per the standard, like:
void func() {
struct A { int h; };
extern struct A x;
enum E : int { e };
struct A { enum E h; };
extern struct A x;
}
Because the structures are declared in the same scope, the type compatibility rule require the structures to use the same types, not merely compatible ones.
Fixes #149965