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."
Rafael Espíndola
rafael.espindola at gmail.com
Wed Mar 25 05:58:31 PDT 2015
Oh, sorry about that. On my machine Modules/cxx-decls.cpp always
failed with this patch.
On 25 March 2015 at 03:22, Chandler Carruth <chandlerc at google.com> wrote:
> 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
>>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
More information about the cfe-commits
mailing list