[AST] Support structural equivalence checking of attributes on Decls #168769
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The structural equivalence checker currently treats any explicit attributes on a declaration as a reason to consider the declarations non-equivalent in C23 mode, even when both declarations carry the same attributes. This is unnecessarily strict and causes two otherwise equivalent declarations to be rejected just because they carry explicitly annotated attributes. This patch enables structural equivalence checking to accept selected attributes as long as the attributes on both definitions are equivalent and appear in the same order. The initial implementation adds support for three attributes: Availability, EnumExtensibility, and Unused. Additional attribute kinds can be added incrementally. This design also allows these utilities to be generated automatically by tablegen in the future. Inherited attributes that are supported are ignored when determining structural equivalence, because inherited attributes should not affect whether two definitions are structurally compatible. This patch also moves the call to CheckStructurallyEquivalentAttributes so that attribute comparison is performed between two definitions of record decls rather than between a declaration and a definition. rdar://163304242
ahatanak
requested review from
AaronBallman,
cor3ntin,
erichkeane and
jyknight
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang Author: Akira Hatanaka (ahatanak) ChangesThe structural equivalence checker currently treats any explicit attributes on a declaration as a reason to consider the declarations non-equivalent in C23 mode, even when both declarations carry the same attributes. This is unnecessarily strict and causes two otherwise equivalent declarations to be rejected just because they carry explicitly annotated attributes. This patch enables structural equivalence checking to accept selected attributes as long as the attributes on both definitions are equivalent and appear in the same order. The initial implementation adds support for three attributes: Availability, EnumExtensibility, and Unused. Additional attribute kinds can be added incrementally. This design also allows these utilities to be generated automatically by tablegen in the future. Inherited attributes that are supported are ignored when determining structural equivalence, because inherited attributes should not affect whether two definitions are structurally compatible. This patch also moves the call to CheckStructurallyEquivalentAttributes so that attribute comparison is performed between two definitions of record decls rather than between a declaration and a definition. rdar://163304242 Full diff: https://github.com/llvm/llvm-project/pull/168769.diff 2 Files Affected:
diff --git a/clang/lib/AST/ASTStructuralEquivalence.cpp b/clang/lib/AST/ASTStructuralEquivalence.cpp
index da64c92221837..fc7f44c6ba563 100644
--- a/clang/lib/AST/ASTStructuralEquivalence.cpp
+++ b/clang/lib/AST/ASTStructuralEquivalence.cpp
@@ -93,6 +93,7 @@
#include "llvm/Support/ErrorHandling.h"
#include <cassert>
#include <optional>
+#include <set>
#include <utility>
using namespace clang;
@@ -451,6 +452,123 @@ class StmtComparer {
};
} // namespace
+namespace {
+enum class AttrComparisonKind { Equal, NotEqual };
+
+/// Represents the result of comparing the attribute sets on two decls. If the
+/// sets are incompatible, A1/A2 point to the offending attributes.
+struct AttrComparisonResult {
+ AttrComparisonKind Kind = AttrComparisonKind::Equal;
+ const Attr *A1 = nullptr, *A2 = nullptr;
+};
+} // namespace
+
+static AttrComparisonResult
+areAvailabilityAttrsEqual(const AvailabilityAttr *A1,
+ const AvailabilityAttr *A2) {
+ if (A1->getPlatform() == A2->getPlatform() &&
+ A1->getIntroduced() == A2->getIntroduced() &&
+ A1->getDeprecated() == A2->getDeprecated() &&
+ A1->getObsoleted() == A2->getObsoleted() &&
+ A1->getUnavailable() == A2->getUnavailable() &&
+ A1->getMessage() == A2->getMessage() &&
+ A1->getReplacement() == A2->getReplacement() &&
+ A1->getStrict() == A2->getStrict() &&
+ A1->getPriority() == A2->getPriority() &&
+ A1->getEnvironment() == A2->getEnvironment())
+ return {AttrComparisonKind::Equal};
+ return {AttrComparisonKind::NotEqual, A1, A2};
+}
+
+static AttrComparisonResult
+areEnumExtensibilityAttrsEqual(const EnumExtensibilityAttr *A1,
+ const EnumExtensibilityAttr *A2) {
+ if (A1->getExtensibility() == A2->getExtensibility())
+ return {AttrComparisonKind::Equal};
+ return {AttrComparisonKind::NotEqual, A1, A2};
+}
+
+static AttrComparisonResult areAttrsEqual(const Attr *A1, const Attr *A2) {
+ auto Kind1 = A1->getKind(), Kind2 = A2->getKind();
+ if (Kind1 != Kind2)
+ return {AttrComparisonKind::NotEqual, A1, A2};
+
+ switch (Kind1) {
+ case attr::Availability:
+ return areAvailabilityAttrsEqual(cast<AvailabilityAttr>(A1),
+ cast<AvailabilityAttr>(A2));
+ case attr::EnumExtensibility:
+ return areEnumExtensibilityAttrsEqual(cast<EnumExtensibilityAttr>(A1),
+ cast<EnumExtensibilityAttr>(A2));
+ case attr::Unused:
+ return {AttrComparisonKind::Equal};
+ default:
+ llvm_unreachable("unexpected attr kind");
+ }
+}
+
+static bool compareAttrKind(const Attr *A1, const Attr *A2) {
+ return A1->getKind() < A2->getKind();
+}
+
+namespace {
+using AttrSet = std::multiset<const Attr *, decltype(&compareAttrKind)>;
+}
+
+/// Collects all supported, non-inherited attributes from the given decl.
+/// Returns true on success. If the decl contains any unsupported attribute,
+/// returns false and sets UnsupportedAttr to point to that attribute.
+static bool collectComparableAttrs(const Decl *D, AttrSet &Attrs,
+ const Attr *&UnsupportedAttr) {
+ for (const Attr *A : D->attrs()) {
+ switch (A->getKind()) {
+ case attr::Availability:
+ case attr::EnumExtensibility:
+ case attr::Unused:
+ if (!A->isInherited())
+ Attrs.insert(A);
+ break;
+
+ default:
+ UnsupportedAttr = A;
+ return false; // unsupported attribute
+ }
+ }
+
+ return true;
+}
+
+/// Determines whether D1 and D2 have compatible sets of attributes for the
+/// purposes of structural equivalence checking.
+static AttrComparisonResult areDeclAttrsEquivalent(const Decl *D1,
+ const Decl *D2) {
+ if (D1->isImplicit() || D2->isImplicit())
+ return {AttrComparisonKind::Equal};
+
+ AttrSet A1(&compareAttrKind), A2(&compareAttrKind);
+
+ const Attr *UnsupportedAttr1 = nullptr, *UnsupportedAttr2 = nullptr;
+ bool HasUnsupportedAttr1 = collectComparableAttrs(D1, A1, UnsupportedAttr1);
+ bool HasUnsupportedAttr2 = collectComparableAttrs(D2, A2, UnsupportedAttr2);
+
+ if (!HasUnsupportedAttr1 || !HasUnsupportedAttr2)
+ return {AttrComparisonKind::NotEqual, UnsupportedAttr1, UnsupportedAttr2};
+
+ auto I1 = A1.begin(), E1 = A1.end(), I2 = A2.begin(), E2 = A2.end();
+ for (; I1 != E1 && I2 != E2; ++I1, ++I2) {
+ AttrComparisonResult R = areAttrsEqual(*I1, *I2);
+ if (R.Kind != AttrComparisonKind::Equal)
+ return R;
+ }
+
+ if (I1 != E1)
+ return {AttrComparisonKind::NotEqual, *I1};
+ if (I2 != E2)
+ return {AttrComparisonKind::NotEqual, nullptr, *I2};
+
+ return {AttrComparisonKind::Equal};
+}
+
static bool
CheckStructurallyEquivalentAttributes(StructuralEquivalenceContext &Context,
const Decl *D1, const Decl *D2,
@@ -465,21 +583,17 @@ CheckStructurallyEquivalentAttributes(StructuralEquivalenceContext &Context,
// the same semantic attribute, differences in attribute arguments, order
// in which attributes are applied, how to merge attributes if the types are
// structurally equivalent, etc.
- const Attr *D1Attr = nullptr, *D2Attr = nullptr;
- if (D1->hasAttrs())
- D1Attr = *D1->getAttrs().begin();
- if (D2->hasAttrs())
- D2Attr = *D2->getAttrs().begin();
- if ((D1Attr || D2Attr) && !D1->isImplicit() && !D2->isImplicit()) {
+ AttrComparisonResult R = areDeclAttrsEquivalent(D1, D2);
+ if (R.Kind != AttrComparisonKind::Equal) {
const auto *DiagnoseDecl = cast<TypeDecl>(PrimaryDecl ? PrimaryDecl : D2);
Context.Diag2(DiagnoseDecl->getLocation(),
diag::warn_odr_tag_type_with_attributes)
<< Context.ToCtx.getTypeDeclType(DiagnoseDecl)
<< (PrimaryDecl != nullptr);
- if (D1Attr)
- Context.Diag1(D1Attr->getLoc(), diag::note_odr_attr_here) << D1Attr;
- if (D2Attr)
- Context.Diag1(D2Attr->getLoc(), diag::note_odr_attr_here) << D2Attr;
+ if (R.A1)
+ Context.Diag1(R.A1->getLoc(), diag::note_odr_attr_here) << R.A1;
+ if (R.A2)
+ Context.Diag1(R.A2->getLoc(), diag::note_odr_attr_here) << R.A2;
}
// The above diagnostic is a warning which defaults to an error. If treated
@@ -1791,12 +1905,6 @@ static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
}
}
- // In C23 mode, check for structural equivalence of attributes on the record
- // itself. FIXME: Should this happen in C++ as well?
- if (Context.LangOpts.C23 &&
- !CheckStructurallyEquivalentAttributes(Context, D1, D2))
- return false;
-
// If the records occur in different context (namespace), these should be
// different. This is specially important if the definition of one or both
// records is missing. In C23, different contexts do not make for a different
@@ -1838,6 +1946,12 @@ static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
if (!D1 || !D2)
return !Context.LangOpts.C23;
+ // In C23 mode, check for structural equivalence of attributes on the record
+ // itself. FIXME: Should this happen in C++ as well?
+ if (Context.LangOpts.C23 &&
+ !CheckStructurallyEquivalentAttributes(Context, D1, D2))
+ return false;
+
// If any of the records has external storage and we do a minimal check (or
// AST import) we assume they are equivalent. (If we didn't have this
// assumption then `RecordDecl::LoadFieldsFromExternalStorage` could trigger
diff --git a/clang/test/C/C23/n3037.c b/clang/test/C/C23/n3037.c
index 113ecf74d8bef..0a82e8846d92a 100644
--- a/clang/test/C/C23/n3037.c
+++ b/clang/test/C/C23/n3037.c
@@ -515,3 +515,90 @@ void gh149965(void) {
enum E2 *eptr2;
[[maybe_unused]] __typeof__(x2.h) *ptr2 = eptr2;
}
+
+struct __attribute__((availability(ios, introduced = 14), availability(macos, introduced = 12))) AvailS0 {
+ // c17-note@-1 4 {{previous definition is here}}
+ // c23-note@-2 2 {{attribute 'availability' here}}
+ int f0 __attribute__((availability(ios, introduced = 15, deprecated = 16)));
+ // c23-note@-1 {{attribute 'availability' here}}
+};
+
+struct __attribute__((availability(ios, introduced = 14), availability(macos, introduced = 12))) AvailS0 {
+ // c17-error@-1 {{redefinition of 'AvailS0'}}
+ int f0 __attribute__((availability(ios, introduced = 15, deprecated = 16)));
+};
+
+// The order of the attributes matters.
+struct __attribute__((availability(macos, introduced = 12), availability(ios, introduced = 14))) AvailS0 {
+ // c17-error@-1 {{redefinition of 'AvailS0'}}
+ // c23-error@-2 {{type 'struct AvailS0' has an attribute which currently causes the types to be treated as though they are incompatible}}
+ // c23-note@-3 {{attribute 'availability' here}}
+ int f0 __attribute__((availability(ios, introduced = 15, deprecated = 16)));
+};
+
+struct __attribute__((availability(ios, introduced = 14))) [[__maybe_unused__]] AvailS0 {
+ // c17-error@-1 {{redefinition of 'AvailS0'}}
+ // c23-error@-2 {{type 'struct AvailS0' has an attribute which currently causes the types to be treated as though they are incompatible}}
+ // c23-note@-3 {{attribute 'maybe_unused' here}}
+ int f0 __attribute__((availability(ios, introduced = 15, deprecated = 16)));
+};
+
+struct __attribute__((availability(ios, introduced = 14), availability(macos, introduced = 12))) AvailS0 {
+ // c17-error@-1 {{redefinition of 'AvailS0'}}
+ // c23-error@-2 {{type 'struct AvailS0' has a member with an attribute which currently causes the types to be treated as though they are incompatible}}
+ int f0 __attribute__((availability(macos, introduced = 13)));
+ // c23-note@-1 {{attribute 'availability' here}}
+};
+
+enum __attribute__((availability(macos, introduced = 12), warn_unused_result)) AvailE0 {
+ // c17-note@-1 {{previous definition is here}}
+ // c23-note@-2 {{attribute 'warn_unused_result' here}}
+ A_E0
+ // c17-note@-1 {{previous definition is here}}
+};
+
+enum __attribute__((availability(macos, introduced = 12), warn_unused_result)) AvailE0 {
+ // c17-error@-1 {{redefinition of 'AvailE0'}}
+ // c23-error@-2 {{type 'enum AvailE0' has an attribute which currently causes the types to be treated as though they are incompatible}}
+ // c23-note@-3 {{attribute 'warn_unused_result' here}}
+ A_E0
+ // c17-error@-1 {{redefinition of enumerator 'A_E0'}}
+};
+
+enum __attribute__((enum_extensibility(closed))) AvailE1 {
+ // c17-note@-1 {{previous definition is here}}
+ A_E1
+ // c17-note@-1 {{previous definition is here}}
+};
+
+enum __attribute__((enum_extensibility(closed))) AvailE1 {
+ // c17-error@-1 {{redefinition of 'AvailE1'}}
+ A_E1
+ // c17-error@-1 {{redefinition of enumerator 'A_E1'}}
+};
+
+struct [[__maybe_unused__]] AvailS1;
+
+struct __attribute__((availability(macos, introduced = 12))) AvailS1 {
+ // c17-note@-1 {{previous definition is here}}
+ int a;
+};
+
+struct __attribute__((availability(macos, introduced = 12))) AvailS1 {
+ // c17-error@-1 {{redefinition of 'AvailS1'}}
+ int a;
+};
+
+struct __attribute__((warn_unused_result)) AvailS2;
+// c23-note@-1 {{attribute 'warn_unused_result' here}}
+
+struct __attribute__((availability(macos, introduced = 12))) AvailS2 {
+ // c17-note@-1 {{previous definition is here}}
+ int a;
+};
+
+struct __attribute__((availability(macos, introduced = 12))) AvailS2 {
+ // c17-error@-1 {{redefinition of 'AvailS2'}}
+ // c23-error@-2 {{type 'struct AvailS2' has an attribute which currently causes the types to be treated as though they are incompatible}}
+ int a;
+};
|
🐧 Linux x64 Test Results
|
Collaborator
Author
|
Any comments? |
AaronBallman
reviewed
Dec 2, 2025
Collaborator
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
Thank you for working on this, and sorry for the delayed review (holiday schedules are hard).
In general, I think we should be doing this with tablegen from the start. With the current patch, we have to update a few switch statements for the attribute to be checked properly and I'm worried people will just continue to do ad hoc solutions here instead of doing the tablegen work, especially as the list grows longer (it's easier to do the tablegen work when there's 3 attributes than when there's 40; and we have ~550 attributes in total so we know the ad hoc solution won't scale). And when I think about the fact that a lot of this is keyed off the attribute kind rather than the spelling, I get worried that the current approach isn't really the direction we should be going. The trouble is:
- Same attribute kind doesn't mean same attribute; we have attributes with a single semantic
Attrsubclass but multiple spellings that mean different things. - We also have the situation where attributes share the same spelling but are different semantic attributes. I think this situation is okay though because those attributes should always be target-specific and I think structural equivalence across targets is handled elsewhere anyway.
- Different attribute arguments don't mean the two declarations aren't structurally equivalent. e.g.,
[[deprecated]]with different messages aren't structurally different, are they? But even with availability as in this PR, is a version of10structurally different from10.0different from10.0.0? There's alsoAvailabilityAttr::equivalentPlatformNames(), etc.
Would you be amenable to doing the tablegen approach instead? I was thinking of a design like:
- If there's no other marking in the attribute definition, then a strict equivalence is checked (same kind, same spelling, identical arguments (so
0and0 + 0are structurally different). - Otherwise, there can be a custom comparator implemented explicitly via a
codefield with a signature along the lines ofbool (const T *, const T *).
But the design is flexible if you've got other ideas, too.
| } | ||
|
|
||
| namespace { | ||
| using AttrSet = std::multiset<const Attr *, decltype(&compareAttrKind)>; |
Collaborator
There was a problem hiding this comment.
Would a SmallPtrSet make more sense?
| return areEnumExtensibilityAttrsEqual(cast<EnumExtensibilityAttr>(A1), | ||
| cast<EnumExtensibilityAttr>(A2)); | ||
| case attr::Unused: | ||
| return {AttrComparisonKind::Equal}; |
Collaborator
There was a problem hiding this comment.
Do we want to require the spellings to be the same? e.g., [[maybe_unused]] vs __attribute__((unused))
I think in general we have to because we have attributes with multiple spellings but the same kinds. e.g.,
def Ownership : InheritableAttr {
let Spellings = [Clang<"ownership_holds">, Clang<"ownership_returns">,
Clang<"ownership_takes">];
let Accessors = [Accessor<"isHolds", [Clang<"ownership_holds">]>,
Accessor<"isReturns", [Clang<"ownership_returns">]>,
Accessor<"isTakes", [Clang<"ownership_takes">]>];
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The structural equivalence checker currently treats any explicit attributes on a declaration as a reason to consider the declarations non-equivalent in C23 mode, even when both declarations carry the same attributes. This is unnecessarily strict and causes two otherwise equivalent declarations to be rejected just because they carry explicitly annotated attributes.
This patch enables structural equivalence checking to accept selected attributes as long as the attributes on both definitions are equivalent and appear in the same order. The initial implementation adds support for three attributes: Availability, EnumExtensibility, and Unused.
Additional attribute kinds can be added incrementally. This design also allows these utilities to be generated automatically by tablegen in the future.
Inherited attributes that are supported are ignored when determining structural equivalence, because inherited attributes should not affect whether two definitions are structurally compatible.
This patch also moves the call to CheckStructurallyEquivalentAttributes so that attribute comparison is performed between two definitions of record decls rather than between a declaration and a definition.
rdar://163304242