From 0f4e2c4d0f46b5edb4159b2d982327475b797efe Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Thu, 6 Aug 2015 04:23:48 +0000 Subject: [PATCH] [modules] Defer setting up the lookup table for a DeclContext until we can determine the primary context, rather than sometimes registering the lookup table on the wrong context. This exposed a couple of bugs: * the odr violation check didn't deal properly with mergeable declarations if the declaration retained by name lookup wasn't in the canonical definition of the class * the (broken) RewriteDecl mechanism would emit two name lookup tables for the same DeclContext into the same module file (one as part of the rewritten declaration and one as a visible update for the old declaration) These are both fixed too. llvm-svn: 244192 --- clang/include/clang/Serialization/ASTReader.h | 35 ++++--- clang/lib/Serialization/ASTReader.cpp | 143 +++++++++++++------------- clang/lib/Serialization/ASTReaderDecl.cpp | 72 ++++++------- clang/lib/Serialization/ASTWriter.cpp | 4 + 4 files changed, 126 insertions(+), 128 deletions(-) diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h index de6f43f..333d8d9 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -495,19 +495,21 @@ private: llvm::DenseMap FileDeclIDs; // Updates for visible decls can occur for other contexts than just the - // TU, and when we read those update records, the actual context will not - // be available yet (unless it's the TU), so have this pending map using the - // ID as a key. It will be realized when the context is actually loaded. - typedef - SmallVector, 1> DeclContextVisibleUpdates; - typedef llvm::DenseMap - DeclContextVisibleUpdatesPending; + // TU, and when we read those update records, the actual context may not + // be available yet, so have this pending map using the ID as a key. It + // will be realized when the context is actually loaded. + struct PendingVisibleUpdate { + ModuleFile *Mod; + const unsigned char *Data; + unsigned BucketOffset; + }; + typedef SmallVector DeclContextVisibleUpdates; /// \brief Updates to the visible declarations of declaration contexts that /// haven't been loaded yet. - DeclContextVisibleUpdatesPending PendingVisibleUpdates; - + llvm::DenseMap + PendingVisibleUpdates; + /// \brief The set of C++ or Objective-C classes that have forward /// declarations that have not yet been linked to their definitions. llvm::SmallPtrSet PendingDefinitions; @@ -524,11 +526,14 @@ private: /// performed deduplication. llvm::SetVector PendingMergedDefinitionsToDeduplicate; - /// \brief Read the records that describe the contents of declcontexts. - bool ReadDeclContextStorage(ModuleFile &M, - llvm::BitstreamCursor &Cursor, - const std::pair &Offsets, - serialization::DeclContextInfo &Info); + /// \brief Read the record that describes the lexical contents of a DC. + bool ReadLexicalDeclContextStorage(ModuleFile &M, + llvm::BitstreamCursor &Cursor, + uint64_t Offset, DeclContext *DC); + /// \brief Read the record that describes the visible contents of a DC. + bool ReadVisibleDeclContextStorage(ModuleFile &M, + llvm::BitstreamCursor &Cursor, + uint64_t Offset, serialization::DeclID ID); /// \brief A vector containing identifiers that have already been /// loaded. diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index abc91e7..41294a8 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -955,48 +955,54 @@ ASTDeclContextNameLookupTrait::ReadData(internal_key_type, return std::make_pair(Start, Start + NumDecls); } -bool ASTReader::ReadDeclContextStorage(ModuleFile &M, - BitstreamCursor &Cursor, - const std::pair &Offsets, - DeclContextInfo &Info) { - SavedStreamPosition SavedPosition(Cursor); - // First the lexical decls. - if (Offsets.first != 0) { - Cursor.JumpToBit(Offsets.first); +bool ASTReader::ReadLexicalDeclContextStorage(ModuleFile &M, + BitstreamCursor &Cursor, + uint64_t Offset, + DeclContext *DC) { + assert(Offset != 0); - RecordData Record; - StringRef Blob; - unsigned Code = Cursor.ReadCode(); - unsigned RecCode = Cursor.readRecord(Code, Record, &Blob); - if (RecCode != DECL_CONTEXT_LEXICAL) { - Error("Expected lexical block"); - return true; - } + SavedStreamPosition SavedPosition(Cursor); + Cursor.JumpToBit(Offset); - Info.LexicalDecls = llvm::makeArrayRef( - reinterpret_cast(Blob.data()), - Blob.size() / sizeof(KindDeclIDPair)); + RecordData Record; + StringRef Blob; + unsigned Code = Cursor.ReadCode(); + unsigned RecCode = Cursor.readRecord(Code, Record, &Blob); + if (RecCode != DECL_CONTEXT_LEXICAL) { + Error("Expected lexical block"); + return true; } - // Now the lookup table. - if (Offsets.second != 0) { - Cursor.JumpToBit(Offsets.second); + M.DeclContextInfos[DC].LexicalDecls = llvm::makeArrayRef( + reinterpret_cast(Blob.data()), + Blob.size() / sizeof(KindDeclIDPair)); + DC->setHasExternalLexicalStorage(true); + return false; +} - RecordData Record; - StringRef Blob; - unsigned Code = Cursor.ReadCode(); - unsigned RecCode = Cursor.readRecord(Code, Record, &Blob); - if (RecCode != DECL_CONTEXT_VISIBLE) { - Error("Expected visible lookup table block"); - return true; - } - Info.NameLookupTableData = ASTDeclContextNameLookupTable::Create( - (const unsigned char *)Blob.data() + Record[0], - (const unsigned char *)Blob.data() + sizeof(uint32_t), - (const unsigned char *)Blob.data(), - ASTDeclContextNameLookupTrait(*this, M)); +bool ASTReader::ReadVisibleDeclContextStorage(ModuleFile &M, + BitstreamCursor &Cursor, + uint64_t Offset, + DeclID ID) { + assert(Offset != 0); + + SavedStreamPosition SavedPosition(Cursor); + Cursor.JumpToBit(Offset); + + RecordData Record; + StringRef Blob; + unsigned Code = Cursor.ReadCode(); + unsigned RecCode = Cursor.readRecord(Code, Record, &Blob); + if (RecCode != DECL_CONTEXT_VISIBLE) { + Error("Expected visible lookup table block"); + return true; } + // We can't safely determine the primary context yet, so delay attaching the + // lookup table until we're done with recursive deserialization. + unsigned BucketOffset = Record[0]; + PendingVisibleUpdates[ID].push_back(PendingVisibleUpdate{ + &M, (const unsigned char *)Blob.data(), BucketOffset}); return false; } @@ -2503,20 +2509,14 @@ ASTReader::ReadASTBlock(ModuleFile &F, unsigned ClientLoadCapabilities) { case UPDATE_VISIBLE: { unsigned Idx = 0; serialization::DeclID ID = ReadDeclID(F, Record, Idx); - ASTDeclContextNameLookupTable *Table = - ASTDeclContextNameLookupTable::Create( - (const unsigned char *)Blob.data() + Record[Idx++], - (const unsigned char *)Blob.data() + sizeof(uint32_t), - (const unsigned char *)Blob.data(), - ASTDeclContextNameLookupTrait(*this, F)); - if (Decl *D = GetExistingDecl(ID)) { - auto *DC = cast(D); - DC->getPrimaryContext()->setHasExternalVisibleStorage(true); - auto *&LookupTable = F.DeclContextInfos[DC].NameLookupTableData; - delete LookupTable; - LookupTable = Table; - } else - PendingVisibleUpdates[ID].push_back(std::make_pair(Table, &F)); + auto *Data = (const unsigned char*)Blob.data(); + unsigned BucketOffset = Record[Idx++]; + PendingVisibleUpdates[ID].push_back( + PendingVisibleUpdate{&F, Data, BucketOffset}); + // If we've already loaded the decl, perform the updates when we finish + // loading this block. + if (Decl *D = GetExistingDecl(ID)) + PendingUpdateRecords.push_back(std::make_pair(ID, D)); break; } @@ -8331,21 +8331,26 @@ void ASTReader::diagnoseOdrViolations() { if (Found) continue; + // Quick check failed, time to do the slow thing. Note, we can't just + // look up the name of D in CanonDef here, because the member that is + // in CanonDef might not be found by name lookup (it might have been + // replaced by a more recent declaration in the lookup table), and we + // can't necessarily find it in the redeclaration chain because it might + // be merely mergeable, not redeclarable. llvm::SmallVector Candidates; - DeclContext::lookup_result R = CanonDef->lookup(D->getDeclName()); - for (DeclContext::lookup_iterator I = R.begin(), E = R.end(); - !Found && I != E; ++I) { - for (auto RI : (*I)->redecls()) { - if (RI->getLexicalDeclContext() == CanonDef) { - // This declaration is present in the canonical definition. If it's - // in the same redecl chain, it's the one we're looking for. - if (RI->getCanonicalDecl() == DCanon) - Found = true; - else - Candidates.push_back(cast(RI)); - break; - } + for (auto *CanonMember : CanonDef->decls()) { + if (CanonMember->getCanonicalDecl() == DCanon) { + // This can happen if the declaration is merely mergeable and not + // actually redeclarable (we looked for redeclarations earlier). + // + // FIXME: We should be able to detect this more efficiently, without + // pulling in all of the members of CanonDef. + Found = true; + break; } + if (auto *ND = dyn_cast(CanonMember)) + if (ND->getDeclName() == D->getDeclName()) + Candidates.push_back(ND); } if (!Found) { @@ -8454,11 +8459,11 @@ void ASTReader::FinishedDeserializing() { } } - diagnoseOdrViolations(); - if (ReadTimer) ReadTimer->stopTimer(); + diagnoseOdrViolations(); + // We are not in recursive loading, so it's safe to pass the "interesting" // decls to the consumer. if (Consumer) @@ -8527,14 +8532,4 @@ ASTReader::ASTReader(Preprocessor &PP, ASTContext &Context, ASTReader::~ASTReader() { if (OwnsDeserializationListener) delete DeserializationListener; - - for (DeclContextVisibleUpdatesPending::iterator - I = PendingVisibleUpdates.begin(), - E = PendingVisibleUpdates.end(); - I != E; ++I) { - for (DeclContextVisibleUpdates::iterator J = I->second.begin(), - F = I->second.end(); - J != F; ++J) - delete J->first; - } } diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp index 06f7aca..0585234 100644 --- a/clang/lib/Serialization/ASTReaderDecl.cpp +++ b/clang/lib/Serialization/ASTReaderDecl.cpp @@ -3339,37 +3339,13 @@ Decl *ASTReader::ReadDeclRecord(DeclID ID) { // If this declaration is also a declaration context, get the // offsets for its tables of lexical and visible declarations. if (DeclContext *DC = dyn_cast(D)) { - // FIXME: This should really be - // DeclContext *LookupDC = DC->getPrimaryContext(); - // but that can walk the redeclaration chain, which might not work yet. - DeclContext *LookupDC = DC; - if (isa(DC)) - LookupDC = DC->getPrimaryContext(); std::pair Offsets = Reader.VisitDeclContext(DC); - if (Offsets.first || Offsets.second) { - if (Offsets.first != 0) - DC->setHasExternalLexicalStorage(true); - if (Offsets.second != 0) - LookupDC->setHasExternalVisibleStorage(true); - if (ReadDeclContextStorage(*Loc.F, DeclsCursor, Offsets, - Loc.F->DeclContextInfos[DC])) - return nullptr; - } - - // Now add the pending visible updates for this decl context, if it has any. - DeclContextVisibleUpdatesPending::iterator I = - PendingVisibleUpdates.find(ID); - if (I != PendingVisibleUpdates.end()) { - // There are updates. This means the context has external visible - // storage, even if the original stored version didn't. - LookupDC->setHasExternalVisibleStorage(true); - for (const auto &Update : I->second) { - DeclContextInfo &Info = Update.second->DeclContextInfos[DC]; - delete Info.NameLookupTableData; - Info.NameLookupTableData = Update.first; - } - PendingVisibleUpdates.erase(I); - } + if (Offsets.first && + ReadLexicalDeclContextStorage(*Loc.F, DeclsCursor, Offsets.first, DC)) + return nullptr; + if (Offsets.second && + ReadVisibleDeclContextStorage(*Loc.F, DeclsCursor, Offsets.second, ID)) + return nullptr; } assert(Idx == Record.size()); @@ -3392,17 +3368,37 @@ Decl *ASTReader::ReadDeclRecord(DeclID ID) { } void ASTReader::loadDeclUpdateRecords(serialization::DeclID ID, Decl *D) { + // Load the pending visible updates for this decl context, if it has any. + auto I = PendingVisibleUpdates.find(ID); + if (I != PendingVisibleUpdates.end()) { + auto VisibleUpdates = std::move(I->second); + PendingVisibleUpdates.erase(I); + + auto *DC = cast(D)->getPrimaryContext(); + for (const PendingVisibleUpdate &Update : VisibleUpdates) { + auto *&LookupTable = Update.Mod->DeclContextInfos[DC].NameLookupTableData; + assert(!LookupTable && "multiple lookup tables for DC in module"); + LookupTable = reader::ASTDeclContextNameLookupTable::Create( + Update.Data + Update.BucketOffset, + Update.Data + sizeof(uint32_t), + Update.Data, + reader::ASTDeclContextNameLookupTrait(*this, *Update.Mod)); + } + DC->setHasExternalVisibleStorage(true); + } + // The declaration may have been modified by files later in the chain. // If this is the case, read the record containing the updates from each file // and pass it to ASTDeclReader to make the modifications. DeclUpdateOffsetsMap::iterator UpdI = DeclUpdateOffsets.find(ID); if (UpdI != DeclUpdateOffsets.end()) { - FileOffsetsTy &UpdateOffsets = UpdI->second; + auto UpdateOffsets = std::move(UpdI->second); + DeclUpdateOffsets.erase(UpdI); + bool WasInteresting = isConsumerInterestedIn(D, false); - for (FileOffsetsTy::iterator - I = UpdateOffsets.begin(), E = UpdateOffsets.end(); I != E; ++I) { - ModuleFile *F = I->first; - uint64_t Offset = I->second; + for (auto &FileAndOffset : UpdateOffsets) { + ModuleFile *F = FileAndOffset.first; + uint64_t Offset = FileAndOffset.second; llvm::BitstreamCursor &Cursor = F->DeclsCursor; SavedStreamPosition SavedPosition(Cursor); Cursor.JumpToBit(Offset); @@ -3817,10 +3813,8 @@ void ASTDeclReader::UpdateDecl(Decl *D, ModuleFile &ModuleFile, // Visible update is handled separately. uint64_t LexicalOffset = Record[Idx++]; if (!HadRealDefinition && LexicalOffset) { - RD->setHasExternalLexicalStorage(true); - Reader.ReadDeclContextStorage(ModuleFile, ModuleFile.DeclsCursor, - std::make_pair(LexicalOffset, 0), - ModuleFile.DeclContextInfos[RD]); + Reader.ReadLexicalDeclContextStorage(ModuleFile, ModuleFile.DeclsCursor, + LexicalOffset, RD); Reader.PendingFakeDefinitionData.erase(OldDD); } diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 7dd16e9..1199803 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -3754,6 +3754,9 @@ uint64_t ASTWriter::WriteDeclContextVisibleBlock(ASTContext &Context, /// (in C++), for namespaces, and for classes with forward-declared unscoped /// enumeration members (in C++11). void ASTWriter::WriteDeclContextVisibleUpdate(const DeclContext *DC) { + if (isRewritten(cast(DC))) + return; + StoredDeclsMap *Map = DC->getLookupPtr(); if (!Map || Map->empty()) return; @@ -5705,6 +5708,7 @@ void ASTWriter::AddedVisibleDecl(const DeclContext *DC, const Decl *D) { if (!(!D->isFromASTFile() && cast(DC)->isFromASTFile())) return; // Not a source decl added to a DeclContext from PCH. + assert(DC == DC->getPrimaryContext() && "added to non-primary context"); assert(!getDefinitiveDeclContext(DC) && "DeclContext not definitive!"); assert(!WritingAST && "Already writing the AST!"); UpdatedDeclContexts.insert(DC); -- 2.7.4