[C23] Implement WG14 N3037 #132939
[C23] Implement WG14 N3037 #132939
Conversation
This changes the type compatibility rules so that it is permitted to redefine tag types within the same TU so long as they are equivalent definitions.
and simplify the implementation somewhat.
AaronBallman
added
clang
AaronBallman
requested review from
Sirraide,
efriedma-quic,
jyknight and
zygoloid
|
@llvm/pr-subscribers-clang Author: Aaron Ballman (AaronBallman) ChangesThis changes the type compatibility rules so that it is permitted to redefine tag types within the same TU so long as they are equivalent definitions. It is intentionally not being exposed as an extension in older C language modes. GCC does not do so and the feature doesn't seem compelling enough to warrant it. Patch is 75.33 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/132939.diff 20 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 97f6ea90d4705..07d29057be86c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -152,6 +152,10 @@ C23 Feature Support
which clarified that a compound literal used within a function prototype is
treated as if the compound literal were within the body rather than at file
scope.
+- Implemented `WG14 N3037 <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3037.pdf>`_
+ which allows tag types to be redefined within the same translation unit so
+ long as both definitions are structurally equivalent (same tag types, same
+ tag names, same tag members, etc).
Non-comprehensive list of changes in this release
-------------------------------------------------
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index af8c49e99a7ce..2e94b475209bf 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -3127,6 +3127,7 @@ class ASTContext : public RefCountedBase<ASTContext> {
QualType mergeTransparentUnionType(QualType, QualType,
bool OfBlockPointer=false,
bool Unqualified = false);
+ QualType mergeTagTypes(QualType, QualType);
QualType mergeObjCGCQualifiers(QualType, QualType);
diff --git a/clang/include/clang/AST/ASTStructuralEquivalence.h b/clang/include/clang/AST/ASTStructuralEquivalence.h
index 67aa0023c25d0..18fdc65624db3 100644
--- a/clang/include/clang/AST/ASTStructuralEquivalence.h
+++ b/clang/include/clang/AST/ASTStructuralEquivalence.h
@@ -43,6 +43,9 @@ struct StructuralEquivalenceContext {
/// key: (from, to, IgnoreTemplateParmDepth)
using NonEquivalentDeclSet = llvm::DenseSet<std::tuple<Decl *, Decl *, int>>;
+ /// The language options to use for making a structural equivalence check.
+ const LangOptions &LangOpts;
+
/// AST contexts for which we are checking structural equivalence.
ASTContext &FromCtx, &ToCtx;
@@ -76,15 +79,17 @@ struct StructuralEquivalenceContext {
/// Whether to ignore comparing the depth of template param(TemplateTypeParm)
bool IgnoreTemplateParmDepth;
- StructuralEquivalenceContext(ASTContext &FromCtx, ASTContext &ToCtx,
+ StructuralEquivalenceContext(const LangOptions &LangOpts, ASTContext &FromCtx,
+ ASTContext &ToCtx,
NonEquivalentDeclSet &NonEquivalentDecls,
StructuralEquivalenceKind EqKind,
bool StrictTypeSpelling = false,
bool Complain = true,
bool ErrorOnTagTypeMismatch = false,
bool IgnoreTemplateParmDepth = false)
- : FromCtx(FromCtx), ToCtx(ToCtx), NonEquivalentDecls(NonEquivalentDecls),
- EqKind(EqKind), StrictTypeSpelling(StrictTypeSpelling),
+ : LangOpts(LangOpts), FromCtx(FromCtx), ToCtx(ToCtx),
+ NonEquivalentDecls(NonEquivalentDecls), EqKind(EqKind),
+ StrictTypeSpelling(StrictTypeSpelling),
ErrorOnTagTypeMismatch(ErrorOnTagTypeMismatch), Complain(Complain),
IgnoreTemplateParmDepth(IgnoreTemplateParmDepth) {}
diff --git a/clang/include/clang/Basic/DiagnosticASTKinds.td b/clang/include/clang/Basic/DiagnosticASTKinds.td
index 8d69a2f2cf4a3..f5e22196867d9 100644
--- a/clang/include/clang/Basic/DiagnosticASTKinds.td
+++ b/clang/include/clang/Basic/DiagnosticASTKinds.td
@@ -478,16 +478,24 @@ def warn_odr_function_type_inconsistent : Warning<
"external function %0 declared with incompatible types in different "
"translation units (%1 vs. %2)">,
InGroup<ODR>;
+def err_odr_attr_inconsistent : Error<
+ "attribute %0 is incompatible%select{| in different translation units}1">;
+def warn_odr_attr_inconsistent : Warning<
+ err_odr_attr_inconsistent.Summary>, InGroup<ODR>;
+def note_odr_attr : Note<"attribute %0 here">;
+def note_odr_attr_missing : Note<"no corresponding attribute here">;
def err_odr_tag_type_inconsistent
- : Error<"type %0 has incompatible definitions in different translation "
- "units">;
+ : Error<"type %0 has incompatible definitions%select{| in different "
+ "translation units}1">;
def warn_odr_tag_type_inconsistent
- : Warning<"type %0 has incompatible definitions in different translation "
- "units">,
+ : Warning<"type %0 has incompatible definitions%select{| in different "
+ "translation units}1">,
InGroup<ODR>;
def note_odr_tag_kind_here: Note<
"%0 is a %select{struct|interface|union|class|enum}1 here">;
def note_odr_field : Note<"field %0 has type %1 here">;
+def note_odr_field_bit_width : Note<"bit-field %0 has bit-width %1 here">;
+def note_odr_field_not_bit_field : Note<"field %0 is not a bit-field">;
def note_odr_field_name : Note<"field has name %0 here">;
def note_odr_missing_field : Note<"no corresponding field here">;
def note_odr_base : Note<"class has base type %0">;
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index fbe2865b1b7c1..f87f9bb93f076 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -2549,7 +2549,8 @@ class Parser : public CodeCompletionHandler {
void ParseEnumSpecifier(SourceLocation TagLoc, DeclSpec &DS,
const ParsedTemplateInfo &TemplateInfo,
AccessSpecifier AS, DeclSpecContext DSC);
- void ParseEnumBody(SourceLocation StartLoc, Decl *TagDecl);
+ void ParseEnumBody(SourceLocation StartLoc, Decl *TagDecl,
+ SkipBodyInfo *SkipBody = nullptr);
void ParseStructUnionBody(SourceLocation StartLoc, DeclSpec::TST TagType,
RecordDecl *TagDecl);
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index e215f07e2bf0a..e324bd07be24a 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4078,7 +4078,8 @@ class Sema final : public SemaBase {
Decl *ActOnEnumConstant(Scope *S, Decl *EnumDecl, Decl *LastEnumConstant,
SourceLocation IdLoc, IdentifierInfo *Id,
const ParsedAttributesView &Attrs,
- SourceLocation EqualLoc, Expr *Val);
+ SourceLocation EqualLoc, Expr *Val,
+ SkipBodyInfo *SkipBody = nullptr);
void ActOnEnumBody(SourceLocation EnumLoc, SourceRange BraceRange,
Decl *EnumDecl, ArrayRef<Decl *> Elements, Scope *S,
const ParsedAttributesView &Attr);
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index de868ac821745..98d9b05cac963 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -16,6 +16,7 @@
#include "clang/AST/APValue.h"
#include "clang/AST/ASTConcept.h"
#include "clang/AST/ASTMutationListener.h"
+#include "clang/AST/ASTStructuralEquivalence.h"
#include "clang/AST/ASTTypeTraits.h"
#include "clang/AST/Attr.h"
#include "clang/AST/AttrIterator.h"
@@ -11432,6 +11433,22 @@ static QualType mergeEnumWithInteger(ASTContext &Context, const EnumType *ET,
return {};
}
+QualType ASTContext::mergeTagTypes(QualType LHS, QualType RHS) {
+ // C17 and earlier and C++ disallow two tag definitions within the same TU
+ // from being compatible.
+ if (LangOpts.CPlusPlus || !LangOpts.C23)
+ return {};
+
+ // C23, on the other hand, requires the members to be "the same enough", so
+ // we use a structural equivalence check.
+ StructuralEquivalenceContext::NonEquivalentDeclSet NonEquivalentDecls;
+ StructuralEquivalenceContext Ctx(
+ getLangOpts(), *this, *this, NonEquivalentDecls,
+ StructuralEquivalenceKind::Default, false /*StrictTypeSpelling*/,
+ false /*Complain*/, true /*ErrorOnTagTypeMismatch*/);
+ return Ctx.IsEquivalent(LHS, RHS) ? LHS : QualType{};
+}
+
QualType ASTContext::mergeTypes(QualType LHS, QualType RHS, bool OfBlockPointer,
bool Unqualified, bool BlockReturnType,
bool IsConditionalOperator) {
@@ -11728,7 +11745,7 @@ QualType ASTContext::mergeTypes(QualType LHS, QualType RHS, bool OfBlockPointer,
/*AllowCXX=*/false, IsConditionalOperator);
case Type::Record:
case Type::Enum:
- return {};
+ return mergeTagTypes(LHS, RHS);
case Type::Builtin:
// Only exactly equal builtin types are compatible, which is tested above.
return {};
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 1db30b3f3f76f..f7dba4163d1cb 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -2497,8 +2497,9 @@ bool ASTNodeImporter::IsStructuralMatch(Decl *From, Decl *To, bool Complain,
}
StructuralEquivalenceContext Ctx(
- Importer.getFromContext(), Importer.getToContext(),
- Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer),
+ Importer.getToContext().getLangOpts(), Importer.getFromContext(),
+ Importer.getToContext(), Importer.getNonEquivalentDecls(),
+ getStructuralEquivalenceKind(Importer),
/*StrictTypeSpelling=*/false, Complain, /*ErrorOnTagTypeMismatch=*/false,
IgnoreTemplateParmDepth);
return Ctx.IsEquivalent(From, To);
@@ -4352,7 +4353,8 @@ static bool IsEquivalentFriend(ASTImporter &Importer, FriendDecl *FD1,
ASTImporter::NonEquivalentDeclSet NonEquivalentDecls;
StructuralEquivalenceContext Ctx(
- FD1->getASTContext(), FD2->getASTContext(), NonEquivalentDecls,
+ Importer.getToContext().getLangOpts(), FD1->getASTContext(),
+ FD2->getASTContext(), NonEquivalentDecls,
StructuralEquivalenceKind::Default,
/* StrictTypeSpelling = */ false, /* Complain = */ false);
return Ctx.IsEquivalent(FD1, FD2);
@@ -10580,8 +10582,8 @@ bool ASTImporter::IsStructurallyEquivalent(QualType From, QualType To,
}
}
- StructuralEquivalenceContext Ctx(FromContext, ToContext, NonEquivalentDecls,
- getStructuralEquivalenceKind(*this), false,
- Complain);
+ StructuralEquivalenceContext Ctx(
+ getToContext().getLangOpts(), FromContext, ToContext, NonEquivalentDecls,
+ getStructuralEquivalenceKind(*this), false, Complain);
return Ctx.IsEquivalent(From, To);
}
diff --git a/clang/lib/AST/ASTStructuralEquivalence.cpp b/clang/lib/AST/ASTStructuralEquivalence.cpp
index 2c2c8fd677500..c3a0980edc1ef 100644
--- a/clang/lib/AST/ASTStructuralEquivalence.cpp
+++ b/clang/lib/AST/ASTStructuralEquivalence.cpp
@@ -61,6 +61,7 @@
#include "clang/AST/ASTStructuralEquivalence.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/ASTDiagnostic.h"
+#include "clang/AST/Attr.h"
#include "clang/AST/Decl.h"
#include "clang/AST/DeclBase.h"
#include "clang/AST/DeclCXX.h"
@@ -111,6 +112,8 @@ static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
NestedNameSpecifier *NNS1,
NestedNameSpecifier *NNS2);
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
+ const Attr *Attr1, const Attr *Attr2);
static bool IsStructurallyEquivalent(const IdentifierInfo *Name1,
const IdentifierInfo *Name2);
@@ -450,6 +453,116 @@ class StmtComparer {
};
} // namespace
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
+ const Attr *Attr1, const Attr *Attr2) {
+ // Two attributes are structurally equivalent if they are the same kind
+ // of attribute, spelled with the same spelling kind, and have the same
+ // arguments. This means that [[noreturn]] and __attribute__((noreturn)) are
+ // not structurally equivalent, nor are [[nodiscard("foo")]] and
+ // [[nodiscard("bar")]].
+ if (Attr1->getKind() != Attr2->getKind())
+ return false;
+
+ if (Attr1->getSyntax() != Attr2->getSyntax())
+ return false;
+
+ if (Attr1->getSpellingListIndex() != Attr2->getSpellingListIndex())
+ return false;
+
+ auto GetAttrName = [](const Attr *A) {
+ if (const IdentifierInfo *II = A->getAttrName())
+ return II->getName();
+ return StringRef{};
+ };
+
+ if (GetAttrName(Attr1) != GetAttrName(Attr2))
+ return false;
+
+ // FIXME: check the attribute arguments. Attr does not track the arguments on
+ // the base class, which makes this awkward. We may want to tablegen a
+ // comparison function for attributes? In the meantime, we're doing this the
+ // cheap way by pretty printing the attributes and checking they produce
+ // equivalent string representations.
+ std::string AttrStr1, AttrStr2;
+ PrintingPolicy DefaultPolicy(Context.LangOpts);
+ llvm::raw_string_ostream SS1(AttrStr1), SS2(AttrStr2);
+ Attr1->printPretty(SS1, DefaultPolicy);
+ Attr2->printPretty(SS2, DefaultPolicy);
+
+ return SS1.str() == SS2.str();
+}
+
+static bool
+CheckStructurallyEquivalentAttributes(StructuralEquivalenceContext &Context,
+ const Decl *D1, const Decl *D2,
+ const Decl *PrimaryDecl = nullptr) {
+ // Gather the attributes and sort them by name so that they're in equivalent
+ // orders. This means that __attribute__((foo, bar)) is equivalent to
+ // __attribute__((bar, foo)).
+ llvm::SmallVector<const Attr *, 2> Attrs1, Attrs2;
+ auto CopyAttrs = [](auto &&Range, llvm::SmallVectorImpl<const Attr *> &Cont) {
+ for (const Attr *A : Range)
+ Cont.push_back(A);
+ };
+ CopyAttrs(D1->attrs(), Attrs1);
+ CopyAttrs(D2->attrs(), Attrs2);
+
+ auto Sorter = [](const Attr *LHS, const Attr *RHS) {
+ const IdentifierInfo *II1 = LHS->getAttrName(), *II2 = RHS->getAttrName();
+ if (!II1 || !II2)
+ return II1 == II2;
+ return *II1 < *II2;
+ };
+ llvm::sort(Attrs1, Sorter);
+ llvm::sort(Attrs2, Sorter);
+
+ auto A2 = Attrs2.begin(), A2End = Attrs2.end();
+ const auto *DiagnoseDecl = cast<TypeDecl>(PrimaryDecl ? PrimaryDecl : D2);
+ for (auto A1 = Attrs1.begin(), A1End = Attrs1.end(); A1 != A1End;
+ ++A1, ++A2) {
+ if (A2 == A2End) {
+ if (Context.Complain) {
+ Context.Diag2(DiagnoseDecl->getLocation(),
+ Context.getApplicableDiagnostic(
+ diag::err_odr_tag_type_inconsistent))
+ << Context.ToCtx.getTypeDeclType(DiagnoseDecl)
+ << (&Context.FromCtx != &Context.ToCtx);
+ Context.Diag1((*A1)->getLocation(), diag::note_odr_attr) << *A1;
+ Context.Diag2(D2->getLocation(), diag::note_odr_attr_missing);
+ }
+ return false;
+ }
+
+ if (!IsStructurallyEquivalent(Context, *A1, *A2)) {
+ if (Context.Complain) {
+ Context.Diag2(DiagnoseDecl->getLocation(),
+ Context.getApplicableDiagnostic(
+ diag::err_odr_tag_type_inconsistent))
+ << Context.ToCtx.getTypeDeclType(DiagnoseDecl)
+ << (&Context.FromCtx != &Context.ToCtx);
+ Context.Diag2((*A2)->getLocation(), diag::note_odr_attr) << *A2;
+ Context.Diag1((*A1)->getLocation(), diag::note_odr_attr) << *A1;
+ }
+ return false;
+ }
+ }
+
+ if (A2 != A2End) {
+ if (Context.Complain) {
+ Context.Diag2(
+ DiagnoseDecl->getLocation(),
+ Context.getApplicableDiagnostic(diag::err_odr_tag_type_inconsistent))
+ << Context.ToCtx.getTypeDeclType(DiagnoseDecl)
+ << (&Context.FromCtx != &Context.ToCtx);
+ Context.Diag1(D1->getLocation(), diag::note_odr_attr_missing);
+ Context.Diag1((*A2)->getLocation(), diag::note_odr_attr) << *A2;
+ }
+ return false;
+ }
+
+ return true;
+}
+
static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
const UnaryOperator *E1,
const CXXOperatorCallExpr *E2) {
@@ -1454,6 +1567,12 @@ static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
QualType Owner2Type) {
const auto *Owner2 = cast<Decl>(Field2->getDeclContext());
+ // In C23 mode, check for structural equivalence of attributes on the fields.
+ // FIXME: Should this happen in C++ as well?
+ if (Context.LangOpts.C23 &&
+ !CheckStructurallyEquivalentAttributes(Context, Field1, Field2, Owner2))
+ return false;
+
// For anonymous structs/unions, match up the anonymous struct/union type
// declarations directly, so that we don't go off searching for anonymous
// types
@@ -1472,7 +1591,7 @@ static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
Context.Diag2(
Owner2->getLocation(),
Context.getApplicableDiagnostic(diag::err_odr_tag_type_inconsistent))
- << Owner2Type;
+ << Owner2Type << (&Context.FromCtx != &Context.ToCtx);
Context.Diag2(Field2->getLocation(), diag::note_odr_field_name)
<< Field2->getDeclName();
Context.Diag1(Field1->getLocation(), diag::note_odr_field_name)
@@ -1487,7 +1606,7 @@ static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
Context.Diag2(
Owner2->getLocation(),
Context.getApplicableDiagnostic(diag::err_odr_tag_type_inconsistent))
- << Owner2Type;
+ << Owner2Type << (&Context.FromCtx != &Context.ToCtx);
Context.Diag2(Field2->getLocation(), diag::note_odr_field)
<< Field2->getDeclName() << Field2->getType();
Context.Diag1(Field1->getLocation(), diag::note_odr_field)
@@ -1496,9 +1615,37 @@ static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
return false;
}
- if (Field1->isBitField())
- return IsStructurallyEquivalent(Context, Field1->getBitWidth(),
- Field2->getBitWidth());
+ if ((Field1->isBitField() || Field2->isBitField()) &&
+ !IsStructurallyEquivalent(Context, Field1->getBitWidth(),
+ Field2->getBitWidth())) {
+ if (Context.Complain) {
+ auto DiagNote = [&](const FieldDecl *FD,
+ DiagnosticBuilder (
+ StructuralEquivalenceContext::*Diag)(
+ SourceLocation, unsigned)) {
+ if (FD->isBitField()) {
+ std::string Str;
+ llvm::raw_string_ostream OS(Str);
+ PrintingPolicy Policy(Context.LangOpts);
+ FD->getBitWidth()->printPretty(OS, nullptr, Policy);
+
+ (Context.*Diag)(FD->getLocation(), diag::note_odr_field_bit_width)
+ << FD->getDeclName() << OS.str();
+ } else {
+ (Context.*Diag)(FD->getLocation(), diag::note_odr_field_not_bit_field)
+ << FD->getDeclName();
+ }
+ };
+
+ Context.Diag2(
+ Owner2->getLocation(),
+ Context.getApplicableDiagnostic(diag::err_odr_tag_type_inconsistent))
+ << Owner2Type << (&Context.FromCtx != &Context.ToCtx);
+ DiagNote(Field2, &StructuralEquivalenceContext::Diag2);
+ DiagNote(Field1, &StructuralEquivalenceContext::Diag1);
+ }
+ return false;
+ }
return true;
}
@@ -1635,6 +1782,28 @@ static bool NameIsStructurallyEquivalent(const TagDecl &D1, const TagDecl &D2) {
/// Determine structural equivalence of two records.
static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
RecordDecl *D1, RecordDecl *D2) {
+ // C23 6.2.7p1:
+ // ... Moreover, two complete structure, union, or enumerated types declared
+ // with the same tag are compatible if members satisfy the following
+ // requirements:
+ // - there shall be a one-to-one correspondence between their members such
+ // that each pair of corresponding members are declared with compatible
+ // types;
+ // - if one member of the pair is declared with an alignment specifier, the
+ // other is declared with an equivalent alignment specifier;
+ // - and, if one member of the pair is declared with a name, the other is
+ // declared with the same name.
+ // For two s...
[truncated]
|
| // FIXME: check the attribute arguments. Attr does not track the arguments on | ||
| // the base class, which makes this awkward. We may want to tablegen a | ||
| // comparison function for attributes? In the meantime, we're doing this the | ||
| // cheap way by pretty printing the attributes and checking they produce |
There was a problem hiding this comment.
I am slightly uncomfortable using pretty printing, I mean theoretically we usually don't consider pretty printing load bearing as such. This is probably an ok short-term trade off but this feels potentially fragile.
There was a problem hiding this comment.
Agreed. I also have design concerns. N3037 is silent on attributes.
Should we maybe conservatively not allow redeclarations of entities relying on compiler extensions and such?
The whole feature seems extremely brittle to maintain in the long run - and to test - so maybe an approach of limiting structural equivalence to things the standard mandates or are motivated by user request is preferable
There was a problem hiding this comment.
consider
[[deprecated]] int a;
int a; // fine
struct S;
struct [[deprecated]] S { // fine
int a;
};
struct S { // not fine?
[[deprecated]] int a; // not fine?
};There was a problem hiding this comment.
I am slightly uncomfortable using pretty printing, I mean theoretically we usually don't consider pretty printing load bearing as such. This is probably an ok short-term trade off but this feels potentially fragile.
Agreed, hence the FIXME. It's a temporary practical solution, but we don't have the underlying machinery to compare attribute arguments generically and that's far outside of the scope of this already large patch.
Agreed. I also have design concerns. N3037 is silent on attributes.
Mismatched attributes are UB by omission, so I went with the most conservative model which is: all attributes have to be identical across objects to be structurally equivalent.
struct S; struct [[deprecated]] S { // fine int a; };
Correct.
struct S { // not fine? [[deprecated]] int a; // not fine? };
Also correct.
There was a problem hiding this comment.
so maybe an approach of limiting structural equivalence to things the standard mandates or are motivated by user request is preferable
That's what this is doing. The standard is silent on whether two tags with different attributes (on the tag or on a member) are compatible or not. So we're taking the conservative approach and saying they're not compatible except when the attributes are the same between the two. Order of attributes doesn't matter because the standard explicitly says it never matters.
There was a problem hiding this comment.
If we treat two types as incompatible when they ought to be compatible, it potentially could result in silent miscompilation of a valid program
Except this is about compatibility of types within the same TU, so there's no miscompile possible because the user gets an error about redefining the same type twice.
If we were to accept two tag definitions as being compatible but they have different attributes, we then have to answer questions like:
struct S {
[[deprecated]] int x;
};
struct S {
int x;
};
Does this mean that use of x should not generate a deprecation warning because the later definition "strips" it? Or should x always get the deprecation warning? If it should, then what happens in a case like:
struct S {
[[deprecated("foo")]] int x;
};
struct S {
[[deprecated("bar")]] int x;
};
which diagnostic is expected? "foo" because it was the first (and thus canonical) definition? "bar" because it was the last definition? Pick randomly?
Rather than have to figure this sort of behavior out (which will almost certainly require case-by-case logic given the 400+ attributes we support), I think it's better to say the attributes have to be the same in order to be compatible; if they're not compatible, we reject the code. It has no impact on cross-TU boundary behavior, only same-TU behavior.
There was a problem hiding this comment.
Please try to engage in the discussion in good faith rather than with snark.
Sorry, it was not meant as snark but to illustrate the point that not mentioning something explicitly in the condition does not imply that there is UB. Otherwise, what would be your rule to determine all the things that need to mentioned or not?
The standard says (C23 6.2.7p1) "Two types are compatible types if they are the same." and then gives additional restrictions on what makes two types [in]compatible. At the end of the paragraph it says "Otherwise, the structure, union, or enumerated types are incompatible.". So the default is that two types are never compatible unless they meet a list of explicit criteria. That criteria does not mention attributes. The high-level decision tree is: are the two types the same? If not, do the two types meet these additional explicitly listed requirements? If not, they're not compatible.
The conditions which says when they are compatible does not refer to attributes, so (standard) attributes do not matter. Once the conditions is fulfill (i.e. the listed conditions are true), then those types are compatible. There is no ambiguity.
So either this situation is UB by omission because the standard doesn't say what to do with attributes
or two tag types are never compatible in the presence of attributes because they're not listed as an exception to the "same type" general rule.
If we say "A and B" are compatible when A and B share property X, then this does not imply that there are incompatible if A and B do not share another property Y. It simply means that you need to check property X while property Y is irrelevant for this question.
UB by omission seems more reasonable to me as it gives implementations thefreedom to decide what is and isn't sufficiently compatible, which is needed anyway for vendor attributes.
(And again, the ideal goal here is to eventually let attributes handle this on an attribute-by-attribute basis.)
Yes, and for vendor attributes I think you have essentially no other choice.
There was a problem hiding this comment.
Just catching up here, with a perhaps new perspective:
We have so many attributes that do so many different things (including vendor extensions, and extensions from OTHER vendors we support), including some that change types, and some that could potentially depend on ordering and context/lookup context.
Therefore textual equivalence is NOT sufficient to make sure these are compatible types. Additionally, we have ''effective' attributes' that are specified by pragmas, or just inheritance that are not necessarily going to be the same thing even if textually the same.
Therefore, it is my preference/insistence that we need to white-list attributes that we know that don't alter the type. For now (as this patch/discussion has gotten large enough), it is sufficient and conservative if we just make the presence of ANY attributes on either declaration enough to make them not structurally equivalent.
At that point, followup patches from interested contributors to white-list specific ones that we have analyzed to make sure that they don't modify the equivalence for the purposes of C redefinitions are welcome.
Hopefully after we do a handful of these, the infrastructure on how to make specifying these more equal is obvious, and we can make it a flag in Attr.td.
There was a problem hiding this comment.
Therefore textual equivalence is NOT sufficient to make sure these are compatible types. Additionally, we have ''effective' attributes' that are specified by pragmas, or just inheritance that are not necessarily going to be the same thing even if textually the same.
Oh, that's an angle I hadn't thought of before: what happens when #pragma clang attribute slams attributes onto declarations where the attribute isn't explicitly written.
Therefore, it is my preference/insistence that we need to white-list attributes that we know that don't alter the type. For now (as this patch/discussion has gotten large enough), it is sufficient and conservative if we just make the presence of ANY attributes on either declaration enough to make them not structurally equivalent.
I suspect this will be onerous for users in some regards given how ubiquitous attributes are, but it is defensible as an initial patch. Do you mean this to be just for declaration attributes, or type attributes as well? e.g.,
struct S {
int * const bar;
int * _Nullable Foo;
};
struct S {
int * const bar; // Fine
int * _Nullable Foo; // Fine, just like other qualifiers
};
There was a problem hiding this comment.
At the moment I'd probably want to have NO attributes including declaration attributes, but am OK white-listing certain ones (like _Nullable, which are always "equal"). But the same problems I mentioned above are present for both type and declaration attributes.
THOUGH type attributes that modify the type then don't appear in the AST (that is, don't become an AttributedType and instead just create a type ala-vector-type attributes) are almost definitely fine, since we have equivalent defined for them already.
| CopyEnumerators(D1->enumerators(), D1Enums); | ||
| CopyEnumerators(D2->enumerators(), D2Enums); | ||
|
|
||
| // In C23 mode, the order of the enumerations does not matter, so sort them |
There was a problem hiding this comment.
I have to say that is a choice.
The only defensible way forward IMO is (5) today. We can use (4) to get us to (3) (and probably should prioritize standard attributes FIRST). As you mentioned (2) can be really bad, but (1) has all the same problems, with a slightly different/more nuanced attack surface. (3) is a great place to be, but is A LOT of work at least for now. (4) is more work, but I think can be a followup. SO I think: (5) for now, then use (4) to get to (3) plus additional attributes we have done case-by-case. |
If either tag type has an attribute or has a member with an attribute, including keywords that are implemented via an attribute like _Alignas, then the types are treated as incompatible. A warning which defaults to an error is emitted; if the user downgrades the error back to a warning or disables the warning entirely, then it's undefined behavior and the user gets what they get. Eventually we will implement tablegen logic so attributes can be handled generically via information encoded in Attr.td.
|
I spoke with @erichkeane about this offline and ended up going with his recommended approach: any attributes cause the types to not be compatible, but instead of issuing a hard error, we issue a warning which defaults to an error. Users are free to disable the warning entirely (or downgrade it from an error) and they'll get what they get in terms of compatibility (we treat it as UB for now). |
We talked about this at the Clang C and C++ Language Working Group meeting and I think the resolution we came to was:
So the changes needed to move this patch forward are to only perform the attribute check when we're checking for a valid redefinition, not type compatibility in general. A follow-up to improve upon this can do the layout check when looking for compatibility. Alternatively (or as another follow-up), we can implement the case-by-case basis logic. Does that match your understanding @jyknight? |
these changes.
The diagnostic has always appeared on Windows and never on Linux, and that hasn't changed with this patch.
|
Ping |
Ping on this @jyknight |
|
I did some initial testing by compiling one of my projects and so far this worked fine. |
Another ping (if no response by mid-week, I'll probably land the changes as-is and address concerns post-commit). |
|
So one usability issue I noticed that we also had in GCC is that the warning about declaring structs in function prototypes (-Wvisibility) does not make sense anymore and becomes annoying. We turned it off in C23 for complete (but not for incomplete) structs) without tag. |
Good to know! On the one hand, the type is not visible, but on the other hand, it can be compatible so you can call the function now I suppose. I had seen that and left the diagnostic on just so users didn't confuse these two situations: but that doesn't necessarily seem worth leaving the diagnostic on for. |
|
| if (D2->hasAttrs()) | ||
| D2Attr = *D2->getAttrs().begin(); | ||
| if (D1Attr || D2Attr) { | ||
| const auto *DiagnoseDecl = cast<TypeDecl>(PrimaryDecl ? PrimaryDecl : D2); |
There was a problem hiding this comment.
@AaronBallman don't we have to guard this piece of code with if (Context.Complain)?
I don't have a small reproducer yet, but the assertion in StructuralEquivalenceContext::Diag2 (assert(Complain && "Not allowed to complain");) is failing when I build an internal project.
There was a problem hiding this comment.
Looks like this issue has been discussed here: #153916 (comment)
There was a problem hiding this comment.
For some more context, we've been qualifying the 21.x branch and discovered that the series of patches related to implementing WG14 N3037, has broken several Objective-C projects with gnu23 set. Building with clang modules is a common configuration that seems to have been broken here by these structural equivalence checks.
These are the relevant commits we found that represent the regression. It is basically the set of commits I needed to be able to revert cleanly.
460ff1eb0ef10a599b96a5a7fa1f4bd3973de4c7 - [C23] AST equivalence of attributes (#151196)
3d6fb12dfcef05407439580491430c376489d6a5 - [C23] More improved type compatibility for enumerations (#150946)
3b53c84e338bfb870f49e6a520725d86ff40c375 - C23] Handle type compatibility for enumerations better (#150282)
6769a836e97d5a09b239f78a8fd63cf4dac1fe13 - [C23] Handle type compatibility of unnamed records (#141783)
ee3610efb511ec0a8d5ebd2016ce23b4b4344cd1 - Remove unused function; NFC
a4ceac7e3e04c32cb3e334eb89b54d8e99a298f8 - [C23] Implement WG14 N3037 (#132939)
Akira has a patch to simply account for Context.Complain when diagnosing violations, but that further exposes an issue with modular builds. I've attached a reproducer that contains a set of files & a modulemap to demonstrate the issue. run.sh just invokes clang; you can change the path to point to what is correct on your system and assumes you are in the repro directory.
The reproduced case basically is that when we are compiling a TU (client.m) that itself is a part of the same project that vends a module. The compiler accepts an option -fmodule-name=Foo to let the compiler know this. When this happens, headers from that module are treated textually, but if those same headers are resolved transitively through a different module, that would be respected as a modular header & thus Foo.pcm is created and also contains the same declarations. This typically would result in a redeclaration issue, but the compiler explicitly supports this case through type merging. It seems like for the unnamed struct that is in ivar, the structural equivalence check now fails, but previously didn't, due to
!D1->getIdentifier() which is the case for unnamed structs.
We do heavily rely on type merging support & this configuration is working. @AaronBallman could you please take a look? Apologies in advance for the delay in reporting the observed breakages.
cc: @vsapsai
There was a problem hiding this comment.
Aaron has been out for a bit but should be back next week. He will have a large backlog of stuff to catch up to.
There was a problem hiding this comment.
Looks like this issue has been discussed here: #153916 (comment)
Yeah, as mentioned in my comment as well as Cyndy's, there's a bigger issue with serialization that I've not been able to resolve; I may need help from @Bigcheese @ChuanqiXu9 as serialization experts. It's not trivial to fix, at least from what I was trying before sabbatical. I'll try to get back on this one, but it might be next week before I have the bandwidth.
There was a problem hiding this comment.
Thanks for the update, Aaron. @vsapsai has also been investigating the impact on Objective-C and modules & is working on a mitigation.
There was a problem hiding this comment.
My investigation is about unnamed records and it is more of a follow-up to #141783 This problem is not related to attributes, so I'll create a separate issue to discuss that.
There was a problem hiding this comment.
As promised, a separate issue #162933
This changes the type compatibility rules so that it is permitted to redefine tag types within the same TU so long as they are equivalent definitions.
It is intentionally not being exposed as an extension in older C language modes. GCC does not do so and the feature doesn't seem compelling enough to warrant it.