On Thu, Aug 6, 2015 at 11:29 AM, Richard Smith <meta...@gmail.com> wrote:
> On Aug 6, 2015 11:01 AM, "David Blaikie" <dblai...@gmail.com> wrote: > > > > > > > > On Wed, Aug 5, 2015 at 9:23 PM, Richard Smith < > richard-l...@metafoo.co.uk> wrote: > >> > >> Author: rsmith > >> Date: Wed Aug 5 23:23:48 2015 > >> New Revision: 244192 > >> > >> URL: http://llvm.org/viewvc/llvm-project?rev=244192&view=rev > >> Log: > >> [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) > > > > > > Is it practical to provide test cases for these? (I guess the second one > is perhaps just a perf problem? The first one sounds like a possible > functional bug, though?) > > The existing test suite failed without these fixes, after the other change > described above. > Ah, OK, I get it now. Thanks! > >> These are both fixed too. > >> > >> Modified: > >> cfe/trunk/include/clang/Serialization/ASTReader.h > >> cfe/trunk/lib/Serialization/ASTReader.cpp > >> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp > >> cfe/trunk/lib/Serialization/ASTWriter.cpp > >> > >> Modified: cfe/trunk/include/clang/Serialization/ASTReader.h > >> URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTReader.h?rev=244192&r1=244191&r2=244192&view=diff > >> > ============================================================================== > >> --- cfe/trunk/include/clang/Serialization/ASTReader.h (original) > >> +++ cfe/trunk/include/clang/Serialization/ASTReader.h Wed Aug 5 > 23:23:48 2015 > >> @@ -495,19 +495,21 @@ private: > >> llvm::DenseMap<FileID, FileDeclsInfo> 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<std::pair<serialization::reader::ASTDeclContextNameLookupTable > *, > >> - ModuleFile*>, 1> DeclContextVisibleUpdates; > >> - typedef llvm::DenseMap<serialization::DeclID, > DeclContextVisibleUpdates> > >> - 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<PendingVisibleUpdate, 1> > DeclContextVisibleUpdates; > >> > >> /// \brief Updates to the visible declarations of declaration > contexts that > >> /// haven't been loaded yet. > >> - DeclContextVisibleUpdatesPending PendingVisibleUpdates; > >> - > >> + llvm::DenseMap<serialization::DeclID, DeclContextVisibleUpdates> > >> + 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<Decl *, 4> PendingDefinitions; > >> @@ -524,11 +526,14 @@ private: > >> /// performed deduplication. > >> llvm::SetVector<NamedDecl*> PendingMergedDefinitionsToDeduplicate; > >> > >> - /// \brief Read the records that describe the contents of > declcontexts. > >> - bool ReadDeclContextStorage(ModuleFile &M, > >> - llvm::BitstreamCursor &Cursor, > >> - const std::pair<uint64_t, uint64_t> > &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. > >> > >> Modified: cfe/trunk/lib/Serialization/ASTReader.cpp > >> URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=244192&r1=244191&r2=244192&view=diff > >> > ============================================================================== > >> --- cfe/trunk/lib/Serialization/ASTReader.cpp (original) > >> +++ cfe/trunk/lib/Serialization/ASTReader.cpp Wed Aug 5 23:23:48 2015 > >> @@ -955,48 +955,54 @@ ASTDeclContextNameLookupTrait::ReadData( > >> return std::make_pair(Start, Start + NumDecls); > >> } > >> > >> -bool ASTReader::ReadDeclContextStorage(ModuleFile &M, > >> - BitstreamCursor &Cursor, > >> - const std::pair<uint64_t, uint64_t> > &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<const KindDeclIDPair *>(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<const KindDeclIDPair *>(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, u > >> 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<DeclContext>(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<const NamedDecl*, 4> 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<NamedDecl>(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<NamedDecl>(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, A > >> 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; > >> - } > >> } > >> > >> Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp > >> URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReaderDecl.cpp?rev=244192&r1=244191&r2=244192&view=diff > >> > ============================================================================== > >> --- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original) > >> +++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Wed Aug 5 23:23:48 > 2015 > >> @@ -3339,37 +3339,13 @@ Decl *ASTReader::ReadDeclRecord(DeclID I > >> // If this declaration is also a declaration context, get the > >> // offsets for its tables of lexical and visible declarations. > >> if (DeclContext *DC = dyn_cast<DeclContext>(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<NamespaceDecl>(DC)) > >> - LookupDC = DC->getPrimaryContext(); > >> std::pair<uint64_t, uint64_t> 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 I > >> } > >> > >> 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<DeclContext>(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, > >> // 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); > >> } > >> > >> > >> Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp > >> URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=244192&r1=244191&r2=244192&view=diff > >> > ============================================================================== > >> --- cfe/trunk/lib/Serialization/ASTWriter.cpp (original) > >> +++ cfe/trunk/lib/Serialization/ASTWriter.cpp Wed Aug 5 23:23:48 2015 > >> @@ -3754,6 +3754,9 @@ uint64_t ASTWriter::WriteDeclContextVisi > >> /// (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<Decl>(DC))) > >> + return; > >> + > >> StoredDeclsMap *Map = DC->getLookupPtr(); > >> if (!Map || Map->empty()) > >> return; > >> @@ -5705,6 +5708,7 @@ void ASTWriter::AddedVisibleDecl(const D > >> if (!(!D->isFromASTFile() && cast<Decl>(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); > >> > >> > >> _______________________________________________ > >> cfe-commits mailing list > >> cfe-commits@lists.llvm.org > >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > > > > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
