<p dir="ltr"><br>
On Mar 24, 2015 5:42 PM, "Chandler Carruth" <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>> wrote:<br>
><br>
> Author: chandlerc<br>
> Date: Tue Mar 24 19:34:51 2015<br>
> New Revision: 233156<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=233156&view=rev">http://llvm.org/viewvc/llvm-project?rev=233156&view=rev</a><br>
> Log:<br>
> [Modules] When writing out the on-disk hash table for the decl context<br>
> lookup tables, we need to establish a stable ordering for constructing<br>
> the hash table. This is trickier than it might seem.<br>
><br>
> Most of these cases are easily handled by sorting the lookup results<br>
> associated with a specific name that has an identifier. However for<br>
> constructors and conversion functions, the story is more complicated.<br>
> Here we need to merge all of the constructors or conversion functions<br>
> together and this merge needs to be stable. We don't have any stable<br>
> ordering for either constructors or conversion functions as both would<br>
> require a stable ordering across types.<br>
><br>
> Instead, when we have constructors or conversion functions in the<br>
> results, we reconstruct a stable order by walking the decl context in<br>
> lexical order and merging them in the order their particular declaration<br>
> names are encountered. </p>
<p dir="ltr">I assume for user written ctors and conversion functions they appear in the order the user wrote them. What happens for implicitly defined ctors? Is their order guaranteed?</p>
<p dir="ltr">> This doesn't generalize as there might be found<br>
> declaration names which don't actually occur within the lexical context,<br>
> but for constructors and conversion functions it is safe. It does<br>
> require loading the entire decl context if necessary to establish the<br>
> ordering but there doesn't seem to be a meaningful way around that.<br>
><br>
> Many thanks to Richard for talking through all of the design choices<br>
> here. While I wrote the code, he guided all the actual decisions about<br>
> how to establish the order of things.<br>
><br>
> No test case yet because the test case I have doesn't pass yet -- there<br>
> are still more sources of non-determinism. However, this is complex<br>
> enough that I wanted it to go into its own commit in case it causes some<br>
> unforseen issue or needs to be reverted.<br>
><br>
> Modified:<br>
> cfe/trunk/lib/Serialization/ASTWriter.cpp<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=233156&r1=233155&r2=233156&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=233156&r1=233155&r2=233156&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)<br>
> +++ cfe/trunk/lib/Serialization/ASTWriter.cpp Tue Mar 24 19:34:51 2015<br>
> @@ -3761,15 +3761,34 @@ ASTWriter::GenerateNameLookupTable(const<br>
> !DC->HasLazyExternalLexicalLookups &&<br>
> "must call buildLookups first");<br>
><br>
> + // Create the on-disk hash table representation.<br>
> llvm::OnDiskChainedHashTableGenerator<ASTDeclContextNameLookupTrait><br>
> Generator;<br>
> ASTDeclContextNameLookupTrait Trait(*this);<br>
><br>
> - // Create the on-disk hash table representation.<br>
> + // Store the sortable lookup results -- IE, those with meaningful names. We<br>
> + // will sort them by the DeclarationName in order to stabilize the ordering<br>
> + // of the hash table. We can't do this for constructors or conversion<br>
> + // functions which are handled separately.<br>
> + SmallVector<std::pair<DeclarationName, DeclContext::lookup_result>, 16><br>
> + NamedResults;<br>
> +<br>
> + // We can't directly construct a nonce constructor or conversion name without<br>
> + // tripping asserts, so we just recall the first one we see. Only the kind<br>
> + // will actually be serialized.<br>
> DeclarationName ConstructorName;<br>
> DeclarationName ConversionName;<br>
> - SmallVector<NamedDecl *, 8> ConstructorDecls;<br>
> - SmallVector<NamedDecl *, 4> ConversionDecls;<br>
> + // Retain a mapping from each actual declaration name to the results<br>
> + // associated with it. We use a map here because the order in which we<br>
> + // discover these lookup results isn't ordered. We have to re-establish<br>
> + // a stable ordering which we do by walking the children of the decl context,<br>
> + // and emitting these in the order in which their names first appeared. Note<br>
> + // that the names' first appearance may not be one of the results or a result<br>
> + // at all. We just use this to establish an ordering.<br>
> + llvm::SmallDenseMap<DeclarationName, DeclContext::lookup_result, 8><br>
> + ConstructorResults;<br>
> + llvm::SmallDenseMap<DeclarationName, DeclContext::lookup_result, 4><br>
> + ConversionResults;<br>
><br>
> visitLocalLookupResults(DC, [&](DeclarationName Name,<br>
> DeclContext::lookup_result Result) {<br>
> @@ -3786,34 +3805,90 @@ ASTWriter::GenerateNameLookupTable(const<br>
> // has the DeclarationName of the inherited constructors.<br>
> if (!ConstructorName)<br>
> ConstructorName = Name;<br>
> - ConstructorDecls.append(Result.begin(), Result.end());<br>
> + ConstructorResults.insert(std::make_pair(Name, Result));<br>
> return;<br>
><br>
> case DeclarationName::CXXConversionFunctionName:<br>
> if (!ConversionName)<br>
> ConversionName = Name;<br>
> - ConversionDecls.append(Result.begin(), Result.end());<br>
> + ConversionResults.insert(std::make_pair(Name, Result));<br>
> return;<br>
><br>
> default:<br>
> - break;<br>
> + NamedResults.push_back(std::make_pair(Name, Result));<br>
> }<br>
><br>
> - Generator.insert(Name, Result, Trait);<br>
> });<br>
><br>
> - // Add the constructors.<br>
> - if (!ConstructorDecls.empty()) {<br>
> + // Sort and emit the named results first. This is the easy case.<br>
> + std::sort(<br>
> + NamedResults.begin(), NamedResults.end(),<br>
> + [](const std::pair<DeclarationName, DeclContext::lookup_result> &LHS,<br>
> + const std::pair<DeclarationName, DeclContext::lookup_result> &RHS) {<br>
> + return LHS.first < RHS.first;<br>
> + });<br>
> + for (auto Results : NamedResults)<br>
> + Generator.insert(Results.first, Results.second, Trait);<br>
> +<br>
> + // We have to specially handle the constructor and conversion function<br>
> + // declarations found.<br>
> + if (ConstructorResults.size() == 1) {<br>
> + // A special case that is easy is when we have just one constructor<br>
> + // declaration name.<br>
> Generator.insert(ConstructorName,<br>
> - DeclContext::lookup_result(ConstructorDecls),<br>
> - Trait);<br>
> + ConstructorResults.lookup(ConstructorName), Trait);<br>
> + ConstructorResults.clear();<br>
> }<br>
> -<br>
> - // Add the conversion functions.<br>
> - if (!ConversionDecls.empty()) {<br>
> - Generator.insert(ConversionName,<br>
> - DeclContext::lookup_result(ConversionDecls),<br>
> + if (ConversionResults.size() == 1) {<br>
> + // A special case that is easy is when we have just one conversion function<br>
> + // declaration name.<br>
> + Generator.insert(ConversionName, ConversionResults.lookup(ConversionName),<br>
> Trait);<br>
> + ConversionResults.clear();<br>
> + }<br>
> + if (!ConstructorResults.empty() || !ConversionResults.empty()) {<br>
> + SmallVector<NamedDecl *, 8> ConstructorDecls;<br>
> + SmallVector<NamedDecl *, 8> ConversionDecls;<br>
> +<br>
> + // Walk the decls in the context and collapse the results in that order. We<br>
> + // handle both constructors and conversion functions in a single walk as<br>
> + // the walk is a relative cache-hostile linked list walk.<br>
> + for (Decl *ChildD : DC->decls())<br>
> + if (auto *ChildND = dyn_cast<NamedDecl>(ChildD)) {<br>
> + auto ConstructorResultsIt =<br>
> + ConstructorResults.find(ChildND->getDeclName());<br>
> + if (ConstructorResultsIt != ConstructorResults.end()) {<br>
> + ConstructorDecls.append(ConstructorResultsIt->second.begin(),<br>
> + ConstructorResultsIt->second.end());<br>
> + ConstructorResults.erase(ConstructorResultsIt);<br>
> + }<br>
> +<br>
> + auto ConversionResultsIt =<br>
> + ConversionResults.find(ChildND->getDeclName());<br>
> + if (ConversionResultsIt != ConversionResults.end()) {<br>
> + ConversionDecls.append(ConversionResultsIt->second.begin(),<br>
> + ConversionResultsIt->second.end());<br>
> + ConversionResults.erase(ConversionResultsIt);<br>
> + }<br>
> +<br>
> + // If we handle all of the results, we're done.<br>
> + if (ConstructorResults.empty() && ConversionResults.empty())<br>
> + break;<br>
> + }<br>
> + assert(ConstructorResults.empty() && "Have constructor lookup results not "<br>
> + "associated with a declaration name "<br>
> + "within the context!");<br>
> + assert(ConversionResults.empty() && "Have conversion function lookup "<br>
> + "results not associated with a "<br>
> + "declaration name within the context!");<br>
> +<br>
> + if (!ConstructorDecls.empty())<br>
> + Generator.insert(ConstructorName,<br>
> + DeclContext::lookup_result(ConstructorDecls), Trait);<br>
> +<br>
> + if (!ConversionDecls.empty())<br>
> + Generator.insert(ConversionName,<br>
> + DeclContext::lookup_result(ConversionDecls), Trait);<br>
> }<br>
><br>
> // Create the on-disk hash table in a buffer.<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</p>