[ODRHash] Hash `ObjCProtocolDecl` and diagnose discovered mismatches.

[vsapsai]

Rebase.

[jansvoboda11]

Should we also update ODRHash::isDeclToBeProcessed?

clang/lib/AST/ODRDiagsEmitter.cpp

312

Are we okay with simply comparing protocol names here instead of the full declarations? I guess we're relying on the top-level diagnostic for those, and avoiding recursing, correct?

1659

This will fall-through and call llvm_unreachable, will it not? I assume that's not intended.

[vsapsai]

In D130324#3843934, @jansvoboda11 wrote:

Should we also update ODRHash::isDeclToBeProcessed?

No, as isDeclToBeProcessed is only for sub-decls and protocols cannot be nested inside other entities. Looks like nested protocols are rejected at the parsing stage※, so I won't test such case.

Your comment is valuable as it reveals bad naming and I plan to rename isDeclToBeProcessed to isSubDeclToBeProcessed in a separate commit (suggestions for alternative names are welcome).

Footnotes

※ - code like

@protocol Foo
@protocol Bar
@end
@end

is rejected with errors

test.m:2:1: error: illegal interface qualifier
@protocol Bar
^
test.m:3:2: error: unknown type name 'end'
@end
 ^
test.m:4:2: error: prefix attribute must be followed by an interface, protocol, or implementation
@end
 ^
test.m:4:5: error: missing '@end'
@end
    ^
test.m:1:1: note: protocol started here
@protocol Foo
^

clang/lib/AST/ODRDiagsEmitter.cpp

312

The main reason to compare just names is so that we don't fail with forward-declared protocols that don't have full declarations (covered by tests). And yes, we are relying on the top-level diagnostic to find mismatches in the referenced protocols.

Note that we have the same behavior in ODRHash::AddObjCProtocolDecl.

1659

Good question. Protocols cannot have fields or vars (properties are covered by D130326). Typedefs are top-level and not nested inside protocols, so that case cannot happen too. Other is covered by FirstDiffType == Other || SecondDiffType == Other and EndOfClass by FirstDiffType != SecondDiffType. So it is correct that in all of these cases we fall-through to llvm_unreachable.

[jansvoboda11]

Thanks for explaining. This LGTM, but you might want to get a second opinion.

clang/lib/AST/ODRDiagsEmitter.cpp

1659

That makes sense, thanks for the explanation. Can you a short summary in a comment?

[bruno]

Great to see both this and D130325 landing, LGTM too.

[vsapsai]

Thanks for the reviews!

clang/include/clang/AST/DeclObjC.h

 class ObjCProtocolDecl : public ObjCContainerDecl,
                          public Redeclarable<ObjCProtocolDecl> {
   struct DefinitionData {
     // The declaration that defines this protocol.
     ObjCProtocolDecl *Definition;
 
     /// Referenced protocols
     ObjCProtocolList ReferencedProtocols;
+
+    /// Tracks whether a ODR hash has been computed for this protocol.
+    unsigned HasODRHash : 1;
+
+    /// A hash of parts of the class to help in ODR checking.
+    unsigned ODRHash = 0;
   };
 
   /// Contains a pointer to the data associated with this class,
   /// which will be NULL if this class has not yet been defined.
   ///
   /// The bit indicates when we don't need to check for out-of-date
   /// declarations. It will be set unless modules are enabled.
   llvm::PointerIntPair<DefinitionData *, 1, bool> Data;
class ObjCProtocolDecl : public ObjCContainerDecl,
   ObjCProtocolDecl *getPreviousDeclImpl() override {
     return getPreviousDecl();
   }
 
   ObjCProtocolDecl *getMostRecentDeclImpl() override {
     return getMostRecentDecl();
   }
 
+  /// True if a valid hash is stored in ODRHash.
+  bool hasODRHash() const;
+  void setHasODRHash(bool HasHash);
+
 public:
   friend class ASTDeclReader;
   friend class ASTDeclWriter;
   friend class ASTReader;
+  friend class ODRDiagsEmitter;
 
   static ObjCProtocolDecl *Create(ASTContext &C, DeclContext *DC,
                                   IdentifierInfo *Id,
                                   SourceLocation nameLoc,
                                   SourceLocation atStartLoc,
                                   ObjCProtocolDecl *PrevDecl);
 
   static ObjCProtocolDecl *CreateDeserialized(ASTContext &C, unsigned ID);
public:
 
   void collectPropertiesToImplement(PropertyMap &PM,
                                     PropertyDeclOrder &PO) const override;
 
   void collectInheritedProtocolProperties(const ObjCPropertyDecl *Property,
                                           ProtocolPropertySet &PS,
                                           PropertyDeclOrder &PO) const;
 
+  /// Get precomputed ODRHash or add a new one.
+  unsigned getODRHash();
+
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) { return K == ObjCProtocol; }
 };
 
 /// ObjCCategoryDecl - Represents a category declaration. A category allows
 /// you to add methods to an existing class (without subclassing or modifying
 /// the original class interface or implementation:-). Categories don't allow
 /// you to add instance data. The following example adds "myMethod" to all

