From 632796131d6a3adf49f8e00ec950e8516419ab36 Mon Sep 17 00:00:00 2001 From: Richard Trieu Date: Sat, 30 Sep 2017 02:19:17 +0000 Subject: [PATCH] [ODRHash] Add base classes to hashing CXXRecordDecl. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@314581 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Basic/DiagnosticSerializationKinds.td | 21 +++ include/clang/Serialization/ASTReader.h | 5 +- lib/AST/ODRHash.cpp | 8 ++ lib/Serialization/ASTReader.cpp | 124 +++++++++++++++++- lib/Serialization/ASTReaderDecl.cpp | 3 +- test/Modules/odr_hash.cpp | 120 +++++++++++++++++ 6 files changed, 277 insertions(+), 4 deletions(-) diff --git a/include/clang/Basic/DiagnosticSerializationKinds.td b/include/clang/Basic/DiagnosticSerializationKinds.td index ca755bd578..3f0a14c59f 100644 --- a/include/clang/Basic/DiagnosticSerializationKinds.td +++ b/include/clang/Basic/DiagnosticSerializationKinds.td @@ -122,6 +122,27 @@ def note_second_module_difference : Note< def err_module_odr_violation_different_instantiations : Error< "instantiation of %q0 is different in different modules">; +def err_module_odr_violation_definition_data : Error < + "%q0 has different definitions in different modules; first difference is " + "%select{definition in module '%2'|defined here}1 found " + "%select{" + "%4 base %plural{1:class|:classes}4|" + "%4 virtual base %plural{1:class|:classes}4|" + "%ordinal4 base class with type %5|" + "%ordinal4 %select{non-virtual|virtual}5 base class %6|" + "%ordinal4 base class %5 with " + "%select{public|protected|private|no}6 access specifier}3">; + +def note_module_odr_violation_definition_data : Note < + "but in '%0' found " + "%select{" + "%2 base %plural{1:class|:classes}2|" + "%2 virtual base %plural{1:class|:classes}2|" + "%ordinal2 base class with different type %3|" + "%ordinal2 %select{non-virtual|virtual}3 base class %4|" + "%ordinal2 base class %3 with " + "%select{public|protected|private|no}4 access specifier}1">; + def err_module_odr_violation_template_parameter : Error < "%q0 has different definitions in different modules; first difference is " "%select{definition in module '%2'|defined here}1 found " diff --git a/include/clang/Serialization/ASTReader.h b/include/clang/Serialization/ASTReader.h index f9a026067e..9c5ad00691 100644 --- a/include/clang/Serialization/ASTReader.h +++ b/include/clang/Serialization/ASTReader.h @@ -1043,8 +1043,11 @@ private: /// once recursing loading has been completed. llvm::SmallVector PendingOdrMergeChecks; + using DataPointers = + std::pair; + /// \brief Record definitions in which we found an ODR violation. - llvm::SmallDenseMap, 2> + llvm::SmallDenseMap, 2> PendingOdrMergeFailures; /// \brief DeclContexts in which we have diagnosed an ODR violation. diff --git a/lib/AST/ODRHash.cpp b/lib/AST/ODRHash.cpp index fb8eade3c0..17c95f2a0a 100644 --- a/lib/AST/ODRHash.cpp +++ b/lib/AST/ODRHash.cpp @@ -456,6 +456,14 @@ void ODRHash::AddCXXRecordDecl(const CXXRecordDecl *Record) { if (TD) { AddTemplateParameterList(TD->getTemplateParameters()); } + + ID.AddInteger(Record->getNumBases()); + auto Bases = Record->bases(); + for (auto Base : Bases) { + AddQualType(Base.getType()); + ID.AddInteger(Base.isVirtual()); + ID.AddInteger(Base.getAccessSpecifierAsWritten()); + } } void ODRHash::AddDecl(const Decl *D) { diff --git a/lib/Serialization/ASTReader.cpp b/lib/Serialization/ASTReader.cpp index 46823030d3..7d745447a8 100644 --- a/lib/Serialization/ASTReader.cpp +++ b/lib/Serialization/ASTReader.cpp @@ -9190,7 +9190,8 @@ void ASTReader::diagnoseOdrViolations() { Merge.first->decls_begin(); Merge.first->bases_begin(); Merge.first->vbases_begin(); - for (auto *RD : Merge.second) { + for (auto &RecordPair : Merge.second) { + auto *RD = RecordPair.first; RD->decls_begin(); RD->bases_begin(); RD->vbases_begin(); @@ -9296,13 +9297,132 @@ void ASTReader::diagnoseOdrViolations() { bool Diagnosed = false; CXXRecordDecl *FirstRecord = Merge.first; std::string FirstModule = getOwningModuleNameForDiagnostic(FirstRecord); - for (CXXRecordDecl *SecondRecord : Merge.second) { + for (auto &RecordPair : Merge.second) { + CXXRecordDecl *SecondRecord = RecordPair.first; // Multiple different declarations got merged together; tell the user // where they came from. if (FirstRecord == SecondRecord) continue; std::string SecondModule = getOwningModuleNameForDiagnostic(SecondRecord); + + auto *FirstDD = FirstRecord->DefinitionData; + auto *SecondDD = RecordPair.second; + + assert(FirstDD && SecondDD && "Definitions without DefinitionData"); + + // Diagnostics from DefinitionData are emitted here. + if (FirstDD != SecondDD) { + enum ODRDefinitionDataDifference { + NumBases, + NumVBases, + BaseType, + BaseVirtual, + BaseAccess, + }; + auto ODRDiagError = [FirstRecord, &FirstModule, + this](SourceLocation Loc, SourceRange Range, + ODRDefinitionDataDifference DiffType) { + return Diag(Loc, diag::err_module_odr_violation_definition_data) + << FirstRecord << FirstModule.empty() << FirstModule << Range + << DiffType; + }; + auto ODRDiagNote = [&SecondModule, + this](SourceLocation Loc, SourceRange Range, + ODRDefinitionDataDifference DiffType) { + return Diag(Loc, diag::note_module_odr_violation_definition_data) + << SecondModule << Range << DiffType; + }; + + ODRHash Hash; + auto ComputeQualTypeODRHash = [&Hash](QualType Ty) { + Hash.clear(); + Hash.AddQualType(Ty); + return Hash.CalculateHash(); + }; + + unsigned FirstNumBases = FirstDD->NumBases; + unsigned FirstNumVBases = FirstDD->NumVBases; + unsigned SecondNumBases = SecondDD->NumBases; + unsigned SecondNumVBases = SecondDD->NumVBases; + + auto GetSourceRange = [](struct CXXRecordDecl::DefinitionData *DD) { + unsigned NumBases = DD->NumBases; + if (NumBases == 0) return SourceRange(); + auto bases = DD->bases(); + return SourceRange(bases[0].getLocStart(), + bases[NumBases - 1].getLocEnd()); + }; + + if (FirstNumBases != SecondNumBases) { + ODRDiagError(FirstRecord->getLocation(), GetSourceRange(FirstDD), + NumBases) + << FirstNumBases; + ODRDiagNote(SecondRecord->getLocation(), GetSourceRange(SecondDD), + NumBases) + << SecondNumBases; + Diagnosed = true; + break; + } + + if (FirstNumVBases != SecondNumVBases) { + ODRDiagError(FirstRecord->getLocation(), GetSourceRange(FirstDD), + NumVBases) + << FirstNumVBases; + ODRDiagNote(SecondRecord->getLocation(), GetSourceRange(SecondDD), + NumVBases) + << SecondNumVBases; + Diagnosed = true; + break; + } + + auto FirstBases = FirstDD->bases(); + auto SecondBases = SecondDD->bases(); + unsigned i = 0; + for (i = 0; i < FirstNumBases; ++i) { + auto FirstBase = FirstBases[i]; + auto SecondBase = SecondBases[i]; + if (ComputeQualTypeODRHash(FirstBase.getType()) != + ComputeQualTypeODRHash(SecondBase.getType())) { + ODRDiagError(FirstRecord->getLocation(), FirstBase.getSourceRange(), + BaseType) + << (i + 1) << FirstBase.getType(); + ODRDiagNote(SecondRecord->getLocation(), + SecondBase.getSourceRange(), BaseType) + << (i + 1) << SecondBase.getType(); + break; + } + + if (FirstBase.isVirtual() != SecondBase.isVirtual()) { + ODRDiagError(FirstRecord->getLocation(), FirstBase.getSourceRange(), + BaseVirtual) + << (i + 1) << FirstBase.isVirtual() << FirstBase.getType(); + ODRDiagNote(SecondRecord->getLocation(), + SecondBase.getSourceRange(), BaseVirtual) + << (i + 1) << SecondBase.isVirtual() << SecondBase.getType(); + break; + } + + if (FirstBase.getAccessSpecifierAsWritten() != + SecondBase.getAccessSpecifierAsWritten()) { + ODRDiagError(FirstRecord->getLocation(), FirstBase.getSourceRange(), + BaseAccess) + << (i + 1) << FirstBase.getType() + << (int)FirstBase.getAccessSpecifierAsWritten(); + ODRDiagNote(SecondRecord->getLocation(), + SecondBase.getSourceRange(), BaseAccess) + << (i + 1) << SecondBase.getType() + << (int)SecondBase.getAccessSpecifierAsWritten(); + break; + } + } + + if (i != FirstNumBases) { + Diagnosed = true; + break; + } + } + using DeclHashes = llvm::SmallVector, 4>; const ClassTemplateDecl *FirstTemplate = diff --git a/lib/Serialization/ASTReaderDecl.cpp b/lib/Serialization/ASTReaderDecl.cpp index 46164a06c3..1283b006df 100644 --- a/lib/Serialization/ASTReaderDecl.cpp +++ b/lib/Serialization/ASTReaderDecl.cpp @@ -1755,7 +1755,8 @@ void ASTDeclReader::MergeDefinitionData( } if (DetectedOdrViolation) - Reader.PendingOdrMergeFailures[DD.Definition].push_back(MergeDD.Definition); + Reader.PendingOdrMergeFailures[DD.Definition].push_back( + {MergeDD.Definition, &MergeDD}); } void ASTDeclReader::ReadCXXRecordDefinition(CXXRecordDecl *D, bool Update) { diff --git a/test/Modules/odr_hash.cpp b/test/Modules/odr_hash.cpp index a3ac510c32..8ff95d2566 100644 --- a/test/Modules/odr_hash.cpp +++ b/test/Modules/odr_hash.cpp @@ -1577,6 +1577,126 @@ using TemplateParameters::S6; #endif } // namespace TemplateParameters +namespace BaseClass { +#if defined(FIRST) +struct B1 {}; +struct S1 : B1 {}; +#elif defined(SECOND) +struct S1 {}; +#else +S1 s1; +// expected-error@second.h:* {{'BaseClass::S1' has different definitions in different modules; first difference is definition in module 'SecondModule' found 0 base classes}} +// expected-note@first.h:* {{but in 'FirstModule' found 1 base class}} +#endif + +#if defined(FIRST) +struct S2 {}; +#elif defined(SECOND) +struct B2 {}; +struct S2 : virtual B2 {}; +#else +S2 s2; +// expected-error@second.h:* {{'BaseClass::S2' has different definitions in different modules; first difference is definition in module 'SecondModule' found 1 base class}} +// expected-note@first.h:* {{but in 'FirstModule' found 0 base classes}} +#endif + +#if defined(FIRST) +struct B3a {}; +struct S3 : B3a {}; +#elif defined(SECOND) +struct B3b {}; +struct S3 : virtual B3b {}; +#else +S3 s3; +// expected-error@second.h:* {{'BaseClass::S3' has different definitions in different modules; first difference is definition in module 'SecondModule' found 1 virtual base class}} +// expected-note@first.h:* {{but in 'FirstModule' found 0 virtual base classes}} +#endif + +#if defined(FIRST) +struct B4a {}; +struct S4 : B4a {}; +#elif defined(SECOND) +struct B4b {}; +struct S4 : B4b {}; +#else +S4 s4; +// expected-error@second.h:* {{'BaseClass::S4' has different definitions in different modules; first difference is definition in module 'SecondModule' found 1st base class with type 'BaseClass::B4b'}} +// expected-note@first.h:* {{but in 'FirstModule' found 1st base class with different type 'BaseClass::B4a'}} +#endif + +#if defined(FIRST) +struct B5a {}; +struct S5 : virtual B5a {}; +#elif defined(SECOND) +struct B5a {}; +struct S5 : B5a {}; +#else +S5 s5; +// expected-error@second.h:* {{'BaseClass::S5' has different definitions in different modules; first difference is definition in module 'SecondModule' found 0 virtual base classes}} +// expected-note@first.h:* {{but in 'FirstModule' found 1 virtual base class}} +#endif + +#if defined(FIRST) +struct B6a {}; +struct S6 : B6a {}; +#elif defined(SECOND) +struct B6a {}; +struct S6 : virtual B6a {}; +#else +S6 s6; +// expected-error@second.h:* {{'BaseClass::S6' has different definitions in different modules; first difference is definition in module 'SecondModule' found 1 virtual base class}} +// expected-note@first.h:* {{but in 'FirstModule' found 0 virtual base classes}} +#endif + +#if defined(FIRST) +struct B7a {}; +struct S7 : protected B7a {}; +#elif defined(SECOND) +struct B7a {}; +struct S7 : B7a {}; +#else +S7 s7; +// expected-error@second.h:* {{'BaseClass::S7' has different definitions in different modules; first difference is definition in module 'SecondModule' found 1st base class 'BaseClass::B7a' with no access specifier}} +// expected-note@first.h:* {{but in 'FirstModule' found 1st base class 'BaseClass::B7a' with protected access specifier}} +#endif + +#if defined(FIRST) +struct B8a {}; +struct S8 : public B8a {}; +#elif defined(SECOND) +struct B8a {}; +struct S8 : private B8a {}; +#else +S8 s8; +// expected-error@second.h:* {{'BaseClass::S8' has different definitions in different modules; first difference is definition in module 'SecondModule' found 1st base class 'BaseClass::B8a' with private access specifier}} +// expected-note@first.h:* {{but in 'FirstModule' found 1st base class 'BaseClass::B8a' with public access specifier}} +#endif + +#if defined(FIRST) +struct B9a {}; +struct S9 : private B9a {}; +#elif defined(SECOND) +struct B9a {}; +struct S9 : public B9a {}; +#else +S9 s9; +// expected-error@second.h:* {{'BaseClass::S9' has different definitions in different modules; first difference is definition in module 'SecondModule' found 1st base class 'BaseClass::B9a' with public access specifier}} +// expected-note@first.h:* {{but in 'FirstModule' found 1st base class 'BaseClass::B9a' with private access specifier}} +#endif + +#if defined(FIRST) +struct B10a {}; +struct S10 : B10a {}; +#elif defined(SECOND) +struct B10a {}; +struct S10 : protected B10a {}; +#else +S10 s10; +// expected-error@second.h:* {{'BaseClass::S10' has different definitions in different modules; first difference is definition in module 'SecondModule' found 1st base class 'BaseClass::B10a' with protected access specifier}} +// expected-note@first.h:* {{but in 'FirstModule' found 1st base class 'BaseClass::B10a' with no access specifier}} +#endif +} // namespace BaseClass + // Interesting cases that should not cause errors. struct S should not error // while struct T should error at the access specifier mismatch at the end. namespace AllDecls { -- 2.40.0