r244192 - [modules] Defer setting up the lookup table for a DeclContext until we can
David Blaikie via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 6 11:01:42 PDT 2015
On Wed, Aug 5, 2015 at 9:23 PM, Richard Smith <richard-llvm at 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?)
> 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 at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150806/c7ca9abf/attachment-0001.html>
More information about the cfe-commits
mailing list