[ODRHash] Detect duplicate `ObjCProtocolDecl` ODR mismatches during parsing.

[vsapsai]

Summary

When during parsing we encountered a duplicate ObjCProtocolDecl, we
were always emitting an error. With this change we accept

• when a previous ObjCProtocolDecl is in a hidden [sub]module;
• parsed ObjCProtocolDecl is the same as the previous one.

And in case of mismatches we provide more detailed error messages.

rdar://93069080

[vsapsai]

Rebase.

[vsapsai]

Rebase.

[vsapsai]

Ping.

[ChuanqiXu]

Although I'm not familiar with Objc, the change here looks good generally.

clang/include/clang/Sema/Sema.h

10131–10137

It looks better to add a comment for the new arg.

[bruno]

LGTM

[vsapsai]

Thanks for reviews! It was a long journey.

[NancyWang2222]

@vsapsai hi Volodymyr Sapsai , test case clang/test/Modules/hidden-duplicates.m is failing on our llvm community AIX box https://lab.llvm.org/buildbot/#/builders/214/builds/4442/steps/6/logs/FAIL__Clang__hidden-duplicates_m , it gives exit code 70 without detail error message, can you help fix it as soon as possible. Thanks

[vsapsai]

In D130327#3944188, @NancyWang2222 wrote:

@vsapsai hi Volodymyr Sapsai , test case clang/test/Modules/hidden-duplicates.m is failing on our llvm community AIX box https://lab.llvm.org/buildbot/#/builders/214/builds/4442/steps/6/logs/FAIL__Clang__hidden-duplicates_m , it gives exit code 70 without detail error message, can you help fix it as soon as possible. Thanks

Does AIX have support for compiling Objective-C? I'm not entirely sure that's the right fix but many tests have // UNSUPPORTED: -zos, -aix, so I'm inclined to disable the test for AIX.

[daltenty]

In D130327#3944591, @vsapsai wrote:

In D130327#3944188, @NancyWang2222 wrote:

@vsapsai hi Volodymyr Sapsai , test case clang/test/Modules/hidden-duplicates.m is failing on our llvm community AIX box https://lab.llvm.org/buildbot/#/builders/214/builds/4442/steps/6/logs/FAIL__Clang__hidden-duplicates_m , it gives exit code 70 without detail error message, can you help fix it as soon as possible. Thanks

Does AIX have support for compiling Objective-C? I'm not entirely sure that's the right fix but many tests have // UNSUPPORTED: -zos, -aix, so I'm inclined to disable the test for AIX.

Generally, no. There is no Objective-C runtime currently available on the platform AFAIK. Committed an XFAIL for the issue here: https://reviews.llvm.org/rGb888cafcbc0a16f68bc0ebea495920b952290ea2

clang/include/clang/AST/DeclObjC.h

public:
   /// definition.
   bool isThisDeclarationADefinition() const {
     return getDefinition() == this;
   }
 
   /// Starts the definition of this Objective-C protocol.
   void startDefinition();
 
