[modules] Generalize the workaround for multiple ambiguous definitions of

internal linkage entities in different modules from r250884 to apply to all
names, not just function names.

This is really awkward: we don't want to merge internal-linkage symbols from
separate modules, because they might not actually be defining the same entity.
But we don't want to reject programs that use such an ambiguous symbol if those
internal-linkage symbols are in fact equivalent. For now, we're resolving the
ambiguity by picking one of the equivalent definitions as an extension.

llvm-svn: 252063

2 files changed, 63 insertions, 29 deletions

diff --git a/clang/lib/Sema/SemaLookup.cpp b/clang/lib/Sema/SemaLookup.cpp
index 66cfada9f43..a020dcf14d6 100644
--- a/clang/lib/Sema/SemaLookup.cpp
+++ b/clang/lib/Sema/SemaLookup.cpp
@@ -463,8 +463,11 @@ void LookupResult::resolveKind() {
   llvm::SmallDenseMap<QualType, unsigned, 16> UniqueTypes;
 
   bool Ambiguous = false;
-  bool HasTag = false, HasFunction = false, HasNonFunction = false;
+  bool HasTag = false, HasFunction = false;
   bool HasFunctionTemplate = false, HasUnresolved = false;
+  NamedDecl *HasNonFunction = nullptr;
+
+  llvm::SmallVector<NamedDecl*, 4> EquivalentNonFunctions;
 
   unsigned UniqueTagIndex = 0;
 
@@ -533,9 +536,21 @@ void LookupResult::resolveKind() {
     } else if (isa<FunctionDecl>(D)) {
       HasFunction = true;
     } else {
-      if (HasNonFunction)
+      if (HasNonFunction) {
+        // If we're about to create an ambiguity between two declarations that
+        // are equivalent, but one is an internal linkage declaration from one
+        // module and the other is an internal linkage declaration from another
+        // module, just skip it.
+        if (getSema().isEquivalentInternalLinkageDeclaration(HasNonFunction,
+                                                             D)) {
+          EquivalentNonFunctions.push_back(D);
+          Decls[I] = Decls[--N];
+          continue;
+        }
+
         Ambiguous = true;
-      HasNonFunction = true;
+      }
+      HasNonFunction = D;
     }
     I++;
   }
@@ -558,6 +573,12 @@ void LookupResult::resolveKind() {
       Ambiguous = true;
   }
 
+  // FIXME: This diagnostic should really be delayed until we're done with
+  // the lookup result, in case the ambiguity is resolved by the caller.
+  if (!EquivalentNonFunctions.empty() && !Ambiguous)
+    getSema().diagnoseEquivalentInternalLinkageDeclarations(
+        getNameLoc(), HasNonFunction, EquivalentNonFunctions);
+
   Decls.set_size(N);
 
   if (HasNonFunction && (HasFunction || HasUnresolved))
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 52c0beac9ca..8ce3c21c3d3 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -8575,22 +8575,38 @@ bool clang::isBetterOverloadCandidate(Sema &S, const OverloadCandidate &Cand1,
   return false;
 }
 
-/// Determine whether two function declarations are "equivalent" for overload
-/// resolution purposes. This applies when the same internal linkage function
-/// is defined by two modules (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++).
-static bool isEquivalentCompatibleOverload(Sema &S,
-                                           const OverloadCandidate &Best,
-                                           const OverloadCandidate &Cand) {
-  return Best.Function && Cand.Function &&
-         Best.Function->getDeclContext()->getRedeclContext()->Equals(
-             Cand.Function->getDeclContext()->getRedeclContext()) &&
-         S.getOwningModule(Best.Function) != S.getOwningModule(Cand.Function) &&
-         !Best.Function->isExternallyVisible() &&
-         !Cand.Function->isExternallyVisible() &&
-         !S.IsOverload(Best.Function, Cand.Function, /*UsingDecl*/false);
+/// 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
+/// 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<ValueDecl>(A) && isa<ValueDecl>(B) &&
+         A->getDeclContext()->getRedeclContext()->Equals(
+             B->getDeclContext()->getRedeclContext()) &&
+         getOwningModule(const_cast<NamedDecl *>(A)) !=
+             getOwningModule(const_cast<NamedDecl *>(B)) &&
+         !A->isExternallyVisible() && !B->isExternallyVisible() &&
+         Context.hasSameType(cast<ValueDecl>(A)->getType(),
+                             cast<ValueDecl>(B)->getType());
+}
+
+void Sema::diagnoseEquivalentInternalLinkageDeclarations(
+    SourceLocation Loc, const NamedDecl *D, ArrayRef<const NamedDecl *> Equiv) {
+  Diag(Loc, diag::ext_equivalent_internal_linkage_decl_in_modules) << D;
+
+  Module *M = getOwningModule(const_cast<NamedDecl*>(D));
+  Diag(D->getLocation(), diag::note_equivalent_internal_linkage_decl)
+      << !M << (M ? M->getFullModuleName() : "");
+
+  for (auto *E : Equiv) {
+    Module *M = getOwningModule(const_cast<NamedDecl*>(E));
+    Diag(E->getLocation(), diag::note_equivalent_internal_linkage_decl)
+        << !M << (M ? M->getFullModuleName() : "");
+  }
 }
 
 static void NoteFunctionCandidate(Sema &S, OverloadCandidate *Cand,
@@ -8623,7 +8639,7 @@ OverloadCandidateSet::BestViableFunction(Sema &S, SourceLocation Loc,
   if (Best == end())
     return OR_No_Viable_Function;
 
-  llvm::SmallVector<const OverloadCandidate *, 4> EquivalentCands;
+  llvm::SmallVector<const NamedDecl *, 4> EquivalentCands;
 
   // Make sure that this function is better than every other viable
   // function. If not, we have an ambiguity.
@@ -8632,8 +8648,9 @@ OverloadCandidateSet::BestViableFunction(Sema &S, SourceLocation Loc,
         Cand != Best &&
         !isBetterOverloadCandidate(S, *Best, *Cand, Loc,
                                    UserDefinedConversion)) {
-      if (isEquivalentCompatibleOverload(S, *Best, *Cand)) {
-        EquivalentCands.push_back(Cand);
+      if (S.isEquivalentInternalLinkageDeclaration(Best->Function,
+                                                   Cand->Function)) {
+        EquivalentCands.push_back(Cand->Function);
         continue;
       }
 
@@ -8648,13 +8665,9 @@ OverloadCandidateSet::BestViableFunction(Sema &S, SourceLocation Loc,
        S.isFunctionConsideredUnavailable(Best->Function)))
     return OR_Deleted;
 
-  if (!EquivalentCands.empty()) {
-    S.Diag(Loc, diag::ext_ovl_equivalent_internal_linkage_functions_in_modules)
-      << Best->Function;
-    S.NoteOverloadCandidate(Best->Function);
-    for (auto *Cand : EquivalentCands)
-      S.NoteOverloadCandidate(Cand->Function);
-  }
+  if (!EquivalentCands.empty())
+    S.diagnoseEquivalentInternalLinkageDeclarations(Loc, Best->Function,
+                                                    EquivalentCands);
 
   return OR_Success;
 }