Basic ODR checking for C++ modules:

If we have multiple definitions of the same entity from different modules, we
nominate the first definition which we see as being the canonical definition.
If we load a declaration from a different definition and we can't find a
corresponding declaration in the canonical definition, issue a diagnostic.

This is insufficient to prevent things from going horribly wrong in all cases
-- we might be in the middle of emitting IR for a function when we trigger some
deserialization and discover that it refers to an incoherent piece of the AST,
by which point it's probably too late to bail out -- but we'll at least produce
a diagnostic.

llvm-svn: 192950

8 files changed, 137 insertions, 4 deletions

diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
index 9f630ae5c1c..81509cc1882 100644
--- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
@@ -67,4 +67,13 @@ def err_pch_pp_detailed_record : Error<
 
 def err_not_a_pch_file : Error<
     "'%0' does not appear to be a precompiled header file">, DefaultFatal;
+
+def err_module_odr_violation_missing_decl : Error<
+  "%q0 from module '%1' is not present in definition of %q2"
+  "%select{ in module '%4'| provided earlier}3">, NoSFINAE;
+def note_module_odr_violation_no_possible_decls : Note<
+  "definition has no member %0">;
+def note_module_odr_violation_possible_decl : Note<
+  "declaration of %0 does not match">;
+
 }
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index 530c54e3ccc..00528cc387f 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -875,6 +875,14 @@ private:
   /// been completed.
   std::deque<PendingDeclContextInfo> PendingDeclContextInfos;
 