+  /// Starts the definition without sharing it with other redeclarations.
+  /// Such definition shouldn't be used for anything but only to compare if
+  /// a duplicate is compatible with previous definition or if it is
+  /// a distinct duplicate.
+  void startDuplicateDefinitionForComparison();
+  void mergeDuplicateDefinitionWithCommon(const ObjCProtocolDecl *Definition);
+
   /// Produce a name to be used for protocol's metadata. It comes either via
   /// objc_runtime_name attribute or protocol name.
   StringRef getObjCRuntimeNameAsString() const;
 
   SourceRange getSourceRange() const override LLVM_READONLY {
     if (isThisDeclarationADefinition())
       return ObjCContainerDecl::getSourceRange();
 

clang/include/clang/AST/ODRDiagsEmitter.h

public:
   /// 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;
 
+  /// Diagnose ODR mismatch between ObjCProtocolDecl with different definitions.
+  bool diagnoseMismatch(const ObjCProtocolDecl *FirstProtocol,
+                        const ObjCProtocolDecl *SecondProtocol) const {
+    assert(FirstProtocol->data().Definition !=
+               SecondProtocol->data().Definition &&
+           "Don't diagnose differences when definitions are merged already");
+    return diagnoseMismatch(FirstProtocol, SecondProtocol,
+                            &SecondProtocol->data());
+  }
+
   /// Get the best name we know for the module that owns the given
   /// declaration, or an empty string if the declaration is not from a module.
   static std::string getOwningModuleNameForDiagnostic(const Decl *D);
 
 private:
   using DeclHashes = llvm::SmallVector<std::pair<const Decl *, unsigned>, 4>;
 
   // Used with err_module_odr_violation_mismatch_decl,

clang/include/clang/Basic/DiagnosticASTKinds.td

def note_module_odr_violation_template_parameter : Note <
   "template parameter with different default argument}1">;
 
 def err_module_odr_violation_mismatch_decl : Error<
   "%q0 has different definitions in different modules; first difference is "
   "%select{definition in module '%2'|defined here}1 found "
   "%select{end of class|public access specifier|private access specifier|"
   "protected access specifier|static assert|field|method|type alias|typedef|"
   "data member|friend declaration|function template|method|property}3">;
-def note_module_odr_violation_mismatch_decl : Note<"but in '%0' found "
+def note_module_odr_violation_mismatch_decl : Note<
+  "but in %select{'%1'|definition here}0 found "
   "%select{end of class|public access specifier|private access specifier|"
   "protected access specifier|static assert|field|method|type alias|typedef|"
-  "data member|friend declaration|function template|method|property}1">;
+  "data member|friend declaration|function template|method|property}2">;
 
 def err_module_odr_violation_record : Error<
   "%q0 has different definitions in different modules; first difference is "
   "%select{definition in module '%2'|defined here}1 found "
   "%select{"
   "static assert with condition|"
   "static assert with message|"
   "static assert with %select{|no }4message|"

 
 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 "
+def note_module_odr_violation_referenced_protocols : Note <
+  "but in %select{'%1'|definition here}0 found "
   "%select{"
-  "%2 referenced %plural{1:protocol|:protocols}2|"
-  "%ordinal2 referenced protocol with different name %3"
-  "}1">;
+  "%3 referenced %plural{1:protocol|:protocols}3|"
+  "%ordinal3 referenced protocol with different name %4"
+  "}2">;
 
 def err_module_odr_violation_objc_method : Error<
   "%q0 has different definitions in different modules; first difference is "
   "%select{definition in module '%2'|defined here}1 found "
   "%select{"
   "method %4 with return type %5|"
   "%select{class|instance}5 method %4|"
   "%select{no|'required'|'optional'}4 method control|"
   "method %4 with %select{no designated initializer|designated initializer}5|"
   "%select{regular|direct}5 method %4|"
   "method %4"
   "}3">;
-def note_module_odr_violation_objc_method : Note<"but in '%0' found "
+def note_module_odr_violation_objc_method : Note<
+  "but in %select{'%1'|definition here}0 found "
   "%select{"
-  "method %2 with different return type %3|"
-  "method %2 as %select{class|instance}3 method|"
-  "%select{no|'required'|'optional'}2 method control|"
-  "method %2 with %select{no designated initializer|designated initializer}3|"
-  "%select{regular|direct}3 method %2|"
-  "different method %2"
-  "}1">;
+  "method %3 with different return type %4|"
+  "method %3 as %select{class|instance}4 method|"
+  "%select{no|'required'|'optional'}3 method control|"
+  "method %3 with %select{no designated initializer|designated initializer}4|"
+  "%select{regular|direct}4 method %3|"
+  "different method %3"
+  "}2">;
 
 def err_module_odr_violation_method_params : Error<
   "%q0 has different definitions in different modules; first difference is "
   "%select{definition in module '%2'|defined here}1 found "
   "%select{"
   "%select{method %5|constructor|destructor}4 "
     "that has %6 parameter%s6|"
   "%select{method %5|constructor|destructor}4 "
     "with %ordinal6 parameter of type %7%select{| decayed from %9}8|"
   "%select{method %5|constructor|destructor}4 "
     "with %ordinal6 parameter named %7"
   "}3">;
-def note_module_odr_violation_method_params : Note<"but in '%0' found "
+def note_module_odr_violation_method_params : Note<
+  "but in %select{'%1'|definition here}0 found "
   "%select{"
-  "%select{method %3|constructor|destructor}2 "
-    "that has %4 parameter%s4|"
-  "%select{method %3|constructor|destructor}2 "
-    "with %ordinal4 parameter of type %5%select{| decayed from %7}6|"
-  "%select{method %3|constructor|destructor}2 "
-    "with %ordinal4 parameter named %5"
-  "}1">;
+  "%select{method %4|constructor|destructor}3 "
+    "that has %5 parameter%s5|"
+  "%select{method %4|constructor|destructor}3 "
+    "with %ordinal5 parameter of type %6%select{| decayed from %8}7|"
+  "%select{method %4|constructor|destructor}3 "
+    "with %ordinal5 parameter named %6"
+  "}2">;
 
 def err_module_odr_violation_objc_property : Error<
   "%q0 has different definitions in different modules; first difference is "
   "%select{definition in module '%2'|defined here}1 found "
   "%select{"
   "property %4|"
   "property %4 with type %5|"
   "%select{no|'required'|'optional'}4 property control|"
   "property %4 with %select{default |}6'%select{none|readonly|getter|assign|"
     "readwrite|retain|copy|nonatomic|setter|atomic|weak|strong|"
     "unsafe_unretained|nullability|null_resettable|class|direct}5' attribute"
   "}3">;
 def note_module_odr_violation_objc_property : Note<
-  "but in '%0' found "
+  "but in %select{'%1'|definition here}0 found "
   "%select{"
-  "property %2|"
-  "property %2 with type %3|"
-  "%select{no|'required'|'optional'}2 property control|"
-  "property %2 with different '%select{none|readonly|getter|assign|"
+  "property %3|"
+  "property %3 with type %4|"
+  "%select{no|'required'|'optional'}3 property control|"
+  "property %3 with different '%select{none|readonly|getter|assign|"
     "readwrite|retain|copy|nonatomic|setter|atomic|weak|strong|"
-    "unsafe_unretained|nullability|null_resettable|class|direct}3' attribute"
-  "}1">;
+    "unsafe_unretained|nullability|null_resettable|class|direct}4' attribute"
+  "}2">;
 
 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|method|"
   "property|unexpected decl}3">;
 def note_module_odr_violation_mismatch_decl_unknown : Note<
-  "but in '%0' found "
+  "but in %select{'%1'|definition here}0 found "
   "%select{||||different static assert|different field|different method|"
   "different type alias|different typedef|different data member|"
   "different friend declaration|different function template|different method|"
-  "different property|another unexpected decl}1">;
+  "different property|another unexpected decl}2">;
 
 
 def remark_sanitize_address_insert_extra_padding_accepted : Remark<
     "-fsanitize-address-field-padding applied to %0">, ShowInSystemHeader,
     InGroup<SanitizeAddressRemarks>;
 def remark_sanitize_address_insert_extra_padding_rejected : Remark<
     "-fsanitize-address-field-padding ignored for %0 because it "
     "%select{is not C++|is packed|is a union|is trivially copyable|"

clang/include/clang/Sema/Sema.h

public:
   /// struct, or union).
   void ActOnTagStartDefinition(Scope *S, Decl *TagDecl);
 
   /// Perform ODR-like check for C/ObjC when merging tag types from modules.
   /// Differently from C++, actually parse the body and reject / error out
   /// in case of a structural mismatch.
   bool ActOnDuplicateDefinition(Decl *Prev, SkipBodyInfo &SkipBody);
 
+  /// Check ODR hashes for C/ObjC when merging types from modules.
+  /// Differently from C++, actually parse the body and reject in case
+  /// of a mismatch.
+  template <typename T,
+            typename = std::enable_if_t<std::is_base_of<NamedDecl, T>::value>>
+  bool ActOnDuplicateODRHashDefinition(T *Duplicate, T *Previous) {
+    if (Duplicate->getODRHash() != Previous->getODRHash())
+      return false;
+
+    // Make the previous decl visible.
+    makeMergedDefinitionVisible(Previous);
+    return true;
+  }
+
   typedef void *SkippedDefinitionContext;
 
   /// Invoked when we enter a tag definition that we're skipping.
   SkippedDefinitionContext ActOnTagStartSkippedDefinition(Scope *S, Decl *TD);
 
   void ActOnObjCContainerStartDefinition(ObjCContainerDecl *IDecl);
 
   /// ActOnStartCXXMemberDeclarations - Invoked when we have parsed a
Decl *ActOnCompatibilityAlias(
                     IdentifierInfo *AliasName,  SourceLocation AliasLocation,
                     IdentifierInfo *ClassName, SourceLocation ClassLocation);
 
   bool CheckForwardProtocolDeclarationForCircularDependency(
     IdentifierInfo *PName,
     SourceLocation &PLoc, SourceLocation PrevLoc,
     const ObjCList<ObjCProtocolDecl> &PList);
 
   ObjCProtocolDecl *ActOnStartProtocolInterface(
       SourceLocation AtProtoInterfaceLoc, IdentifierInfo *ProtocolName,
       SourceLocation ProtocolLoc, Decl *const *ProtoRefNames,
       unsigned NumProtoRefs, const SourceLocation *ProtoLocs,
-      SourceLocation EndProtoLoc, const ParsedAttributesView &AttrList);
+      SourceLocation EndProtoLoc, const ParsedAttributesView &AttrList,
+      SkipBodyInfo *SkipBody);
 
   ObjCCategoryDecl *ActOnStartCategoryInterface(
       SourceLocation AtInterfaceLoc, IdentifierInfo *ClassName,
       SourceLocation ClassLoc, ObjCTypeParamList *typeParamList,
       IdentifierInfo *CategoryName, SourceLocation CategoryLoc,
       Decl *const *ProtoRefs, unsigned NumProtoRefs,
       const SourceLocation *ProtoLocs, SourceLocation EndProtoLoc,
       const ParsedAttributesView &AttrList);
 

