[modules] PR24954

[Richard Smith via cfe-commits]

Please restrict this to the case where the function template is a
friend (check D->getFriendObjectKind()). Otherwise, this looks good to
me. Thanks!

On Mon, Mar 7, 2016 at 2:23 AM, Vassil Vassilev <v.g.vassilev at gmail.com> wrote:
> ping...
>
> On 22/02/16 21:51, Vassil Vassilev wrote:
>>
>> On 02/02/16 02:52, Richard Smith via cfe-commits wrote:
>>>
>>> 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.
>>
>> The attached patch fixes the issues more or less in the direction you
>> suggest. I am not sure if I expressed well enough the conditions of the
>> template friend corner case. Could you have a look?
>>>
>>>
>>>> --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
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>>
>