[modules] PR24954

[Richard Smith via cfe-commits]

On Thu, Jan 28, 2016 at 8:23 AM, Vassil Vassilev <vvasilev at cern.ch> wrote:
> Would this patch be more reasonable? It follows what
> RegisterTemplateSpecialization (introduced in r245779) does. AFAICT this
> adds an update record far less often.

It's still adding redundant update records. We'll write the
appropriate update record as part of serializing the declaration
itself, so we only need to ensure that the declaration is emitted, not
actually emit an update record for it. Perhaps you could store a list
of such declarations on the ASTWriter, and call GetDeclRef on each of
them once we start emitting the AST file (or maybe just push them into
UpdatingVisibleDecls). Please also restrict this to the template
friend corner case.

> --Vassil
>
> On 12/12/15 16:13, Vassil Vassilev wrote:
>
> I couldn't find GetDecl routine in the ASTWriter. Could you elaborate?
>
> Assuming you meant ASTWriter::GetDeclRef(D): It seems that the conditions
> when calling GetDeclRef differ from the conditions of
> AddedCXXTemplateSpecialization. Eg:
>
> ASTWriter::AddedCXXTemplateSpecialization {
>   assert(!WritingAST && "Already writing the AST!");
>   ...
> }
> ASTWriter::GetDeclRef {
>   assert(WritingAST && "Cannot request a declaration ID before AST
> writing");
>   ..
> }
>
> IIUC this particular instantiation happens *after* module B was built, thus
> it needs to be retrospectively added to the serialized namespace. It looks
> like even avoiding somehow the asserts of GetDeclRef it wouldn't help much.
>
> Alternatively I could try to reduce the redundant update records by
> narrowing down to instantiations coming in the context of friends.
>
> --Vassil
>
> On 12/12/15 01:07, Richard Smith wrote:
>
> Instead of adding an update record directly in this case (which will emit
> far more update records than necessary), how about just calling GetDecl(D)
> from AddedCXXTemplateSpecialization to ensure that it gets emitted?
>
> On Fri, Dec 4, 2015 at 7:46 AM, Vassil Vassilev <vvasilev at cern.ch> wrote:
>>
>> Hi,
>>   Could you review my fix please.
>> Many thanks,
>> Vassil
>>
>> On 08/10/15 15:53, Vassil Vassilev wrote:
>>>
>>> Hi Richard,
>>>   I started working on https://llvm.org/bugs/show_bug.cgi?id=24954
>>>
>>>   IIUC r228485 introduces an abstraction to deal with
>>> not-really-anonymous friend decls
>>> (serialization::needsAnonymousDeclarationNumber in ASTCommon.cpp).
>>>
>>>   A comment explicitly says:
>>>   "// This doesn't apply to friend tag decls; Sema makes those available
>>> to name
>>>    // lookup in the surrounding context."
>>>
>>>   In the bug reproducer, the friend function (wrt __iom_t10) is forward
>>> declared in the same namespace, where Sema makes the friend available for a
>>> name lookup.
>>>
>>>   It seems that the friend operator<< in __iom_t10 (sorry about the names
>>> they come from libcxx) doesn't get registered in the ASTWriter's DeclIDs but
>>> it gets registered in outer namespace's lookup table. Thus, assert is
>>> triggered when finalizing module A, since it rebuilds the lookups of the
>>> updated contexts.
>>>
>>>   The issue only appears when building module A deserializes/uses module
>>> B.
>>>
>>>   Currently I was assume that something wrong happens in either
>>> needsAnonymousDeclarationNumber or I hit a predicted issue
>>> ASTWriterDecl.cpp:1602
>>>     // FIXME: This is not correct; when we reach an imported declaration
>>> we
>>>     // won't emit its previous declaration.
>>>     (void)Writer.GetDeclRef(D->getPreviousDecl());
>>>     (void)Writer.GetDeclRef(MostRecent);
>>>
>>>   The issue seems a fairly complex one and I am a bit stuck.
>>>
>>>   Any hints are very very welcome ;)
>>> Many thanks,
>>> Vassil
>>>
>>>
>>>
>>
>
>
>