clang/lib/AST/DeclObjC.cpp

 void ObjCProtocolDecl::startDefinition() {
   allocateDefinitionData();
 
   // Update all of the declarations with a pointer to the definition.
   for (auto *RD : redecls())
     RD->Data = this->Data;
 }
 
+void ObjCProtocolDecl::startDuplicateDefinitionForComparison() {
+  Data.setPointer(nullptr);
+  allocateDefinitionData();
+  // Don't propagate data to other redeclarations.
+}
+
+void ObjCProtocolDecl::mergeDuplicateDefinitionWithCommon(
+    const ObjCProtocolDecl *Definition) {
+  Data = Definition->Data;
+}
+
 void ObjCProtocolDecl::collectPropertiesToImplement(PropertyMap &PM) const {
   if (const ObjCProtocolDecl *PDecl = getDefinition()) {
     for (auto *Prop : PDecl->properties()) {
       // Insert into PM if not there already.
       PM.insert(std::make_pair(
           std::make_pair(Prop->getIdentifier(), Prop->isClassProperty()),
           Prop));
     }

clang/lib/AST/ODRDiagsEmitter.cpp

return Diags.Report(FirstMethod->getLocation(),
            << FirstName;
   };
   auto DiagNote = [&Diags, &GetDiagMethodType, SecondModule,
                    SecondMethod](ODRMethodParametersDifference DiffType) {
     DeclarationName SecondName = SecondMethod->getDeclName();
     DiagMethodType SecondMethodType = GetDiagMethodType(SecondMethod);
     return Diags.Report(SecondMethod->getLocation(),
                         diag::note_module_odr_violation_method_params)
-           << SecondModule << SecondMethod->getSourceRange() << DiffType
-           << SecondMethodType << SecondName;
+           << SecondModule.empty() << SecondModule
+           << SecondMethod->getSourceRange() << DiffType << SecondMethodType
+           << SecondName;
   };
 
   const unsigned FirstNumParameters = FirstMethod->param_size();
   const unsigned SecondNumParameters = SecondMethod->param_size();
   if (FirstNumParameters != SecondNumParameters) {
     DiagError(NumberParameters) << FirstNumParameters;
     DiagNote(NumberParameters) << SecondNumParameters;
     return true;
auto DiagRefProtocolError = [FirstContainer, FirstModule,
     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;
+           << SecondModule.empty() << 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()) {
return Diag(FirstMethod->getLocation(),
                 diag::err_module_odr_violation_objc_method)
            << FirstObjCContainer << FirstModule.empty() << FirstModule
            << FirstMethod->getSourceRange() << DiffType;
   };
   auto DiagNote = [SecondModule, SecondMethod,
                    this](ODRMethodDifference DiffType) {
     return Diag(SecondMethod->getLocation(),
                 diag::note_module_odr_violation_objc_method)
-           << SecondModule << SecondMethod->getSourceRange() << DiffType;
+           << SecondModule.empty() << SecondModule
+           << SecondMethod->getSourceRange() << DiffType;
   };
 
   if (computeODRHash(FirstMethod->getReturnType()) !=
       computeODRHash(SecondMethod->getReturnType())) {
     DiagError(ReturnType) << FirstMethod << FirstMethod->getReturnType();
     DiagNote(ReturnType) << SecondMethod << SecondMethod->getReturnType();
     return true;
   }
auto DiagError = [FirstObjCContainer, FirstModule, FirstProp,
                     this](SourceLocation Loc, ODRPropertyDifference DiffType) {
     return Diag(Loc, diag::err_module_odr_violation_objc_property)
            << FirstObjCContainer << FirstModule.empty() << FirstModule
            << FirstProp->getSourceRange() << DiffType;
   };
   auto DiagNote = [SecondModule, SecondProp,
                    this](SourceLocation Loc, ODRPropertyDifference DiffType) {
     return Diag(Loc, diag::note_module_odr_violation_objc_property)
-           << SecondModule << SecondProp->getSourceRange() << DiffType;
+           << SecondModule.empty() << SecondModule
+           << SecondProp->getSourceRange() << DiffType;
   };
 
   IdentifierInfo *FirstII = FirstProp->getIdentifier();
   IdentifierInfo *SecondII = SecondProp->getIdentifier();
   if (FirstII->getName() != SecondII->getName()) {
     DiagError(FirstProp->getLocation(), Name) << FirstII;
     DiagNote(SecondProp->getLocation(), Name) << SecondII;
     return true;
auto FirstDiagInfo =
       GetMismatchedDeclLoc(FirstRecord, DR.FirstDiffType, DR.FirstDecl);
   Diag(FirstDiagInfo.first, diag::err_module_odr_violation_mismatch_decl)
       << FirstRecord << FirstModule.empty() << FirstModule
       << FirstDiagInfo.second << DR.FirstDiffType;
 
   auto SecondDiagInfo =
       GetMismatchedDeclLoc(SecondRecord, DR.SecondDiffType, DR.SecondDecl);
   Diag(SecondDiagInfo.first, diag::note_module_odr_violation_mismatch_decl)
-      << SecondModule << SecondDiagInfo.second << DR.SecondDiffType;
+      << SecondModule.empty() << SecondModule << SecondDiagInfo.second
+      << DR.SecondDiffType;
 }
 
 bool ODRDiagsEmitter::diagnoseMismatch(
     const CXXRecordDecl *FirstRecord, const CXXRecordDecl *SecondRecord,
     const struct CXXRecordDecl::DefinitionData *SecondDD) const {
   // Multiple different declarations got merged together; tell the user
   // where they came from.
   if (FirstRecord == SecondRecord)
bool ODRDiagsEmitter::diagnoseMismatch(
   }
 
   Diag(FirstDecl->getLocation(),
        diag::err_module_odr_violation_mismatch_decl_unknown)
       << FirstRecord << FirstModule.empty() << FirstModule << FirstDiffType
       << FirstDecl->getSourceRange();
   Diag(SecondDecl->getLocation(),
        diag::note_module_odr_violation_mismatch_decl_unknown)
-      << SecondModule << FirstDiffType << SecondDecl->getSourceRange();
+      << SecondModule.empty() << SecondModule << FirstDiffType
+      << SecondDecl->getSourceRange();
   return true;
 }
 
 bool ODRDiagsEmitter::diagnoseMismatch(
     const FunctionDecl *FirstFunction,
     const FunctionDecl *SecondFunction) const {
   if (FirstFunction == SecondFunction)
     return false;
bool ODRDiagsEmitter::diagnoseMismatch(
   }
 
   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();
+      << SecondModule.empty() << SecondModule << FirstDiffType
+      << SecondDecl->getSourceRange();
   return true;
 }

clang/lib/Parse/ParseObjc.cpp

 //===--- ParseObjC.cpp - Objective C Parsing ------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file implements the Objective-C portions of the Parser interface.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ODRDiagsEmitter.h"
 #include "clang/AST/PrettyDeclStackTrace.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Parse/ParseDiagnostic.h"
 #include "clang/Parse/Parser.h"
 #include "clang/Parse/RAIIObjectsForParser.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/Scope.h"
Parser::ParseObjCAtProtocolDeclaration(SourceLocation AtLoc,
   SmallVector<Decl *, 8> ProtocolRefs;
   SmallVector<SourceLocation, 8> ProtocolLocs;
   if (Tok.is(tok::less) &&
       ParseObjCProtocolReferences(ProtocolRefs, ProtocolLocs, false, true,
                                   LAngleLoc, EndProtoLoc,
                                   /*consumeLastToken=*/true))
     return nullptr;
 
-  Decl *ProtoType = Actions.ActOnStartProtocolInterface(
+  Sema::SkipBodyInfo SkipBody;
+  ObjCProtocolDecl *ProtoType = Actions.ActOnStartProtocolInterface(
       AtLoc, protocolName, nameLoc, ProtocolRefs.data(), ProtocolRefs.size(),
-      ProtocolLocs.data(), EndProtoLoc, attrs);
+      ProtocolLocs.data(), EndProtoLoc, attrs, &SkipBody);
 
   ParseObjCInterfaceDeclList(tok::objc_protocol, ProtoType);
+  if (SkipBody.CheckSameAsPrevious) {
+    auto *PreviousDef = cast<ObjCProtocolDecl>(SkipBody.Previous);
+    if (Actions.ActOnDuplicateODRHashDefinition(ProtoType, PreviousDef)) {
+      ProtoType->mergeDuplicateDefinitionWithCommon(
+          PreviousDef->getDefinition());
+    } else {
+      ODRDiagsEmitter DiagsEmitter(Diags, Actions.getASTContext(),
+                                   getPreprocessor().getLangOpts());
+      DiagsEmitter.diagnoseMismatch(PreviousDef, ProtoType);
+    }
+  }
   return Actions.ConvertDeclToDeclGroup(ProtoType);
 }
 
 ///   objc-implementation:
 ///     objc-class-implementation-prologue
 ///     objc-category-implementation-prologue
 ///
 ///   objc-class-implementation-prologue:

clang/lib/Sema/SemaDeclObjC.cpp

bool Sema::CheckForwardProtocolDeclarationForCircularDependency(
   }
   return res;
 }
 
 ObjCProtocolDecl *Sema::ActOnStartProtocolInterface(
     SourceLocation AtProtoInterfaceLoc, IdentifierInfo *ProtocolName,
     SourceLocation ProtocolLoc, Decl *const *ProtoRefs, unsigned NumProtoRefs,
     const SourceLocation *ProtoLocs, SourceLocation EndProtoLoc,
-    const ParsedAttributesView &AttrList) {
+    const ParsedAttributesView &AttrList, SkipBodyInfo *SkipBody) {
   bool err = false;
   // FIXME: Deal with AttrList.
   assert(ProtocolName && "Missing protocol identifier");
   ObjCProtocolDecl *PrevDecl = LookupProtocol(ProtocolName, ProtocolLoc,
                                               forRedeclarationInCurContext());
   ObjCProtocolDecl *PDecl = nullptr;
   if (ObjCProtocolDecl *Def = PrevDecl? PrevDecl->getDefinition() : nullptr) {
-    // If we already have a definition, complain.
-    Diag(ProtocolLoc, diag::warn_duplicate_protocol_def) << ProtocolName;
-    Diag(Def->getLocation(), diag::note_previous_definition);
-
     // Create a new protocol that is completely distinct from previous
     // declarations, and do not make this protocol available for name lookup.
     // That way, we'll end up completely ignoring the duplicate.
     // FIXME: Can we turn this into an error?
     PDecl = ObjCProtocolDecl::Create(Context, CurContext, ProtocolName,
                                      ProtocolLoc, AtProtoInterfaceLoc,
-                                     /*PrevDecl=*/nullptr);
+                                     /*PrevDecl=*/Def);
+
+    if (SkipBody && !hasVisibleDefinition(Def)) {
+      SkipBody->CheckSameAsPrevious = true;
+      SkipBody->New = PDecl;
+      SkipBody->Previous = Def;
+    } else {
+      // If we already have a definition, complain.
+      Diag(ProtocolLoc, diag::warn_duplicate_protocol_def) << ProtocolName;
+      Diag(Def->getLocation(), diag::note_previous_definition);
+    }
 
     // If we are using modules, add the decl to the context in order to
     // serialize something meaningful.
     if (getLangOpts().Modules)
       PushOnScopeChains(PDecl, TUScope);
-    PDecl->startDefinition();
+    PDecl->startDuplicateDefinitionForComparison();
   } else {
     if (PrevDecl) {
       // Check for circular dependencies among protocol declarations. This can
       // only happen if this protocol was forward-declared.
       ObjCList<ObjCProtocolDecl> PList;
       PList.set((ObjCProtocolDecl *const*)ProtoRefs, NumProtoRefs, Context);
       err = CheckForwardProtocolDeclarationForCircularDependency(
               ProtocolName, ProtocolLoc, PrevDecl->getLocation(), PList);

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

 // 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
 
+// Run the same test with second.h being modular
+// RUN: cat %t/include/second.modulemap >> %t/include/module.modulemap
+// RUN: %clang_cc1 -I%t/include -verify %t/test.m -fblocks -fobjc-arc -DTEST_MODULAR=1 \
+// 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
module First {
   module Empty {
     header "first-empty.h"
   }
   module Hidden {
     header "first.h"
     export *
   }
 }
+//--- include/second.modulemap
 module Second {
   header "second.h"
   export *
 }
 
 //--- test.m
 #if defined(FIRST) || defined(SECOND)
 # include "common.h"

 @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}}
+#ifdef TEST_MODULAR
 // expected-note@second.h:* {{but in 'Second' found 0 referenced protocols}}
+#else
+// expected-note@second.h:* {{but in definition here found 0 referenced protocols}}
+#endif
 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}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} 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'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} 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'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found 1st referenced protocol with different name 'ExtraProtocol'}}
 #endif
 
 #if defined(FIRST)
 @protocol CompareMatchingMethods
 - (float)matchingMethod:(int)arg;
 @end
 
 @protocol CompareMethodPresence1

 @protocol CompareMethodRequirednessDefault
 @required
 - (void)methodRequiredness;
 @end
 #else
 id<CompareMatchingMethods> compareMatchingMethods; // no error
 id<CompareMethodPresence1> compareMethodPresence1;
 // expected-error@first.h:* {{'CompareMethodPresence1' has different definitions in different modules; first difference is definition in module 'First.Hidden' found method}}
