[ODRHash] Detect duplicate ObjCProtocolDecl ODR mismatches during parsing.

vsapsai · vsapsai · commit a65d5309d5b7 · 2022-11-17T18:31:32.000-08:00
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 Differential Revision: https://reviews.llvm.org/D130327
diff --git a/clang/include/clang/AST/DeclObjC.h b/clang/include/clang/AST/DeclObjC.h
@@ -2230,6 +2230,13 @@ class ObjCProtocolDecl : public ObjCContainerDecl,
   /// 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;
diff --git a/clang/include/clang/AST/ODRDiagsEmitter.h b/clang/include/clang/AST/ODRDiagsEmitter.h
@@ -55,6 +55,16 @@ class ODRDiagsEmitter {
       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);
diff --git a/clang/include/clang/Basic/DiagnosticASTKinds.td b/clang/include/clang/Basic/DiagnosticASTKinds.td
@@ -622,10 +622,11 @@ def err_module_odr_violation_mismatch_decl : Error<
   "%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 "
@@ -843,11 +844,12 @@ def err_module_odr_violation_referenced_protocols : Error <
   "%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 "
@@ -860,15 +862,16 @@ def err_module_odr_violation_objc_method : Error<
   "%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 "
@@ -881,15 +884,16 @@ def err_module_odr_violation_method_params : Error<
   "%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 "
@@ -903,15 +907,15 @@ def err_module_odr_violation_objc_property : Error<
     "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 "
@@ -920,11 +924,11 @@ def err_module_odr_violation_mismatch_decl_unknown : Error<
   "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<
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
@@ -3411,6 +3411,20 @@ class Sema final {
   /// 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.
@@ -10118,7 +10132,8 @@ class Sema final {
       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,
diff --git a/clang/lib/AST/DeclObjC.cpp b/clang/lib/AST/DeclObjC.cpp
@@ -1997,6 +1997,17 @@ void ObjCProtocolDecl::startDefinition() {
     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()) {
diff --git a/clang/lib/AST/ODRDiagsEmitter.cpp b/clang/lib/AST/ODRDiagsEmitter.cpp
@@ -90,8 +90,9 @@ static bool diagnoseSubMismatchMethodParameters(DiagnosticsEngine &Diags,
     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();
@@ -378,7 +379,7 @@ bool ODRDiagsEmitter::diagnoseSubMismatchProtocols(
                               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())
@@ -440,7 +441,8 @@ bool ODRDiagsEmitter::diagnoseSubMismatchObjCMethod(
                    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()) !=
@@ -517,7 +519,8 @@ bool ODRDiagsEmitter::diagnoseSubMismatchObjCProperty(
   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();
@@ -690,7 +693,8 @@ void ODRDiagsEmitter::diagnoseSubMismatchDifferentDeclKinds(
   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(
@@ -1538,7 +1542,8 @@ bool ODRDiagsEmitter::diagnoseMismatch(
       << 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;
 }
 
@@ -1908,6 +1913,7 @@ bool ODRDiagsEmitter::diagnoseMismatch(
       << 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;
 }
diff --git a/clang/lib/Parse/ParseObjc.cpp b/clang/lib/Parse/ParseObjc.cpp
@@ -11,6 +11,7 @@
 //===----------------------------------------------------------------------===//
 
 #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"
@@ -2088,11 +2089,23 @@ Parser::ParseObjCAtProtocolDeclaration(SourceLocation AtLoc,
                                   /*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);
 }
 
diff --git a/clang/lib/Sema/SemaDeclObjC.cpp b/clang/lib/Sema/SemaDeclObjC.cpp
@@ -1213,31 +1213,37 @@ 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
diff --git a/clang/test/Modules/compare-objc-protocol.m b/clang/test/Modules/compare-objc-protocol.m
diff --git a/clang/test/Modules/hidden-duplicates.m b/clang/test/Modules/hidden-duplicates.m