clang/include/clang/AST/ODRDiagsEmitter.h

 //===- ODRDiagsEmitter.h - Emits diagnostic for ODR mismatches --*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_AST_ODRDIAGSEMITTER_H
 #define LLVM_CLANG_AST_ODRDIAGSEMITTER_H
 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclObjC.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/LangOptions.h"
 
 namespace clang {
 
 class ODRDiagsEmitter {
 public:
   ODRDiagsEmitter(DiagnosticsEngine &Diags, const ASTContext &Context,
                   const LangOptions &LangOpts)
       : Diags(Diags), Context(Context), LangOpts(LangOpts) {}
 
   bool diagnoseMismatch(const FunctionDecl *FirstFunction,
                         const FunctionDecl *SecondFunction) const;
 
   bool diagnoseMismatch(const EnumDecl *FirstEnum,
                         const EnumDecl *SecondEnum) const;
 
   bool
   diagnoseMismatch(const CXXRecordDecl *FirstRecord,
                    const CXXRecordDecl *SecondRecord,
                    const struct CXXRecordDecl::DefinitionData *SecondDD) const;
 
+  /// Diagnose ODR mismatch between 2 ObjCProtocolDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  /// To compare 2 declarations with merged and identical definition data
+  /// you need to provide pre-merge definition data in \p SecondDD.
+  bool diagnoseMismatch(
+      const ObjCProtocolDecl *FirstProtocol,
+      const ObjCProtocolDecl *SecondProtocol,
+      const struct ObjCProtocolDecl::DefinitionData *SecondDD) const;
+
 private:
   using DeclHashes = llvm::SmallVector<std::pair<const Decl *, unsigned>, 4>;
 
   // Used with err_module_odr_violation_mismatch_decl and
   // note_module_odr_violation_mismatch_decl
   // This list should be the same Decl's as in ODRHash::isDeclToBeProcessed
   enum ODRMismatchDecl {
     EndOfClass,
bool diagnoseSubMismatchTypedef(const NamedDecl *FirstRecord,
                                   const TypedefNameDecl *SecondTD,
                                   bool IsTypeAlias) const;
 
   bool diagnoseSubMismatchVar(const NamedDecl *FirstRecord,
                               StringRef FirstModule, StringRef SecondModule,
                               const VarDecl *FirstVD,
                               const VarDecl *SecondVD) const;
 
+  /// Check if protocol lists are the same and diagnose if they are different.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  bool diagnoseSubMismatchProtocols(const ObjCProtocolList &FirstProtocols,
+                                    const ObjCContainerDecl *FirstContainer,
+                                    StringRef FirstModule,
+                                    const ObjCProtocolList &SecondProtocols,
+                                    const ObjCContainerDecl *SecondContainer,
+                                    StringRef SecondModule) const;
+
 private:
   DiagnosticsEngine &Diags;
   const ASTContext &Context;
   const LangOptions &LangOpts;
 };
 
 } // namespace clang
 
 #endif

clang/include/clang/AST/ODRHash.h

public:
   // more information than the AddDecl class.  SkipBody will process the
   // hash as if the function has no body.
   void AddFunctionDecl(const FunctionDecl *Function, bool SkipBody = false);
 
   // Use this for ODR checking enums between modules.  This method compares
   // more information than the AddDecl class.
   void AddEnumDecl(const EnumDecl *Enum);
 
+  // Use this for ODR checking ObjC protocols. This
+  // method compares more information than the AddDecl class.
+  void AddObjCProtocolDecl(const ObjCProtocolDecl *P);
+
   // Process SubDecls of the main Decl.  This method calls the DeclVisitor
   // while AddDecl does not.
   void AddSubDecl(const Decl *D);
 
   // Reset the object for reuse.
   void clear();
 
   // Add booleans to ID and uses it to calculate the hash.

clang/include/clang/Basic/DiagnosticASTKinds.td