-// expected-note@second.h:* {{but in 'Second' found end of class}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found end of class}}
 id<CompareMethodPresence2> compareMethodPresence2;
 // expected-error@first.h:* {{'CompareMethodPresence2' has different definitions in different modules; first difference is definition in module 'First.Hidden' found end of class}}
-// expected-note@second.h:* {{but in 'Second' found method}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found method}}
 id<CompareMethodName> compareMethodName;
 // expected-error@first.h:* {{'CompareMethodName' has different definitions in different modules; first difference is definition in module 'First.Hidden' found method 'methodNameA'}}
-// expected-note@second.h:* {{but in 'Second' found different method 'methodNameB'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found different method 'methodNameB'}}
 
 id<CompareMethodArgCount> compareMethodArgCount;
 // expected-error@first.h:* {{'CompareMethodArgCount' has different definitions in different modules; first difference is definition in module 'First.Hidden' found method 'methodArgCount::' that has 2 parameters}}
-// expected-note@second.h:* {{but in 'Second' found method 'methodArgCount:' that has 1 parameter}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found method 'methodArgCount:' that has 1 parameter}}
 id<CompareMethodArgName> compareMethodArgName;
 // expected-error@first.h:* {{'CompareMethodArgName' has different definitions in different modules; first difference is definition in module 'First.Hidden' found method 'methodArgName:' with 1st parameter named 'argNameA'}}
