[ASTReader] Treat multiple defns of ObjC protocols the same as interf…

…aces.

Summary:
In change 2ba19793512, the ASTReader logic for ObjC interfaces was modified to
preserve the first definition-data read, "merging" later definitions into it
rather than overwriting it (though this "merging" is, in practice, a no-op that
discards the later definition-data).

Unfortunately this change was only made to ObjC interfaces, not protocols; this
means that when (for example) loading a protocol that references an interface,
if both the protocol and interface are multiply defined (as can easily happen
if the same header is read from multiple contexts), an _inconsistent_ pair of
definitions is loaded: first-read for the interface and last-read for the
protocol.

This in turn causes very subtle downstream bugs in the Swift ClangImporter,
which filters the results of name lookups based on the owning module of a
definition; inconsistency between a pair of related definitions causes name
lookup failures at various stages of compilation.

To fix these downstream issues, this change replicates the logic applied to
interfaces in change 2ba19793512, but for ObjC protocols.

rdar://30851899

Reviewers: doug.gregor, rsmith

Reviewed By: doug.gregor

Subscribers: jordan_rose, cfe-commits

Differential Revision: https://reviews.llvm.org/D34741

llvm-svn: 306583

clang/include/clang/AST/Redeclarable.h

@@ -21,6 +21,60 @@
 namespace clang {
 class ASTContext;
 
+// Some notes on redeclarables:
+//
+//  - Every redeclarable is on a circular linked list.
+//
+//  - Every decl has a pointer to the first element of the chain _and_ a
+//    DeclLink that may point to one of 3 possible states:
+//      - the "previous" (temporal) element in the chain
+//      - the "latest" (temporal) element in the chain
+//      - the an "uninitialized-latest" value (when newly-constructed)
+//
+//  - The first element is also often called the canonical element. Every
+//    element has a pointer to it so that "getCanonical" can be fast.
+//
+//  - Most links in the chain point to previous, except the link out of
+//    the first; it points to latest.
+//
+//  - Elements are called "first", "previous", "latest" or
+//    "most-recent" when referring to temporal order: order of addition
+//    to the chain.
+//
+//  - To make matters confusing, the DeclLink type uses the term "next"
+//    for its pointer-storage internally (thus functions like
+//    NextIsPrevious). It's easiest to just ignore the implementation of
+//    DeclLink when making sense of the redeclaration chain.
+//
+//  - There's also a "definition" link for several types of
+//    redeclarable, where only one definition should exist at any given
+//    time (and the defn pointer is stored in the decl's "data" which
+//    is copied to every element on the chain when it's changed).
+//
+//    Here is some ASCII art:
+//
+//      "first"                                     "latest"
+//      "canonical"                                 "most recent"
+//      +------------+         first                +--------------+
+//      |            | <--------------------------- |              |
+//      |            |                              |              |
+//      |            |                              |              |
+//      |            |       +--------------+       |              |
+//      |            | first |              |       |              |
+//      |            | <---- |              |       |              |
+//      |            |       |              |       |              |
+//      | @class A   |  link | @interface A |  link | @class A     |
+//      | seen first | <---- | seen second  | <---- | seen third   |
+//      |            |       |              |       |              |
+//      +------------+       +--------------+       +--------------+
+//      | data       | defn  | data         |  defn | data         |
+//      |            | ----> |              | <---- |              |
+//      +------------+       +--------------+       +--------------+
+//        |                     |     ^                  ^
+//        |                     |defn |                  |
+//        | link                +-----+                  |
+//        +-->-------------------------------------------+
+
 /// \brief Provides common interface for the Decls that can be redeclared.
 template<typename decl_type>
 class Redeclarable {

clang/lib/Serialization/ASTReaderDecl.cpp

@@ -126,6 +126,9 @@ namespace clang {
     void ReadObjCDefinitionData(struct ObjCInterfaceDecl::DefinitionData &Data);
     void MergeDefinitionData(ObjCInterfaceDecl *D,
                              struct ObjCInterfaceDecl::DefinitionData &&NewDD);
+    void ReadObjCDefinitionData(struct ObjCProtocolDecl::DefinitionData &Data);
+    void MergeDefinitionData(ObjCProtocolDecl *D,
+                             struct ObjCProtocolDecl::DefinitionData &&NewDD);
 
     static NamedDecl *getAnonymousDeclForMerging(ASTReader &Reader,
                                                  DeclContext *DC,
@@ -1045,18 +1048,8 @@ void ASTDeclReader::VisitObjCIvarDecl(ObjCIvarDecl *IVD) {
   IVD->setSynthesize(synth);
 }
 
-void ASTDeclReader::VisitObjCProtocolDecl(ObjCProtocolDecl *PD) {
-  RedeclarableResult Redecl = VisitRedeclarable(PD);
-  VisitObjCContainerDecl(PD);
-  mergeRedeclarable(PD, Redecl);
-
-  if (Record.readInt()) {
-    // Read the definition.
-    PD->allocateDefinitionData();
-
-    // Set the definition data of the canonical declaration, so other
-    // redeclarations will see it.
-    PD->getCanonicalDecl()->Data = PD->Data;
+void ASTDeclReader::ReadObjCDefinitionData(
+         struct ObjCProtocolDecl::DefinitionData &Data) {
 
     unsigned NumProtoRefs = Record.readInt();
     SmallVector<ObjCProtocolDecl *, 16> ProtoRefs;
@@ -1067,9 +1060,37 @@ void ASTDeclReader::VisitObjCProtocolDecl(ObjCProtocolDecl *PD) {
     ProtoLocs.reserve(NumProtoRefs);
     for (unsigned I = 0; I != NumProtoRefs; ++I)
       ProtoLocs.push_back(ReadSourceLocation());
-    PD->setProtocolList(ProtoRefs.data(), NumProtoRefs, ProtoLocs.data(),
-                        Reader.getContext());
+    Data.ReferencedProtocols.set(ProtoRefs.data(), NumProtoRefs,
+                                 ProtoLocs.data(), Reader.getContext());
+}
+
+void ASTDeclReader::MergeDefinitionData(ObjCProtocolDecl *D,
+         struct ObjCProtocolDecl::DefinitionData &&NewDD) {
+  // FIXME: odr checking?
+}
 
+void ASTDeclReader::VisitObjCProtocolDecl(ObjCProtocolDecl *PD) {
+  RedeclarableResult Redecl = VisitRedeclarable(PD);
+  VisitObjCContainerDecl(PD);
+  mergeRedeclarable(PD, Redecl);
+
+  if (Record.readInt()) {
+    // Read the definition.
+    PD->allocateDefinitionData();
+
+    ReadObjCDefinitionData(PD->data());
+
+    ObjCProtocolDecl *Canon = PD->getCanonicalDecl();
+    if (Canon->Data.getPointer()) {
+      // If we already have a definition, keep the definition invariant and
+      // merge the data.
+      MergeDefinitionData(Canon, std::move(PD->data()));
+      PD->Data = Canon->Data;
+    } else {
+      // Set the definition data of the canonical declaration, so other
+      // redeclarations will see it.
+      PD->getCanonicalDecl()->Data = PD->Data;
+    }
     // Note that we have deserialized a definition.
     Reader.PendingDefinitions.insert(PD);
   } else {