https://github.com/ahatanak created https://github.com/llvm/llvm-project/pull/168769
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 >From 516b90f080fe28c93e9127686773d3f72c96517b Mon Sep 17 00:00:00 2001 From: Akira Hatanaka <[email protected]> Date: Wed, 19 Nov 2025 12:09:26 -0800 Subject: [PATCH] [AST] Support structural equivalence checking of attributes on Decls 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 --- clang/lib/AST/ASTStructuralEquivalence.cpp | 146 ++++++++++++++++++--- clang/test/C/C23/n3037.c | 87 ++++++++++++ 2 files changed, 217 insertions(+), 16 deletions(-) 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; +}; _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
