r244192 - [modules] Defer setting up the lookup table for a DeclContext until we can
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 6 11:29:07 PDT 2015
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.
>> 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/c3b535f3/attachment-0001.html>
More information about the cfe-commits
mailing list