def note_module_odr_violation_enum : Note<"but in '%0' found "
   "enum %select{without|with}2 specified type|"
   "enum with specified type %2|"
   "enum with %2 element%s2|"
   "%ordinal2 element has name %3|"
   "%ordinal2 element %3 %select{has|does not have}4 an initializer|"
   "%ordinal2 element %3 has different initializer|"
   "}1">;
 
+def err_module_odr_violation_referenced_protocols : Error <
+  "%q0 has different definitions in different modules; first difference is "
+  "%select{definition in module '%2'|defined here}1 found "
+  "%select{"
+  "%4 referenced %plural{1:protocol|:protocols}4|"
+  "%ordinal4 referenced protocol with name %5"
+  "}3">;
+def note_module_odr_violation_referenced_protocols : Note <"but in '%0' found "
+  "%select{"
+  "%2 referenced %plural{1:protocol|:protocols}2|"
+  "%ordinal2 referenced protocol with different name %3"
+  "}1">;
+
 def err_module_odr_violation_mismatch_decl_unknown : Error<
   "%q0 %select{with definition in module '%2'|defined here}1 has different "
   "definitions in different modules; first difference is this "
   "%select{||||static assert|field|method|type alias|typedef|data member|"
   "friend declaration|function template|"
   "unexpected decl}3">;
 def note_module_odr_violation_mismatch_decl_unknown : Note<
   "but in '%0' found "

clang/include/clang/Serialization/ASTReader.h

private:
   /// 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;
 
   using DataPointers =
       std::pair<CXXRecordDecl *, struct CXXRecordDecl::DefinitionData *>;
+  using ObjCProtocolDataPointers =
+      std::pair<ObjCProtocolDecl *, struct ObjCProtocolDecl::DefinitionData *>;
 
   /// Record definitions in which we found an ODR violation.
   llvm::SmallDenseMap<CXXRecordDecl *, llvm::SmallVector<DataPointers, 2>, 2>
       PendingOdrMergeFailures;
 
   /// Function definitions in which we found an ODR violation.
   llvm::SmallDenseMap<FunctionDecl *, llvm::SmallVector<FunctionDecl *, 2>, 2>
       PendingFunctionOdrMergeFailures;
 
   /// Enum definitions in which we found an ODR violation.
   llvm::SmallDenseMap<EnumDecl *, llvm::SmallVector<EnumDecl *, 2>, 2>
       PendingEnumOdrMergeFailures;
 
+  /// ObjCProtocolDecl in which we found an ODR violation.
+  llvm::SmallDenseMap<ObjCProtocolDecl *,
+                      llvm::SmallVector<ObjCProtocolDataPointers, 2>, 2>
+      PendingObjCProtocolOdrMergeFailures;
+
   /// DeclContexts in which we have diagnosed an ODR violation.
   llvm::SmallPtrSet<DeclContext*, 2> DiagnosedOdrMergeFailures;
 
   /// The set of Objective-C categories that have been deserialized
   /// since the last time the declaration chains were linked.
   llvm::SmallPtrSet<ObjCCategoryDecl *, 16> CategoriesDeserialized;
 
   /// The set of Objective-C class definitions that have already been

clang/lib/AST/DeclObjC.cpp

 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTMutationListener.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