-// expected-note@second.h:* {{but in 'Second' found method 'methodArgName:' with 1st parameter named 'argNameB'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found method 'methodArgName:' with 1st parameter named 'argNameB'}}
 id<CompareMethodArgType> compareMethodArgType;
 // expected-error@first.h:* {{'CompareMethodArgType' has different definitions in different modules; first difference is definition in module 'First.Hidden' found method 'methodArgType:' with 1st parameter of type 'int'}}
-// expected-note@second.h:* {{but in 'Second' found method 'methodArgType:' with 1st parameter of type 'float'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found method 'methodArgType:' with 1st parameter of type 'float'}}
 
 id<CompareMethodReturnType> compareMethodReturnType;
 // expected-error@first.h:* {{'CompareMethodReturnType' has different definitions in different modules; first difference is definition in module 'First.Hidden' found method 'methodReturnType' with return type 'int'}}
-// expected-note@second.h:* {{but in 'Second' found method 'methodReturnType' with different return type 'float'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found method 'methodReturnType' with different return type 'float'}}
 
 id<CompareMethodOrder> compareMethodOrder;
 // expected-error@first.h:* {{'CompareMethodOrder' has different definitions in different modules; first difference is definition in module 'First.Hidden' found method 'methodOrderFirst'}}
-// expected-note@second.h:* {{but in 'Second' found different method 'methodOrderSecond'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found different method 'methodOrderSecond'}}
 id<CompareMethodClassInstance> compareMethodClassInstance;
 // expected-error@first.h:* {{'CompareMethodClassInstance' has different definitions in different modules; first difference is definition in module 'First.Hidden' found instance method 'methodClassInstance'}}
