diff --git a/clang/include/clang/AST/DeclObjC.h b/clang/include/clang/AST/DeclObjC.h --- a/clang/include/clang/AST/DeclObjC.h +++ b/clang/include/clang/AST/DeclObjC.h @@ -2055,6 +2055,12 @@ /// 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, @@ -2091,10 +2097,15 @@ 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, @@ -2250,6 +2261,9 @@ 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; } }; diff --git a/clang/include/clang/AST/ODRDiagsEmitter.h b/clang/include/clang/AST/ODRDiagsEmitter.h --- a/clang/include/clang/AST/ODRDiagsEmitter.h +++ b/clang/include/clang/AST/ODRDiagsEmitter.h @@ -11,6 +11,7 @@ #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" @@ -44,6 +45,16 @@ 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; + /// 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); @@ -116,6 +127,16 @@ 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; diff --git a/clang/include/clang/AST/ODRHash.h b/clang/include/clang/AST/ODRHash.h --- a/clang/include/clang/AST/ODRHash.h +++ b/clang/include/clang/AST/ODRHash.h @@ -64,6 +64,10 @@ // 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); diff --git a/clang/include/clang/Basic/DiagnosticASTKinds.td b/clang/include/clang/Basic/DiagnosticASTKinds.td --- a/clang/include/clang/Basic/DiagnosticASTKinds.td +++ b/clang/include/clang/Basic/DiagnosticASTKinds.td @@ -848,6 +848,19 @@ "%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 " diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -1131,6 +1131,8 @@ using DataPointers = std::pair; + using ObjCProtocolDataPointers = + std::pair; /// Record definitions in which we found an ODR violation. llvm::SmallDenseMap, 2> @@ -1144,6 +1146,11 @@ llvm::SmallDenseMap, 2> PendingEnumOdrMergeFailures; + /// ObjCProtocolDecl in which we found an ODR violation. + llvm::SmallDenseMap, 2> + PendingObjCProtocolOdrMergeFailures; + /// DeclContexts in which we have diagnosed an ODR violation. llvm::SmallPtrSet DiagnosedOdrMergeFailures; diff --git a/clang/lib/AST/DeclObjC.cpp b/clang/lib/AST/DeclObjC.cpp --- a/clang/lib/AST/DeclObjC.cpp +++ b/clang/lib/AST/DeclObjC.cpp @@ -16,6 +16,7 @@ #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" @@ -1985,6 +1986,7 @@ assert(!Data.getPointer() && "Protocol already has a definition!"); Data.setPointer(new (getASTContext()) DefinitionData); Data.getPointer()->Definition = this; + Data.getPointer()->HasODRHash = false; } void ObjCProtocolDecl::startDefinition() { @@ -2037,6 +2039,33 @@ 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 //===----------------------------------------------------------------------===// diff --git a/clang/lib/AST/ODRDiagsEmitter.cpp b/clang/lib/AST/ODRDiagsEmitter.cpp --- a/clang/lib/AST/ODRDiagsEmitter.cpp +++ b/clang/lib/AST/ODRDiagsEmitter.cpp @@ -266,6 +266,65 @@ 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) { @@ -1533,3 +1592,92 @@ } 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) { + // Already handled. + case EndOfClass: + case Other: + // Cannot be contained by ObjCProtocolDecl, invalid in this context. + 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; +} diff --git a/clang/lib/AST/ODRHash.cpp b/clang/lib/AST/ODRHash.cpp --- a/clang/lib/AST/ODRHash.cpp +++ b/clang/lib/AST/ODRHash.cpp @@ -628,6 +628,31 @@ } +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 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(); diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -9434,7 +9434,8 @@ 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 @@ -9480,6 +9481,16 @@ } } + // 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. @@ -9564,7 +9575,7 @@ } if (OdrMergeFailures.empty() && FunctionOdrMergeFailures.empty() && - EnumOdrMergeFailures.empty()) + EnumOdrMergeFailures.empty() && ObjCProtocolOdrMergeFailures.empty()) return; // Ensure we don't accidentally recursively enter deserialization while @@ -9635,6 +9646,25 @@ (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; + + 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() { diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp --- a/clang/lib/Serialization/ASTReaderDecl.cpp +++ b/clang/lib/Serialization/ASTReaderDecl.cpp @@ -1328,18 +1328,23 @@ 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? + Reader.MergedDeclContexts.insert( + std::make_pair(NewDD.Definition, DD.Definition)); + Reader.mergeDefinitionVisibility(DD.Definition, NewDD.Definition); + + if (D->getODRHash() != NewDD.ODRHash) + Reader.PendingObjCProtocolOdrMergeFailures[DD.Definition].push_back( + {NewDD.Definition, &NewDD}); } void ASTDeclReader::VisitObjCProtocolDecl(ObjCProtocolDecl *PD) { diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp --- a/clang/lib/Serialization/ASTWriterDecl.cpp +++ b/clang/lib/Serialization/ASTWriterDecl.cpp @@ -828,6 +828,7 @@ Record.AddDeclRef(I); for (const auto &PL : D->protocol_locs()) Record.AddSourceLocation(PL); + Record.push_back(D->getODRHash()); } Code = serialization::DECL_OBJC_PROTOCOL; diff --git a/clang/test/Modules/compare-objc-protocol.m b/clang/test/Modules/compare-objc-protocol.m new file mode 100644 --- /dev/null +++ b/clang/test/Modules/compare-objc-protocol.m @@ -0,0 +1,113 @@ +// 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 @end +#elif defined(SECOND) +@protocol CompareForwardDeclaration1 @end +@protocol CompareForwardDeclaration2; +#else +id compareForwardDeclaration1; +id compareForwardDeclaration2; +#endif + +#if defined(FIRST) +@protocol CompareMatchingConformingProtocols @end +@protocol ForwardProtocol; +@protocol CompareMatchingConformingForwardProtocols @end + +@protocol CompareProtocolPresence1 @end +@protocol CompareProtocolPresence2 @end + +@protocol CompareDifferentProtocols @end +@protocol CompareProtocolOrder @end +#elif defined(SECOND) +@protocol CompareMatchingConformingProtocols @end +@protocol ForwardProtocol @end +@protocol CompareMatchingConformingForwardProtocols @end + +@protocol CompareProtocolPresence1 @end +@protocol CompareProtocolPresence2 @end + +@protocol CompareDifferentProtocols @end +@protocol CompareProtocolOrder @end +#else +id compareMatchingConformingProtocols; +id compareMatchingConformingForwardProtocols; + +id 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; +// 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; +// 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; +// 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