[ASTReader] Treat multiple defns of ObjC protocols the same as interfaces.

[graydon]

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

[doug.gregor]

Looks great!

include/clang/AST/Redeclarable.h

 
 #include "clang/AST/ExternalASTSource.h"
 #include "llvm/Support/Casting.h"
 #include <iterator>
 
 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 {
 protected:
   class DeclLink {
     /// A pointer to a known latest declaration, either statically known or
     /// generationally updated as decls are added by an external source.
     typedef LazyGenerationalUpdatePtr<const Decl*, Decl*,

lib/Serialization/ASTReaderDecl.cpp

class ASTDeclReader : public DeclVisitor<ASTDeclReader, void> {
     void ReadCXXRecordDefinition(CXXRecordDecl *D, bool Update);
     void ReadCXXDefinitionData(struct CXXRecordDecl::DefinitionData &Data,
                                const CXXRecordDecl *D);
     void MergeDefinitionData(CXXRecordDecl *D,
                              struct CXXRecordDecl::DefinitionData &&NewDD);
     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,
                                                  unsigned Index);
     static void setAnonymousDeclForMerging(ASTReader &Reader, DeclContext *DC,
                                            unsigned Index, NamedDecl *D);
 
     /// Results from loading a RedeclarableDecl.
void ASTDeclReader::VisitObjCIvarDecl(ObjCIvarDecl *IVD) {
   VisitFieldDecl(IVD);
   IVD->setAccessControl((ObjCIvarDecl::AccessControl)Record.readInt());
   // This field will be built lazily.
   IVD->setNextIvar(nullptr);
   bool synth = Record.readInt();
   IVD->setSynthesize(synth);
 }
 
+void ASTDeclReader::ReadObjCDefinitionData(
+         struct ObjCProtocolDecl::DefinitionData &Data) {
+
+    unsigned NumProtoRefs = Record.readInt();
+    SmallVector<ObjCProtocolDecl *, 16> ProtoRefs;
+    ProtoRefs.reserve(NumProtoRefs);
+    for (unsigned I = 0; I != NumProtoRefs; ++I)
+      ProtoRefs.push_back(ReadDeclAs<ObjCProtocolDecl>());
+    SmallVector<SourceLocation, 16> ProtoLocs;
+    ProtoLocs.reserve(NumProtoRefs);
+    for (unsigned I = 0; I != NumProtoRefs; ++I)
+      ProtoLocs.push_back(ReadSourceLocation());
+    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;
-
-    unsigned NumProtoRefs = Record.readInt();
-    SmallVector<ObjCProtocolDecl *, 16> ProtoRefs;
-    ProtoRefs.reserve(NumProtoRefs);
-    for (unsigned I = 0; I != NumProtoRefs; ++I)
-      ProtoRefs.push_back(ReadDeclAs<ObjCProtocolDecl>());
-    SmallVector<SourceLocation, 16> ProtoLocs;
-    ProtoLocs.reserve(NumProtoRefs);
-    for (unsigned I = 0; I != NumProtoRefs; ++I)
-      ProtoLocs.push_back(ReadSourceLocation());
-    PD->setProtocolList(ProtoRefs.data(), NumProtoRefs, ProtoLocs.data(),
-                        Reader.getContext());
-
+    }
     // Note that we have deserialized a definition.
     Reader.PendingDefinitions.insert(PD);
   } else {
     PD->Data = PD->getCanonicalDecl()->Data;
   }
 }
 
 void ASTDeclReader::VisitObjCAtDefsFieldDecl(ObjCAtDefsFieldDecl *FD) {