-// expected-note@second.h:* {{but in 'Second' found method 'methodClassInstance' as class method}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found method 'methodClassInstance' as class method}}
 
 id<CompareMethodRequirednessExplicit> compareMethodRequirednessExplicit;
 // expected-error@first.h:* {{'CompareMethodRequirednessExplicit' has different definitions in different modules; first difference is definition in module 'First.Hidden' found 'optional' method control}}
-// expected-note@second.h:* {{but in 'Second' found 'required' method control}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found 'required' method control}}
 id<CompareMethodRequirednessDefault> compareMethodRequirednessDefault; // no error
 #endif
 
 #if defined(FIRST)
 @protocol CompareMatchingProperties
 @property int matchingPropName;
 @end
 

 @end
 @protocol CompareLastImplAttribute
 @property int lastImplAttribute;
 @end
 #else
 id<CompareMatchingProperties> compareMatchingProperties;
 id<ComparePropertyPresence1> comparePropertyPresence1;
 // expected-error@first.h:* {{'ComparePropertyPresence1' has different definitions in different modules; first difference is definition in module 'First.Hidden' found property}}
-// expected-note@second.h:* {{but in 'Second' found end of class}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found end of class}}
 id<ComparePropertyPresence2> comparePropertyPresence2;
 // expected-error@first.h:* {{'ComparePropertyPresence2' has different definitions in different modules; first difference is definition in module 'First.Hidden' found end of class}}
