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