[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.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@252063 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 08337e88c21db7bac6c910801ad3d6e0f9a15566..de623ca9b035c005e7415a83a4780be53095f775 100644 (file)
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2986,9 +2986,6 @@ def note_ovl_candidate : Note<"candidate "
     "%select{none|const|restrict|const and restrict|volatile|const and volatile"
     "|volatile and restrict|const, volatile, and restrict}4)"
     "| made ineligible by enable_if}2">;
-def ext_ovl_equivalent_internal_linkage_functions_in_modules : ExtWarn<
-  "ambiguous use of internal linkage function %0 defined in multiple modules">,
-  InGroup<DiagGroup<"modules-ambiguous-internal-linkage-function">>;
 
 def note_ovl_candidate_inherited_constructor : Note<"inherited from here">;
 def note_ovl_candidate_illegal_constructor : Note<
@@ -7869,6 +7866,12 @@ def err_module_self_import : Error<
   "import of module '%0' appears within same top-level module '%1'">;
 def err_module_import_in_implementation : Error<
   "@import of module '%0' in implementation of '%1'; use #import">;
+
+def ext_equivalent_internal_linkage_decl_in_modules : ExtWarn<
+  "ambiguous use of internal linkage declaration %0 defined in multiple modules">,
+  InGroup<DiagGroup<"modules-ambiguous-internal-linkage">>;
+def note_equivalent_internal_linkage_decl : Note<
+  "declared here%select{ in module '%1'|}0">;
 }
 
 let CategoryName = "Coroutines Issue" in {

diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
index 78dfe59392ab9aaf31ab7cef149e364ebd392566..39b7d8fcb5957d841e95db9a722b87e9682d3dff 100644 (file)
--- a/include/clang/Sema/Sema.h
+++ b/include/clang/Sema/Sema.h
@@ -1386,6 +1386,15 @@ public:
   hasVisibleDefaultArgument(const NamedDecl *D,
                             llvm::SmallVectorImpl<Module *> *Modules = nullptr);
 
+  /// Determine if \p A and \p B are equivalent internal linkage declarations
+  /// from different modules, and thus an ambiguity error can be downgraded to
+  /// an extension warning.
+  bool isEquivalentInternalLinkageDeclaration(const NamedDecl *A,
+                                              const NamedDecl *B);
+  void diagnoseEquivalentInternalLinkageDeclarations(
+      SourceLocation Loc, const NamedDecl *D,
+      ArrayRef<const NamedDecl *> Equiv);
+
   bool RequireCompleteType(SourceLocation Loc, QualType T,
                            TypeDiagnoser &Diagnoser);
   bool RequireCompleteType(SourceLocation Loc, QualType T,

diff --git a/lib/Sema/SemaLookup.cpp b/lib/Sema/SemaLookup.cpp
index 66cfada9f43431fee4bfccde5bf255f2f482b303..a020dcf14d698dd62ab9e4ecb7012013f7bb7ba8 100644 (file)
--- a/lib/Sema/SemaLookup.cpp
+++ b/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/lib/Sema/SemaOverload.cpp b/lib/Sema/SemaOverload.cpp
index 52c0beac9ca69b8f00a63fab8a102b8a2dc1cbce..8ce3c21c3d33d98aa4dc46544a7a598e1fb40d11 100644 (file)
--- a/lib/Sema/SemaOverload.cpp
+++ b/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;
 }

diff --git a/test/Modules/Inputs/libstdcxx-ambiguous-internal/a.h b/test/Modules/Inputs/libstdcxx-ambiguous-internal/a.h
index e079bd0908eab585a9f0694cd1de80a1e63f943c..0971369913333835fd81efc1d1107582e7c82a6a 100644 (file)
--- a/test/Modules/Inputs/libstdcxx-ambiguous-internal/a.h
+++ b/test/Modules/Inputs/libstdcxx-ambiguous-internal/a.h
@@ -1,4 +1,5 @@
 #ifndef A_H
 #define A_H
 static inline void f() {}
+constexpr int n = 0;
 #endif

diff --git a/test/Modules/Inputs/libstdcxx-ambiguous-internal/d.h b/test/Modules/Inputs/libstdcxx-ambiguous-internal/d.h
index 1627bfb99e231636fb50796a157711f0d3f69455..efec99f003d8c72e4f22ffb69fb2f6f4cd2422ed 100644 (file)
--- a/test/Modules/Inputs/libstdcxx-ambiguous-internal/d.h
+++ b/test/Modules/Inputs/libstdcxx-ambiguous-internal/d.h
@@ -1,3 +1,4 @@
 #include "b.h"
 #include "c.h"
 inline void g() { f(); }
+inline int h() { return n; }

diff --git a/test/Modules/libstdcxx-ambiguous-internal.cpp b/test/Modules/libstdcxx-ambiguous-internal.cpp
index 966b9c9768188de870c8b503364d504b62cb0f8d..784b442eab468f97c1cb1f3c2d01ef6f200a4938 100644 (file)
--- a/test/Modules/libstdcxx-ambiguous-internal.cpp
+++ b/test/Modules/libstdcxx-ambiguous-internal.cpp
@@ -3,5 +3,11 @@
 // RUN: %clang_cc1 -x c++ -std=c++11 -fmodules -emit-module -fmodule-name=std -fmodules-cache-path=%t %S/Inputs/libstdcxx-ambiguous-internal/module.modulemap -fmodules-local-submodule-visibility -DAMBIGUOUS 2>&1| FileCheck %s
 
 // CHECK-NOT: error
-// CHECK: warning: ambiguous use of internal linkage function 'f' defined in multiple modules
+// CHECK: warning: ambiguous use of internal linkage declaration 'f' defined in multiple modules
+// CHECK: note: declared here in module 'std.C'
+// CHECK: note: declared here in module 'std.B'
+// CHECK-NOT: error
+// CHECK: warning: ambiguous use of internal linkage declaration 'n' defined in multiple modules
+// CHECK: note: declared here in module 'std.C'
+// CHECK: note: declared here in module 'std.B'
 // CHECK-NOT: error