r233172 - Revert "[Modules] When writing out the on-disk hash table for the decl context lookup tables, we need to establish a stable ordering for constructing the hash table. This is trickier than it might seem."
Chandler Carruth
chandlerc at google.com
Wed Mar 25 00:22:42 PDT 2015
Sorry, yes, the correct fix was to disable the tests.
I tried to do that originally, but apparently didn't disable enough
(despite running the tests many times).
On Wed, Mar 25, 2015 at 12:02 AM, Daniel Jasper <djasper at google.com> wrote:
> The problem is that the test is for testing non-determinism. And obviously
> there might just be instances where the test passes out of luck.
>
> I have temporarily disabled one more of those tests in r233173, which
> should hopefully appease all the bots for now.
>
> On Wed, Mar 25, 2015 at 7:35 AM, Justin Bogner <mail at justinbogner.com>
> wrote:
>
>> Rafael Espindola <rafael.espindola at gmail.com> writes:
>> > Author: rafael
>> > Date: Tue Mar 24 23:43:15 2015
>> > New Revision: 233172
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=233172&view=rev
>> > Log:
>> > Revert "[Modules] When writing out the on-disk hash table for the decl
>> > context lookup tables, we need to establish a stable ordering for
>> > constructing the hash table. This is trickier than it might seem."
>> >
>> > This reverts commit r233156. It broke the bots.
>>
>> Which bots did it break? Lots of bots (and my local check-clang) went
>> red from the revert, because the tests in r233162 depend on it. I can't
>> actually find any bots that look like r233156 broke (at least that were
>> still broken after r233162+r233163), but I'm probably not looking in the
>> right place.
>>
>> > Modified:
>> > cfe/trunk/lib/Serialization/ASTWriter.cpp
>> >
>> > Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
>> > URL:
>> >
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=233172&r1=233171&r2=233172&view=diff
>> >
>> ==============================================================================
>> > --- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
>> > +++ cfe/trunk/lib/Serialization/ASTWriter.cpp Tue Mar 24 23:43:15 2015
>> > @@ -3761,34 +3761,15 @@ ASTWriter::GenerateNameLookupTable(const
>> > !DC->HasLazyExternalLexicalLookups &&
>> > "must call buildLookups first");
>> >
>> > - // Create the on-disk hash table representation.
>> > llvm::OnDiskChainedHashTableGenerator<ASTDeclContextNameLookupTrait>
>> > Generator;
>> > ASTDeclContextNameLookupTrait Trait(*this);
>> >
>> > - // Store the sortable lookup results -- IE, those with meaningful
>> names. We
>> > - // will sort them by the DeclarationName in order to stabilize the
>> ordering
>> > - // of the hash table. We can't do this for constructors or conversion
>> > - // functions which are handled separately.
>> > - SmallVector<std::pair<DeclarationName, DeclContext::lookup_result>,
>> 16>
>> > - NamedResults;
>> > -
>> > - // We can't directly construct a nonce constructor or conversion
>> name without
>> > - // tripping asserts, so we just recall the first one we see. Only
>> the kind
>> > - // will actually be serialized.
>> > + // Create the on-disk hash table representation.
>> > DeclarationName ConstructorName;
>> > DeclarationName ConversionName;
>> > - // Retain a mapping from each actual declaration name to the results
>> > - // associated with it. We use a map here because the order in which
>> we
>> > - // discover these lookup results isn't ordered. We have to
>> re-establish
>> > - // a stable ordering which we do by walking the children of the decl
>> context,
>> > - // and emitting these in the order in which their names first
>> appeared. Note
>> > - // that the names' first appearance may not be one of the results or
>> a result
>> > - // at all. We just use this to establish an ordering.
>> > - llvm::SmallDenseMap<DeclarationName, DeclContext::lookup_result, 8>
>> > - ConstructorResults;
>> > - llvm::SmallDenseMap<DeclarationName, DeclContext::lookup_result, 4>
>> > - ConversionResults;
>> > + SmallVector<NamedDecl *, 8> ConstructorDecls;
>> > + SmallVector<NamedDecl *, 4> ConversionDecls;
>> >
>> > visitLocalLookupResults(DC, [&](DeclarationName Name,
>> > DeclContext::lookup_result Result) {
>> > @@ -3805,90 +3786,34 @@ ASTWriter::GenerateNameLookupTable(const
>> > // has the DeclarationName of the inherited constructors.
>> > if (!ConstructorName)
>> > ConstructorName = Name;
>> > - ConstructorResults.insert(std::make_pair(Name, Result));
>> > + ConstructorDecls.append(Result.begin(), Result.end());
>> > return;
>> >
>> > case DeclarationName::CXXConversionFunctionName:
>> > if (!ConversionName)
>> > ConversionName = Name;
>> > - ConversionResults.insert(std::make_pair(Name, Result));
>> > + ConversionDecls.append(Result.begin(), Result.end());
>> > return;
>> >
>> > default:
>> > - NamedResults.push_back(std::make_pair(Name, Result));
>> > + break;
>> > }
>> >
>> > + Generator.insert(Name, Result, Trait);
>> > });
>> >
>> > - // Sort and emit the named results first. This is the easy case.
>> > - std::sort(
>> > - NamedResults.begin(), NamedResults.end(),
>> > - [](const std::pair<DeclarationName, DeclContext::lookup_result>
>> &LHS,
>> > - const std::pair<DeclarationName, DeclContext::lookup_result>
>> &RHS) {
>> > - return LHS.first < RHS.first;
>> > - });
>> > - for (auto Results : NamedResults)
>> > - Generator.insert(Results.first, Results.second, Trait);
>> > -
>> > - // We have to specially handle the constructor and conversion
>> function
>> > - // declarations found.
>> > - if (ConstructorResults.size() == 1) {
>> > - // A special case that is easy is when we have just one constructor
>> > - // declaration name.
>> > + // Add the constructors.
>> > + if (!ConstructorDecls.empty()) {
>> > Generator.insert(ConstructorName,
>> > - ConstructorResults.lookup(ConstructorName),
>> Trait);
>> > - ConstructorResults.clear();
>> > - }
>> > - if (ConversionResults.size() == 1) {
>> > - // A special case that is easy is when we have just one conversion
>> function
>> > - // declaration name.
>> > - Generator.insert(ConversionName,
>> ConversionResults.lookup(ConversionName),
>> > + DeclContext::lookup_result(ConstructorDecls),
>> > Trait);
>> > - ConversionResults.clear();
>> > }
>> > - if (!ConstructorResults.empty() || !ConversionResults.empty()) {
>> > - SmallVector<NamedDecl *, 8> ConstructorDecls;
>> > - SmallVector<NamedDecl *, 8> ConversionDecls;
>> > -
>> > - // Walk the decls in the context and collapse the results in that
>> order. We
>> > - // handle both constructors and conversion functions in a single
>> walk as
>> > - // the walk is a relative cache-hostile linked list walk.
>> > - for (Decl *ChildD : DC->decls())
>> > - if (auto *ChildND = dyn_cast<NamedDecl>(ChildD)) {
>> > - auto ConstructorResultsIt =
>> > - ConstructorResults.find(ChildND->getDeclName());
>> > - if (ConstructorResultsIt != ConstructorResults.end()) {
>> > - ConstructorDecls.append(ConstructorResultsIt->second.begin(),
>> > - ConstructorResultsIt->second.end());
>> > - ConstructorResults.erase(ConstructorResultsIt);
>> > - }
>> > -
>> > - auto ConversionResultsIt =
>> > - ConversionResults.find(ChildND->getDeclName());
>> > - if (ConversionResultsIt != ConversionResults.end()) {
>> > - ConversionDecls.append(ConversionResultsIt->second.begin(),
>> > - ConversionResultsIt->second.end());
>> > - ConversionResults.erase(ConversionResultsIt);
>> > - }
>> > -
>> > - // If we handle all of the results, we're done.
>> > - if (ConstructorResults.empty() && ConversionResults.empty())
>> > - break;
>> > - }
>> > - assert(ConstructorResults.empty() && "Have constructor lookup
>> results not "
>> > - "associated with a
>> declaration name "
>> > - "within the context!");
>> > - assert(ConversionResults.empty() && "Have conversion function
>> lookup "
>> > - "results not associated with a
>> "
>> > - "declaration name within the
>> context!");
>> > -
>> > - if (!ConstructorDecls.empty())
>> > - Generator.insert(ConstructorName,
>> > - DeclContext::lookup_result(ConstructorDecls),
>> Trait);
>> > -
>> > - if (!ConversionDecls.empty())
>> > - Generator.insert(ConversionName,
>> > - DeclContext::lookup_result(ConversionDecls),
>> Trait);
>> > +
>> > + // Add the conversion functions.
>> > + if (!ConversionDecls.empty()) {
>> > + Generator.insert(ConversionName,
>> > + DeclContext::lookup_result(ConversionDecls),
>> > + Trait);
>> > }
>> >
>> > // Create the on-disk hash table in a buffer.
>> >
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150325/648635ba/attachment.html>
More information about the cfe-commits
mailing list