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:31:13 PDT 2015
On Thu, Aug 6, 2015 at 11:29 AM, Richard Smith <metafoo at gmail.com> wrote:
> On Aug 6, 2015 11:01 AM, "David Blaikie" <dblaikie at gmail.com> wrote:
> >
> >
> >
> > 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?)
>
> 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 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/f69562db/attachment-0001.html>
More information about the cfe-commits
mailing list