-// expected-note@second.h:* {{but in 'Second' found property}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found property}}
 id<ComparePropertyName> comparePropertyName;
 // expected-error@first.h:* {{'ComparePropertyName' has different definitions in different modules; first difference is definition in module 'First.Hidden' found property 'propNameA'}}
-// expected-note@second.h:* {{but in 'Second' found property 'propNameB'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found property 'propNameB'}}
 id<ComparePropertyType> comparePropertyType;
 // expected-error@first.h:* {{'ComparePropertyType' has different definitions in different modules; first difference is definition in module 'First.Hidden' found property 'propType' with type 'int'}}
-// expected-note@second.h:* {{but in 'Second' found property 'propType' with type 'float'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found property 'propType' with type 'float'}}
 id<ComparePropertyOrder> comparePropertyOrder;
 // expected-error@first.h:* {{'ComparePropertyOrder' has different definitions in different modules; first difference is definition in module 'First.Hidden' found property 'propOrderX'}}
-// expected-note@second.h:* {{but in 'Second' found property 'propOrderY'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found property 'propOrderY'}}
 
 id<CompareMatchingPropertyAttributes> compareMatchingPropertyAttributes;
 id<ComparePropertyAttributes> comparePropertyAttributes;
 // expected-error@first.h:* {{'ComparePropertyAttributes' has different definitions in different modules; first difference is definition in module 'First.Hidden' found property 'propAttributes' with 'nonatomic' attribute}}
