From d10ae9b34e251034cd8c0d217d926fbdf73b4d57 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Fri, 13 Nov 2015 03:52:13 +0000 Subject: [PATCH] [modules] Follow the C++ standard's rule for linkage of enumerators: they have the linkage of the enumeration. For enumerators of unnamed enumerations, extend the -Wmodules-ambiguous-internal-linkage extension to allow selecting an arbitrary enumerator (but only if they all have the same value, otherwise it's ambiguous). git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@253010 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Sema/Sema.h | 3 +- lib/AST/Decl.cpp | 5 ++- lib/AST/DeclBase.cpp | 17 +++++--- lib/Sema/SemaDecl.cpp | 59 ++++++++++++++++---------- lib/Sema/SemaOverload.cpp | 53 ++++++++++++++++++----- test/Index/linkage.c | 2 +- test/Modules/Inputs/no-linkage/decls.h | 1 - test/Modules/no-linkage.cpp | 4 -- test/Modules/submodules-merge-defs.cpp | 8 ++++ 9 files changed, 105 insertions(+), 47 deletions(-) diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index 469c181f10..7c2fa04989 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -2121,7 +2121,8 @@ public: void mergeDeclAttributes(NamedDecl *New, Decl *Old, AvailabilityMergeKind AMK = AMK_Redeclaration); - void MergeTypedefNameDecl(TypedefNameDecl *New, LookupResult &OldDecls); + void MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New, + LookupResult &OldDecls); bool MergeFunctionDecl(FunctionDecl *New, NamedDecl *&Old, Scope *S, bool MergeTypeWithOld); bool MergeCompatibleFunctionDecls(FunctionDecl *New, FunctionDecl *Old, diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp index 8c50af615c..3f6941ab72 100644 --- a/lib/AST/Decl.cpp +++ b/lib/AST/Decl.cpp @@ -1239,7 +1239,6 @@ static LinkageInfo computeLVForDecl(const NamedDecl *D, // Note that the name of a typedef, namespace alias, using declaration, // and so on are not the name of the corresponding type, namespace, or // declaration, so they do *not* have linkage. - case Decl::EnumConstant: // FIXME: This has linkage, but that's dumb. case Decl::ImplicitParam: case Decl::Label: case Decl::NamespaceAlias: @@ -1249,6 +1248,10 @@ static LinkageInfo computeLVForDecl(const NamedDecl *D, case Decl::UsingDirective: return LinkageInfo::none(); + case Decl::EnumConstant: + // C++ [basic.link]p4: an enumerator has the linkage of its enumeration. + return getLVForDecl(cast(D->getDeclContext()), computation); + case Decl::Typedef: case Decl::TypeAlias: // A typedef declaration has linkage if it gives a type a name for diff --git a/lib/AST/DeclBase.cpp b/lib/AST/DeclBase.cpp index fd6df024bb..16394e865e 100644 --- a/lib/AST/DeclBase.cpp +++ b/lib/AST/DeclBase.cpp @@ -1210,13 +1210,16 @@ void DeclContext::removeDecl(Decl *D) { // Remove only decls that have a name if (!ND->getDeclName()) return; - StoredDeclsMap *Map = getPrimaryContext()->LookupPtr; - if (!Map) return; - - StoredDeclsMap::iterator Pos = Map->find(ND->getDeclName()); - assert(Pos != Map->end() && "no lookup entry for decl"); - if (Pos->second.getAsVector() || Pos->second.getAsDecl() == ND) - Pos->second.remove(ND); + auto *DC = this; + do { + StoredDeclsMap *Map = DC->getPrimaryContext()->LookupPtr; + if (Map) { + StoredDeclsMap::iterator Pos = Map->find(ND->getDeclName()); + assert(Pos != Map->end() && "no lookup entry for decl"); + if (Pos->second.getAsVector() || Pos->second.getAsDecl() == ND) + Pos->second.remove(ND); + } + } while (DC->isTransparentContext() && (DC = DC->getParent())); } } diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp index a23d9debb8..e7c10d18a6 100644 --- a/lib/Sema/SemaDecl.cpp +++ b/lib/Sema/SemaDecl.cpp @@ -1880,7 +1880,8 @@ bool Sema::isIncompatibleTypedef(TypeDecl *Old, TypedefNameDecl *New) { /// how to resolve this situation, merging decls or emitting /// diagnostics as appropriate. If there was an error, set New to be invalid. /// -void Sema::MergeTypedefNameDecl(TypedefNameDecl *New, LookupResult &OldDecls) { +void Sema::MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New, + LookupResult &OldDecls) { // If the new decl is known invalid already, don't bother doing any // merging checks. if (New->isInvalidDecl()) return; @@ -1961,6 +1962,19 @@ void Sema::MergeTypedefNameDecl(TypedefNameDecl *New, LookupResult &OldDecls) { // Make the old tag definition visible. makeMergedDefinitionVisible(Hidden, NewTag->getLocation()); + + // If this was an unscoped enumeration, yank all of its enumerators + // out of the scope. + if (isa(NewTag)) { + Scope *EnumScope = getNonFieldDeclScope(S); + for (auto *D : NewTag->decls()) { + auto *ED = cast(D); + assert(EnumScope->isDeclScope(ED)); + EnumScope->RemoveDecl(ED); + IdResolver.RemoveDecl(ED); + ED->getLexicalDeclContext()->removeDecl(ED); + } + } } } @@ -5212,7 +5226,7 @@ Sema::ActOnTypedefNameDecl(Scope *S, DeclContext *DC, TypedefNameDecl *NewTD, filterNonConflictingPreviousTypedefDecls(*this, NewTD, Previous); if (!Previous.empty()) { Redeclaration = true; - MergeTypedefNameDecl(NewTD, Previous); + MergeTypedefNameDecl(S, NewTD, Previous); } // If this is the C FILE type, notify the AST context. @@ -13994,12 +14008,27 @@ Decl *Sema::ActOnEnumConstant(Scope *S, Decl *theEnumDecl, Decl *lastEnumConst, PrevDecl = nullptr; } + // C++ [class.mem]p15: + // If T is the name of a class, then each of the following shall have a name + // different from T: + // - every enumerator of every member of class T that is an unscoped + // enumerated type + if (!TheEnumDecl->isScoped()) + DiagnoseClassNameShadow(TheEnumDecl->getDeclContext(), + DeclarationNameInfo(Id, IdLoc)); + + EnumConstantDecl *New = + CheckEnumConstant(TheEnumDecl, LastEnumConst, IdLoc, Id, Val); + if (!New) + return nullptr; + if (PrevDecl) { // When in C++, we may get a TagDecl with the same name; in this case the // enum constant will 'hide' the tag. assert((getLangOpts().CPlusPlus || !isa(PrevDecl)) && "Received TagDecl when not in C++!"); - if (!isa(PrevDecl) && isDeclInScope(PrevDecl, CurContext, S)) { + if (!isa(PrevDecl) && isDeclInScope(PrevDecl, CurContext, S) && + shouldLinkPossiblyHiddenDecl(PrevDecl, New)) { if (isa(PrevDecl)) Diag(IdLoc, diag::err_redefinition_of_enumerator) << Id; else @@ -14009,26 +14038,12 @@ Decl *Sema::ActOnEnumConstant(Scope *S, Decl *theEnumDecl, Decl *lastEnumConst, } } - // C++ [class.mem]p15: - // If T is the name of a class, then each of the following shall have a name - // different from T: - // - every enumerator of every member of class T that is an unscoped - // enumerated type - if (!TheEnumDecl->isScoped()) - DiagnoseClassNameShadow(TheEnumDecl->getDeclContext(), - DeclarationNameInfo(Id, IdLoc)); - - EnumConstantDecl *New = - CheckEnumConstant(TheEnumDecl, LastEnumConst, IdLoc, Id, Val); - - if (New) { - // Process attributes. - if (Attr) ProcessDeclAttributeList(S, New, Attr); + // Process attributes. + if (Attr) ProcessDeclAttributeList(S, New, Attr); - // Register this decl in the current scope stack. - New->setAccess(TheEnumDecl->getAccess()); - PushOnScopeChains(New, S); - } + // Register this decl in the current scope stack. + New->setAccess(TheEnumDecl->getAccess()); + PushOnScopeChains(New, S); ActOnDocumentableDecl(New); diff --git a/lib/Sema/SemaOverload.cpp b/lib/Sema/SemaOverload.cpp index 8ce3c21c3d..4ac5a50281 100644 --- a/lib/Sema/SemaOverload.cpp +++ b/lib/Sema/SemaOverload.cpp @@ -8576,22 +8576,55 @@ bool clang::isBetterOverloadCandidate(Sema &S, const OverloadCandidate &Cand1, } /// Determine whether two declarations are "equivalent" for the purposes of -/// name lookup and overload resolution. This applies when the same internal -/// linkage variable or function is defined by two modules (textually including +/// name lookup and overload resolution. This applies when the same internal/no +/// linkage entity is defined by two modules (probably by textually including /// the same header). In such a case, we don't consider the declarations to /// declare the same entity, but we also don't want lookups with both /// declarations visible to be ambiguous in some cases (this happens when using /// a modularized libstdc++). bool Sema::isEquivalentInternalLinkageDeclaration(const NamedDecl *A, const NamedDecl *B) { - return A && B && isa(A) && isa(B) && - A->getDeclContext()->getRedeclContext()->Equals( - B->getDeclContext()->getRedeclContext()) && - getOwningModule(const_cast(A)) != - getOwningModule(const_cast(B)) && - !A->isExternallyVisible() && !B->isExternallyVisible() && - Context.hasSameType(cast(A)->getType(), - cast(B)->getType()); + auto *VA = dyn_cast_or_null(A); + auto *VB = dyn_cast_or_null(B); + if (!VA || !VB) + return false; + + // The declarations must be declaring the same name as an internal linkage + // entity in different modules. + if (!VA->getDeclContext()->getRedeclContext()->Equals( + VB->getDeclContext()->getRedeclContext()) || + getOwningModule(const_cast(VA)) == + getOwningModule(const_cast(VB)) || + VA->isExternallyVisible() || VB->isExternallyVisible()) + return false; + + // Check that the declarations appear to be equivalent. + // + // FIXME: Checking the type isn't really enough to resolve the ambiguity. + // For constants and functions, we should check the initializer or body is + // the same. For non-constant variables, we shouldn't allow it at all. + if (Context.hasSameType(VA->getType(), VB->getType())) + return true; + + // Enum constants within unnamed enumerations will have different types, but + // may still be similar enough to be interchangeable for our purposes. + if (auto *EA = dyn_cast(VA)) { + if (auto *EB = dyn_cast(VB)) { + // Only handle anonymous enums. If the enumerations were named and + // equivalent, they would have been merged to the same type. + auto *EnumA = cast(EA->getDeclContext()); + auto *EnumB = cast(EB->getDeclContext()); + if (EnumA->hasNameForLinkage() || EnumB->hasNameForLinkage() || + !Context.hasSameType(EnumA->getIntegerType(), + EnumB->getIntegerType())) + return false; + // Allow this only if the value is the same for both enumerators. + return llvm::APSInt::isSameValue(EA->getInitVal(), EB->getInitVal()); + } + } + + // Nothing else is sufficiently similar. + return false; } void Sema::diagnoseEquivalentInternalLinkageDeclarations( diff --git a/test/Index/linkage.c b/test/Index/linkage.c index b0dcb30990..ab006590b6 100644 --- a/test/Index/linkage.c +++ b/test/Index/linkage.c @@ -20,7 +20,7 @@ void f16(void) { // CHECK: EnumDecl=Baz:3:6 (Definition)linkage=External -// CHECK: EnumConstantDecl=Qux:3:12 (Definition)linkage=NoLinkage +// CHECK: EnumConstantDecl=Qux:3:12 (Definition)linkage=External // CHECK: VarDecl=x:4:5linkage=External // CHECK: FunctionDecl=foo:5:6linkage=External // CHECK: VarDecl=w:6:12linkage=Internal diff --git a/test/Modules/Inputs/no-linkage/decls.h b/test/Modules/Inputs/no-linkage/decls.h index 5c29044abf..9e18e10ce9 100644 --- a/test/Modules/Inputs/no-linkage/decls.h +++ b/test/Modules/Inputs/no-linkage/decls.h @@ -2,5 +2,4 @@ namespace RealNS { int UsingDecl; } namespace NS = RealNS; typedef int Typedef; using AliasDecl = int; -enum Enum { Enumerator }; using RealNS::UsingDecl; diff --git a/test/Modules/no-linkage.cpp b/test/Modules/no-linkage.cpp index 0cc808b633..d4a9eaba40 100644 --- a/test/Modules/no-linkage.cpp +++ b/test/Modules/no-linkage.cpp @@ -6,21 +6,18 @@ namespace NS { int n; } // expected-note {{candidate}} struct Typedef { int n; }; // expected-note {{candidate}} int AliasDecl; // expected-note {{candidate}} -enum AlsoAnEnum { Enumerator }; // expected-note {{candidate}} int UsingDecl; // expected-note {{candidate}} // expected-note@decls.h:2 {{candidate}} // expected-note@decls.h:3 {{candidate}} // expected-note@decls.h:4 {{candidate}} // expected-note@decls.h:5 {{candidate}} -// expected-note@decls.h:6 {{candidate}} void use(int); void use_things() { use(Typedef().n); use(NS::n); use(AliasDecl); - use(Enumerator); use(UsingDecl); } @@ -30,6 +27,5 @@ void use_things_again() { use(Typedef().n); // expected-error {{ambiguous}} use(NS::n); // expected-error {{ambiguous}} use(AliasDecl); // expected-error {{ambiguous}} - use(Enumerator); // expected-error {{ambiguous}} use(UsingDecl); // expected-error {{ambiguous}} } diff --git a/test/Modules/submodules-merge-defs.cpp b/test/Modules/submodules-merge-defs.cpp index 361f05b553..12e3cadd53 100644 --- a/test/Modules/submodules-merge-defs.cpp +++ b/test/Modules/submodules-merge-defs.cpp @@ -116,4 +116,12 @@ void use_static_inline() { StaticInline::g({}); } // expected-note@defs.h:71 {{declared here in module 'redef'}} // expected-note@defs.h:71 {{declared here in module 'stuff.use'}} #endif +int use_anon_enum = G::g; +#ifdef EARLY_INDIRECT_INCLUDE +// expected-warning@-2 3{{ambiguous use of internal linkage declaration 'g' defined in multiple modules}} +// FIXME: These notes are produced, but -verify can't match them? +// FIXME-note@defs.h:51 3{{declared here in module 'redef'}} +// FIXME-note@defs.h:51 3{{declared here in module 'stuff.use'}} +#endif +int use_named_enum = G::i; #endif -- 2.40.0