+  /// \brief The set of NamedDecls that have been loaded, but are members of a
+  /// context that has been merged into another context where the corresponding
+  /// declaration is either missing or has not yet been loaded.
+  ///
+  /// We will check whether the corresponding declaration is in fact missing
+  /// once recursing loading has been completed.
+  llvm::SmallVector<NamedDecl *, 16> PendingOdrMergeChecks;
+
   /// \brief The set of Objective-C categories that have been deserialized
   /// since the last time the declaration chains were linked.
   llvm::SmallPtrSet<ObjCCategoryDecl *, 16> CategoriesDeserialized;
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 9bb9a7a8632..bafe74b174a 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -7337,7 +7337,8 @@ void ASTReader::ReadComments() {
 
 void ASTReader::finishPendingActions() {
   while (!PendingIdentifierInfos.empty() || !PendingDeclChains.empty() ||
-         !PendingMacroIDs.empty() || !PendingDeclContextInfos.empty()) {
+         !PendingMacroIDs.empty() || !PendingDeclContextInfos.empty() ||
+         !PendingOdrMergeChecks.empty()) {
     // If any identifiers with corresponding top-level declarations have
     // been loaded, load those declarations now.
     typedef llvm::DenseMap<IdentifierInfo *, SmallVector<Decl *, 2> >
@@ -7400,6 +7401,64 @@ void ASTReader::finishPendingActions() {
       DeclContext *LexicalDC = cast<DeclContext>(GetDecl(Info.LexicalDC));
       Info.D->setDeclContextsImpl(SemaDC, LexicalDC, getContext());
     }
+
+    // For each declaration from a merged context, check that the canonical
+    // definition of that context also contains a declaration of the same
+    // entity.
+    while (!PendingOdrMergeChecks.empty()) {
+      NamedDecl *D = PendingOdrMergeChecks.pop_back_val();
+
+      // FIXME: Skip over implicit declarations for now. This matters for things
+      // like implicitly-declared special member functions. This isn't entirely
+      // correct; we can end up with multiple unmerged declarations of the same
+      // implicit entity.
+      if (D->isImplicit())
+        continue;
+
+      DeclContext *CanonDef = D->getDeclContext();
+      DeclContext::lookup_result R = CanonDef->lookup(D->getDeclName());
+
+      bool Found = false;
+      const Decl *DCanon = D->getCanonicalDecl();
+
+      llvm::SmallVector<const NamedDecl*, 4> Candidates;
+      for (DeclContext::lookup_iterator I = R.begin(), E = R.end();
+           !Found && I != E; ++I) {
+        for (Decl::redecl_iterator RI = (*I)->redecls_begin(),
+                                   RE = (*I)->redecls_end();
+             RI != RE; ++RI) {
+          if ((*RI)->getLexicalDeclContext() == CanonDef) {
+            // This declaration is present in the canonical definition. If it's
+            // in the same redecl chain, it's the one we're looking for.
+            if ((*RI)->getCanonicalDecl() == DCanon)
+              Found = true;
+            else
+              Candidates.push_back(cast<NamedDecl>(*RI));
+            break;
+          }
+        }
+      }
+
+      if (!Found) {
+        D->setInvalidDecl();
+
+        Module *CanonDefModule = cast<Decl>(CanonDef)->getOwningModule();
+        Diag(D->getLocation(), diag::err_module_odr_violation_missing_decl)
+          << D << D->getOwningModule()->getFullModuleName()
+          << CanonDef << !CanonDefModule
+          << (CanonDefModule ? CanonDefModule->getFullModuleName() : "");
+
+        if (Candidates.empty())
+          Diag(cast<Decl>(CanonDef)->getLocation(),
+               diag::note_module_odr_violation_no_possible_decls) << D;
+        else {
+          for (unsigned I = 0, N = Candidates.size(); I != N; ++I)
+            Diag(Candidates[I]->getLocation(),
+                 diag::note_module_odr_violation_possible_decl)
+              << Candidates[I];
+        }
+      }
+    }
   }
   
   // If we deserialized any C++ or Objective-C class definitions, any
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 14379f31167..e68c7fc6f9f 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -2193,6 +2193,9 @@ ASTDeclReader::FindExistingResult ASTDeclReader::findExisting(NamedDecl *D) {
     return Result;
   }
 
+  // FIXME: Bail out for non-canonical declarations. We will have performed any
+  // necessary merging already.
+
   DeclContext *DC = D->getDeclContext()->getRedeclContext();
   if (DC->isTranslationUnit() && Reader.SemaObj) {
     IdentifierResolver &IdResolver = Reader.SemaObj->IdResolver;
@@ -2226,17 +2229,23 @@ ASTDeclReader::FindExistingResult ASTDeclReader::findExisting(NamedDecl *D) {
       if (isSameEntity(*I, D))
         return FindExistingResult(Reader, D, *I);
     }
-    return FindExistingResult(Reader, D, /*Existing=*/0);
   } else if (DeclContext *MergeDC = getPrimaryContextForMerging(DC)) {
     DeclContext::lookup_result R = MergeDC->noload_lookup(Name);
     for (DeclContext::lookup_iterator I = R.begin(), E = R.end(); I != E; ++I) {
       if (isSameEntity(*I, D))
         return FindExistingResult(Reader, D, *I);
     }
-    return FindExistingResult(Reader, D, /*Existing=*/0);
+  } else {
+    // Not in a mergeable context.
+    return FindExistingResult(Reader);
   }
 
-  return FindExistingResult(Reader);
+  // If this declaration is from a merged context, make a note that we need to
+  // check that the canonical definition of that context contains the decl.
+  if (Reader.MergedDeclContexts.count(D->getLexicalDeclContext()))
+    Reader.PendingOdrMergeChecks.push_back(D);
+
+  return FindExistingResult(Reader, D, /*Existing=*/0);
 }
 
 void ASTDeclReader::attachPreviousDecl(Decl *D, Decl *previous) {
diff --git a/clang/test/Modules/Inputs/odr/a.h b/clang/test/Modules/Inputs/odr/a.h
new file mode 100644
index 00000000000..26144b86e8d
--- /dev/null
+++ b/clang/test/Modules/Inputs/odr/a.h
@@ -0,0 +1,13 @@
+extern struct Y {
+  int n;
+  float f;
+} y1;
+enum E { e1 };
+
+struct X {
+  int n;
+} x1;
+
+int f() {
+  return y1.n + e1 + y1.f + x1.n;
+}
diff --git a/clang/test/Modules/Inputs/odr/b.h b/clang/test/Modules/Inputs/odr/b.h
new file mode 100644
index 00000000000..b4063979474
--- /dev/null
+++ b/clang/test/Modules/Inputs/odr/b.h
@@ -0,0 +1,9 @@
+struct Y {
+  int m;
+  double f;
+} y2;
+enum E { e2 };
+
+int g() {
+  return y2.m + e2 + y2.f;
+}
diff --git a/clang/test/Modules/Inputs/odr/module.map b/clang/test/Modules/Inputs/odr/module.map
new file mode 100644
index 00000000000..81f396342fa
--- /dev/null
+++ b/clang/test/Modules/Inputs/odr/module.map
@@ -0,0 +1,6 @@
+module a {
+  header "a.h"
+}
+module b {
+  header "b.h"
+}
diff --git a/clang/test/Modules/odr.cpp b/clang/test/Modules/odr.cpp
new file mode 100644
index 00000000000..5ab10d2ce41
--- /dev/null
+++ b/clang/test/Modules/odr.cpp
@@ -0,0 +1,20 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x objective-c++ -fmodules -fmodules-cache-path=%t -I %S/Inputs/odr %s -verify -std=c++11
+
+// expected-error@a.h:8 {{'X::n' from module 'a' is not present in definition of 'X' provided earlier}}
+struct X { // expected-note {{definition has no member 'n'}}
+};
+
+@import a;
+@import b;
+
+// Trigger the declarations from a and b to be imported.
+int x = f() + g();
+
+// expected-note@a.h:5 {{definition has no member 'e2'}}
+// expected-note@a.h:3 {{declaration of 'f' does not match}}
+// expected-note@a.h:1 {{definition has no member 'm'}}
+
+// expected-error@b.h:5 {{'E::e2' from module 'b' is not present in definition of 'E' in module 'a'}}
+// expected-error@b.h:3 {{'Y::f' from module 'b' is not present in definition of 'Y' in module 'a'}}
+// expected-error@b.h:2 {{'Y::m' from module 'b' is not present in definition of 'Y' in module 'a'}}