-// expected-note@second.h:* {{but in 'Second' found property 'propAttributes' with different 'nonatomic' attribute}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found property 'propAttributes' with different 'nonatomic' attribute}}
 id<CompareFirstImplAttribute> compareFirstImplAttribute;
 // expected-error@first.h:* {{'CompareFirstImplAttribute' has different definitions in different modules; first difference is definition in module 'First.Hidden' found property 'firstImplAttribute' with default 'readonly' attribute}}
-// expected-note@second.h:* {{but in 'Second' found property 'firstImplAttribute' with different 'readonly' attribute}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found property 'firstImplAttribute' with different 'readonly' attribute}}
 id<CompareLastImplAttribute> compareLastImplAttribute;
 // expected-error@first.h:* {{'CompareLastImplAttribute' has different definitions in different modules; first difference is definition in module 'First.Hidden' found property 'lastImplAttribute' with 'class' attribute}}
-// expected-note@second.h:* {{but in 'Second' found property 'lastImplAttribute' with different 'class' attribute}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found property 'lastImplAttribute' with different 'class' attribute}}
 #endif

clang/test/Modules/hidden-duplicates.m

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -emit-llvm -o %t/test.bc -I %t/include %t/test.m -verify \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+// RUN: %clang_cc1 -emit-llvm -o %t/test.bc -I %t/include %t/test.m -verify -DTEST_MAKE_HIDDEN_VISIBLE=1 \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+// RUN: %clang_cc1 -emit-llvm -o %t/test.bc -I %t/include %t/test.m -verify -x objective-c++ \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+// RUN: %clang_cc1 -emit-llvm -o %t/test.bc -I %t/include %t/test.m -verify -DTEST_MAKE_HIDDEN_VISIBLE=1 -x objective-c++ \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Test parsing duplicate Objective-C entities when a previous entity is defined
+// in a hidden [sub]module and cannot be used.
+//
+// Testing with header guards and includes on purpose as tracking imports in
+// modules is a separate issue.
+
+//--- include/textual.h
+#ifndef TEXTUAL_H
+#define TEXTUAL_H
+
+@protocol TestProtocol
+- (void)someMethod;
+@end
+
+@protocol ForwardDeclaredProtocolWithoutDefinition;
+
+id<TestProtocol> protocolDefinition(id<TestProtocol> t);
+id<ForwardDeclaredProtocolWithoutDefinition> forwardDeclaredProtocol(
+    id<ForwardDeclaredProtocolWithoutDefinition> t);
+
+#endif
+
+//--- include/empty.h
+//--- include/initially_hidden.h
+#include "textual.h"
+
+//--- include/module.modulemap
+module Piecewise {
+  module Empty {
+    header "empty.h"
+  }
+  module InitiallyHidden {
+    header "initially_hidden.h"
+    export *
+  }
+}
+
+//--- test.m
+// Including empty.h loads the entire module Piecewise but keeps InitiallyHidden hidden.
+#include "empty.h"
+#include "textual.h"
+#ifdef TEST_MAKE_HIDDEN_VISIBLE
+#include "initially_hidden.h"
+#endif
+// expected-no-diagnostics