+#include "clang/AST/ODRHash.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/None.h"
if ((MethodDecl = I->lookupMethod(Sel, isInstance)))
       return MethodDecl;
   return nullptr;
 }
 
 void ObjCProtocolDecl::allocateDefinitionData() {
   assert(!Data.getPointer() && "Protocol already has a definition!");
   Data.setPointer(new (getASTContext()) DefinitionData);
   Data.getPointer()->Definition = this;
+  Data.getPointer()->HasODRHash = false;
 }
 
 void ObjCProtocolDecl::startDefinition() {
   allocateDefinitionData();
 
   // Update all of the declarations with a pointer to the definition.
   for (auto *RD : redecls())
     RD->Data = this->Data;

 StringRef
 ObjCProtocolDecl::getObjCRuntimeNameAsString() const {
   if (const auto *ObjCRTName = getAttr<ObjCRuntimeNameAttr>())
     return ObjCRTName->getMetadataName();
 
   return getName();
 }
 
+unsigned ObjCProtocolDecl::getODRHash() {
+  assert(hasDefinition() && "ODRHash only for records with definitions");
+
+  // Previously calculated hash is stored in DefinitionData.
+  if (hasODRHash())
+    return data().ODRHash;
+
+  // Only calculate hash on first call of getODRHash per record.
+  ODRHash Hasher;
+  Hasher.AddObjCProtocolDecl(getDefinition());
+  data().ODRHash = Hasher.CalculateHash();
+  setHasODRHash(true);
+
+  return data().ODRHash;
+}
+
+bool ObjCProtocolDecl::hasODRHash() const {
+  if (!hasDefinition())
+    return false;
+  return data().HasODRHash;
+}
+
+void ObjCProtocolDecl::setHasODRHash(bool HasHash) {
+  assert(hasDefinition() && "Cannot set ODRHash without definition");
+  data().HasODRHash = HasHash;
+}
+
 //===----------------------------------------------------------------------===//
 // ObjCCategoryDecl
 //===----------------------------------------------------------------------===//
 
 void ObjCCategoryDecl::anchor() {}
 
 ObjCCategoryDecl::ObjCCategoryDecl(DeclContext *DC, SourceLocation AtLoc,
                                    SourceLocation ClassNameLoc,

clang/lib/AST/ODRDiagsEmitter.cpp

bool ODRDiagsEmitter::diagnoseSubMismatchVar(const NamedDecl *FirstRecord,
   if (FirstIsConstexpr != SecondIsConstexpr) {
     DiagError(VarConstexpr) << FirstName << FirstIsConstexpr;
     DiagNote(VarConstexpr) << SecondName << SecondIsConstexpr;
     return true;
   }
   return false;
 }
 
+bool ODRDiagsEmitter::diagnoseSubMismatchProtocols(
+    const ObjCProtocolList &FirstProtocols,
+    const ObjCContainerDecl *FirstContainer, StringRef FirstModule,
+    const ObjCProtocolList &SecondProtocols,
+    const ObjCContainerDecl *SecondContainer, StringRef SecondModule) const {
+  // Keep in sync with err_module_odr_violation_referenced_protocols.
+  enum ODRReferencedProtocolDifference {
+    NumProtocols,
+    ProtocolType,
+  };
+  auto DiagRefProtocolError = [FirstContainer, FirstModule,
+                               this](SourceLocation Loc, SourceRange Range,
+                                     ODRReferencedProtocolDifference DiffType) {
+    return Diag(Loc, diag::err_module_odr_violation_referenced_protocols)
+           << FirstContainer << FirstModule.empty() << FirstModule << Range
+           << DiffType;
+  };
+  auto DiagRefProtocolNote = [SecondModule,
+                              this](SourceLocation Loc, SourceRange Range,
+                                    ODRReferencedProtocolDifference DiffType) {
+    return Diag(Loc, diag::note_module_odr_violation_referenced_protocols)
+           << SecondModule << Range << DiffType;
+  };
+  auto GetProtoListSourceRange = [](const ObjCProtocolList &PL) {
+    if (PL.empty())
+      return SourceRange();
+    return SourceRange(*PL.loc_begin(), *std::prev(PL.loc_end()));
+  };
+
+  if (FirstProtocols.size() != SecondProtocols.size()) {
+    DiagRefProtocolError(FirstContainer->getLocation(),
+                         GetProtoListSourceRange(FirstProtocols), NumProtocols)
+        << FirstProtocols.size();
+    DiagRefProtocolNote(SecondContainer->getLocation(),
+                        GetProtoListSourceRange(SecondProtocols), NumProtocols)
+        << SecondProtocols.size();
+    return true;
+  }
+
+  for (unsigned I = 0, E = FirstProtocols.size(); I != E; ++I) {
+    const ObjCProtocolDecl *FirstProtocol = FirstProtocols[I];
+    const ObjCProtocolDecl *SecondProtocol = SecondProtocols[I];
+    DeclarationName FirstProtocolName = FirstProtocol->getDeclName();
+    DeclarationName SecondProtocolName = SecondProtocol->getDeclName();
+    if (FirstProtocolName != SecondProtocolName) {
+      SourceLocation FirstLoc = *(FirstProtocols.loc_begin() + I);
+      SourceLocation SecondLoc = *(SecondProtocols.loc_begin() + I);
+      SourceRange EmptyRange;
+      DiagRefProtocolError(FirstLoc, EmptyRange, ProtocolType)
+          << (I + 1) << FirstProtocolName;
+      DiagRefProtocolNote(SecondLoc, EmptyRange, ProtocolType)
+          << (I + 1) << SecondProtocolName;
+      return true;
+    }
+  }
+
+  return false;
+}
+
 ODRDiagsEmitter::DiffResult
 ODRDiagsEmitter::FindTypeDiffs(DeclHashes &FirstHashes,
                                DeclHashes &SecondHashes) {
   auto DifferenceSelector = [](const Decl *D) {
     assert(D && "valid Decl required");
     switch (D->getKind()) {
     default:
       return Other;
if (computeODRHash(FirstInit) != computeODRHash(SecondInit)) {
           << I + 1 << FirstConstant;
       DiagNote(SecondConstant, EnumConstantDifferentInitializer)
           << I + 1 << SecondConstant;
       return true;
     }
   }
   return false;
 }
+
+bool ODRDiagsEmitter::diagnoseMismatch(
+    const ObjCProtocolDecl *FirstProtocol,
+    const ObjCProtocolDecl *SecondProtocol,
+    const struct ObjCProtocolDecl::DefinitionData *SecondDD) const {
+  if (FirstProtocol == SecondProtocol)
+    return false;
+
+  std::string FirstModule = getOwningModuleNameForDiagnostic(FirstProtocol);
+  std::string SecondModule = getOwningModuleNameForDiagnostic(SecondProtocol);
+
+  const ObjCProtocolDecl::DefinitionData *FirstDD = &FirstProtocol->data();
+  assert(FirstDD && SecondDD && "Definitions without DefinitionData");
+  // Diagnostics from ObjCProtocol DefinitionData are emitted here.
+  if (FirstDD != SecondDD) {
+    // Check both protocols reference the same protocols.
+    const ObjCProtocolList &FirstProtocols =
+        FirstProtocol->getReferencedProtocols();
+    const ObjCProtocolList &SecondProtocols = SecondDD->ReferencedProtocols;
+    if (diagnoseSubMismatchProtocols(FirstProtocols, FirstProtocol, FirstModule,
+                                     SecondProtocols, SecondProtocol,
+                                     SecondModule))
+      return true;
+  }
+
+  auto PopulateHashes = [](DeclHashes &Hashes, const ObjCProtocolDecl *ID,
+                           const DeclContext *DC) {
+    for (const Decl *D : ID->decls()) {
+      if (!ODRHash::isDeclToBeProcessed(D, DC))
+        continue;
+      Hashes.emplace_back(D, computeODRHash(D));
+    }
+  };
+
+  DeclHashes FirstHashes;
+  DeclHashes SecondHashes;
+  // Use definition as DeclContext because definitions are merged when
+  // DeclContexts are merged and separate when DeclContexts are separate.
+  PopulateHashes(FirstHashes, FirstProtocol, FirstProtocol->getDefinition());
+  PopulateHashes(SecondHashes, SecondProtocol, SecondProtocol->getDefinition());
+
+  DiffResult DR = FindTypeDiffs(FirstHashes, SecondHashes);
+  ODRMismatchDecl FirstDiffType = DR.FirstDiffType;
+  ODRMismatchDecl SecondDiffType = DR.SecondDiffType;
+  const Decl *FirstDecl = DR.FirstDecl;
+  const Decl *SecondDecl = DR.SecondDecl;
+
+  if (FirstDiffType == Other || SecondDiffType == Other) {
+    diagnoseSubMismatchUnexpected(DR, FirstProtocol, FirstModule,
+                                  SecondProtocol, SecondModule);
+    return true;
+  }
+
+  if (FirstDiffType != SecondDiffType) {
+    diagnoseSubMismatchDifferentDeclKinds(DR, FirstProtocol, FirstModule,
+                                          SecondProtocol, SecondModule);
+    return true;
+  }
+
+  assert(FirstDiffType == SecondDiffType);
+  switch (FirstDiffType) {
+  case EndOfClass:
+  case Other:
+  case Field:
+  case TypeDef:
+  case Var:
+  // C++ only, invalid in this context.
+  case PublicSpecifer:
+  case PrivateSpecifer:
+  case ProtectedSpecifer:
+  case StaticAssert:
+  case CXXMethod:
+  case TypeAlias:
+  case Friend:
+  case FunctionTemplate:
+    llvm_unreachable("Invalid diff type");
+  }
+
+  Diag(FirstDecl->getLocation(),
+       diag::err_module_odr_violation_mismatch_decl_unknown)
+      << FirstProtocol << FirstModule.empty() << FirstModule << FirstDiffType
+      << FirstDecl->getSourceRange();
+  Diag(SecondDecl->getLocation(),
+       diag::note_module_odr_violation_mismatch_decl_unknown)
+      << SecondModule << FirstDiffType << SecondDecl->getSourceRange();
+  return true;
+}

clang/lib/AST/ODRHash.cpp

void ODRHash::AddEnumDecl(const EnumDecl *Enum) {
 
   ID.AddInteger(Decls.size());
   for (auto SubDecl : Decls) {
     AddSubDecl(SubDecl);
   }
 
 }
 
+void ODRHash::AddObjCProtocolDecl(const ObjCProtocolDecl *P) {
+  AddDecl(P);
+
+  // Hash referenced protocols.
+  ID.AddInteger(P->getReferencedProtocols().size());
+  for (const ObjCProtocolDecl *RefP : P->protocols()) {
+    // Hash the name only as a referenced protocol can be a forward declaration.
+    AddDeclarationName(RefP->getDeclName());
+  }
+
+  // Filter out sub-Decls which will not be processed in order to get an
+  // accurate count of Decl's.
+  llvm::SmallVector<const Decl *, 16> Decls;
+  for (Decl *SubDecl : P->decls()) {
+    if (isDeclToBeProcessed(SubDecl, P)) {
+      Decls.push_back(SubDecl);
+    }
+  }
+
+  ID.AddInteger(Decls.size());
+  for (auto *SubDecl : Decls) {
+    AddSubDecl(SubDecl);
+  }
+}
+
 void ODRHash::AddDecl(const Decl *D) {
   assert(D && "Expecting non-null pointer.");
   D = D->getCanonicalDecl();
 
   const NamedDecl *ND = dyn_cast<NamedDecl>(D);
   AddBoolean(ND);
   if (!ND) {
     ID.AddInteger(D->getKind());

clang/lib/Serialization/ASTReader.cpp

void ASTReader::finishPendingActions() {
   for (auto *ND : PendingMergedDefinitionsToDeduplicate)
     getContext().deduplicateMergedDefinitonsFor(ND);
   PendingMergedDefinitionsToDeduplicate.clear();
 }
 
 void ASTReader::diagnoseOdrViolations() {
   if (PendingOdrMergeFailures.empty() && PendingOdrMergeChecks.empty() &&
       PendingFunctionOdrMergeFailures.empty() &&
-      PendingEnumOdrMergeFailures.empty())
+      PendingEnumOdrMergeFailures.empty() &&
+      PendingObjCProtocolOdrMergeFailures.empty())
     return;
 
   // Trigger the import of the full definition of each class that had any
   // odr-merging problems, so we can produce better diagnostics for them.
   // These updates may in turn find and diagnose some ODR failures, so take
   // ownership of the set first.
   auto OdrMergeFailures = std::move(PendingOdrMergeFailures);
   PendingOdrMergeFailures.clear();
void ASTReader::diagnoseOdrViolations() {
   PendingEnumOdrMergeFailures.clear();
   for (auto &Merge : EnumOdrMergeFailures) {
     Merge.first->decls_begin();
     for (auto &Enum : Merge.second) {
       Enum->decls_begin();
     }
   }
 
+  // Trigger the import of the full protocol definition.
+  auto ObjCProtocolOdrMergeFailures =
+      std::move(PendingObjCProtocolOdrMergeFailures);
+  PendingObjCProtocolOdrMergeFailures.clear();
+  for (auto &Merge : ObjCProtocolOdrMergeFailures) {
+    Merge.first->decls_begin();
+    for (auto &ProtocolPair : Merge.second)
+      ProtocolPair.first->decls_begin();
+  }
+
   // For each declaration from a merged context, check that the canonical
   // definition of that context also contains a declaration of the same
   // entity.
   //
   // Caution: this loop does things that might invalidate iterators into
   // PendingOdrMergeChecks. Don't turn this into a range-based for loop!
   while (!PendingOdrMergeChecks.empty()) {
     NamedDecl *D = PendingOdrMergeChecks.pop_back_val();
if (!Found) {
             << Candidates[I];
       }
 
       DiagnosedOdrMergeFailures.insert(CanonDef);
     }
   }
 
   if (OdrMergeFailures.empty() && FunctionOdrMergeFailures.empty() &&
-      EnumOdrMergeFailures.empty())
+      EnumOdrMergeFailures.empty() && ObjCProtocolOdrMergeFailures.empty())
     return;
 
   // Ensure we don't accidentally recursively enter deserialization while
   // we're producing our diagnostics.
   Deserializing RecursionGuard(this);
 
   // Issue any pending ODR-failure diagnostics.
   for (auto &Merge : OdrMergeFailures) {
for (auto &SecondEnum : Merge.second) {
       if (DiagsEmitter.diagnoseMismatch(FirstEnum, SecondEnum)) {
         Diagnosed = true;
         break;
       }
     }
     (void)Diagnosed;
     assert(Diagnosed && "Unable to emit ODR diagnostic.");
   }
+
+  for (auto &Merge : ObjCProtocolOdrMergeFailures) {
+    // If we've already pointed out a specific problem with this protocol,
+    // don't bother issuing a general "something's different" diagnostic.
+    if (!DiagnosedOdrMergeFailures.insert(Merge.first).second)
+      continue;
+
+    ODRDiagsEmitter DiagsEmitter(Diags, getContext(),
+                                 getPreprocessor().getLangOpts());
+    ObjCProtocolDecl *FirstProtocol = Merge.first;
+    bool Diagnosed = false;
+    for (auto &ProtocolPair : Merge.second) {
+      if (DiagsEmitter.diagnoseMismatch(FirstProtocol, ProtocolPair.first,
+                                        ProtocolPair.second)) {
+        Diagnosed = true;
+        break;
+      }
+    }
+    (void)Diagnosed;
+    assert(Diagnosed && "Unable to emit ODR diagnostic.");
+  }
 }
 
 void ASTReader::StartedDeserializing() {
   if (++NumCurrentElementsDeserializing == 1 && ReadTimer.get())
     ReadTimer->startTimer();
 }
 
 void ASTReader::FinishedDeserializing() {

clang/lib/Serialization/ASTReaderDecl.cpp

void ASTDeclReader::ReadObjCDefinitionData(
     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());
+    Data.ODRHash = Record.readInt();
+    Data.HasODRHash = true;
 }
 
-void ASTDeclReader::MergeDefinitionData(ObjCProtocolDecl *D,
-         struct ObjCProtocolDecl::DefinitionData &&NewDD) {
+void ASTDeclReader::MergeDefinitionData(
+    ObjCProtocolDecl *D, struct ObjCProtocolDecl::DefinitionData &&NewDD) {
   struct ObjCProtocolDecl::DefinitionData &DD = D->data();
-  if (DD.Definition != NewDD.Definition) {
-    Reader.MergedDeclContexts.insert(
-        std::make_pair(NewDD.Definition, DD.Definition));
-    Reader.mergeDefinitionVisibility(DD.Definition, NewDD.Definition);
-  }
+  if (DD.Definition == NewDD.Definition)
+    return;
+
 
-  // FIXME: odr checking?
+  if (D->getODRHash() != NewDD.ODRHash)
+    Reader.PendingObjCProtocolOdrMergeFailures[DD.Definition].push_back(
+        {NewDD.Definition, &NewDD});
 }
 
 void ASTDeclReader::VisitObjCProtocolDecl(ObjCProtocolDecl *PD) {
   RedeclarableResult Redecl = VisitRedeclarable(PD);
   VisitObjCContainerDecl(PD);
   mergeRedeclarable(PD, Redecl);
 
   if (Record.readInt()) {

clang/lib/Serialization/ASTWriterDecl.cpp

void ASTDeclWriter::VisitObjCProtocolDecl(ObjCProtocolDecl *D) {
 
   Record.push_back(D->isThisDeclarationADefinition());
   if (D->isThisDeclarationADefinition()) {
     Record.push_back(D->protocol_size());
     for (const auto *I : D->protocols())
       Record.AddDeclRef(I);
     for (const auto &PL : D->protocol_locs())
       Record.AddSourceLocation(PL);
+    Record.push_back(D->getODRHash());
   }
 
   Code = serialization::DECL_OBJC_PROTOCOL;
 }
 
 void ASTDeclWriter::VisitObjCAtDefsFieldDecl(ObjCAtDefsFieldDecl *D) {
   VisitFieldDecl(D);
   Code = serialization::DECL_OBJC_AT_DEFS_FIELD;

clang/test/Modules/compare-objc-protocol.m

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// Build first header file
+// RUN: echo "#define FIRST" >> %t/include/first.h
+// RUN: cat %t/test.m        >> %t/include/first.h
+// RUN: echo "#undef FIRST"  >> %t/include/first.h
+
+// Build second header file
+// RUN: echo "#define SECOND" >> %t/include/second.h
+// RUN: cat %t/test.m         >> %t/include/second.h
+// RUN: echo "#undef SECOND"  >> %t/include/second.h
+
+// Test that each header can compile
+// RUN: %clang_cc1 -fsyntax-only -x objective-c %t/include/first.h -fblocks -fobjc-arc
+// RUN: %clang_cc1 -fsyntax-only -x objective-c %t/include/second.h -fblocks -fobjc-arc
+
+// Run test
+// RUN: %clang_cc1 -I%t/include -verify %t/test.m -fblocks -fobjc-arc \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// In non-modular case we ignore protocol redefinitions. But with modules
+// previous definition can come from a hidden [sub]module. And in this case we
+// allow a new definition if it is equivalent to the hidden one.
+//
+// This test case is to verify equivalence checks.
+
+//--- include/common.h
+#ifndef COMMON_H
+#define COMMON_H
+@protocol CommonProtocol @end
+@protocol ExtraProtocol @end
+#endif
+
+//--- include/first-empty.h
+//--- include/module.modulemap
+module Common {
+  header "common.h"
+  export *
+}
+module First {
+  module Empty {
+    header "first-empty.h"
+  }
+  module Hidden {
+    header "first.h"
+    export *
+  }
+}
+module Second {
+  header "second.h"
+  export *
+}
+
+//--- test.m
+#if defined(FIRST) || defined(SECOND)
+# include "common.h"
+#endif
+
+#if !defined(FIRST) && !defined(SECOND)
+# include "first-empty.h"
+# include "second.h"
+#endif
+
+#if defined(FIRST)
+@protocol CompareForwardDeclaration1;
+@protocol CompareForwardDeclaration2<CommonProtocol> @end
+#elif defined(SECOND)
+@protocol CompareForwardDeclaration1<CommonProtocol> @end
+@protocol CompareForwardDeclaration2;
+#else
+id<CompareForwardDeclaration1> compareForwardDeclaration1;
+id<CompareForwardDeclaration2> compareForwardDeclaration2;
+#endif
+
+#if defined(FIRST)
+@protocol CompareMatchingConformingProtocols<CommonProtocol> @end
+@protocol ForwardProtocol;
+@protocol CompareMatchingConformingForwardProtocols<ForwardProtocol> @end
+
+@protocol CompareProtocolPresence1<CommonProtocol> @end
+@protocol CompareProtocolPresence2 @end
+
+@protocol CompareDifferentProtocols<CommonProtocol> @end
+@protocol CompareProtocolOrder<CommonProtocol, ExtraProtocol> @end
+#elif defined(SECOND)
+@protocol CompareMatchingConformingProtocols<CommonProtocol> @end
+@protocol ForwardProtocol @end
+@protocol CompareMatchingConformingForwardProtocols<ForwardProtocol> @end
+
+@protocol CompareProtocolPresence1 @end
+@protocol CompareProtocolPresence2<CommonProtocol> @end
+
+@protocol CompareDifferentProtocols<ExtraProtocol> @end
+@protocol CompareProtocolOrder<ExtraProtocol, CommonProtocol> @end
+#else
+id<CompareMatchingConformingProtocols> compareMatchingConformingProtocols;
+id<CompareMatchingConformingForwardProtocols> compareMatchingConformingForwardProtocols;
+
+id<CompareProtocolPresence1> compareProtocolPresence1;
+// expected-error@first.h:* {{'CompareProtocolPresence1' has different definitions in different modules; first difference is definition in module 'First.Hidden' found 1 referenced protocol}}
+// expected-note@second.h:* {{but in 'Second' found 0 referenced protocols}}
+id<CompareProtocolPresence2> compareProtocolPresence2;
+// expected-error@first.h:* {{'CompareProtocolPresence2' has different definitions in different modules; first difference is definition in module 'First.Hidden' found 0 referenced protocols}}
+// expected-note@second.h:* {{but in 'Second' found 1 referenced protocol}}
+
+id<CompareDifferentProtocols> compareDifferentProtocols;
+// expected-error@first.h:* {{'CompareDifferentProtocols' has different definitions in different modules; first difference is definition in module 'First.Hidden' found 1st referenced protocol with name 'CommonProtocol'}}
+// expected-note@second.h:* {{but in 'Second' found 1st referenced protocol with different name 'ExtraProtocol'}}
+id<CompareProtocolOrder> compareProtocolOrder;
+// expected-error@first.h:* {{'CompareProtocolOrder' has different definitions in different modules; first difference is definition in module 'First.Hidden' found 1st referenced protocol with name 'CommonProtocol'}}
+// expected-note@second.h:* {{but in 'Second' found 1st referenced protocol with different name 'ExtraProtocol'}}
+#endif