[modules] PR24954

[Vassil Vassilev via cfe-commits]

On 16/03/16 03:22, Richard Smith wrote:
> Please restrict this to the case where the function template is a
> friend (check D->getFriendObjectKind()). Otherwise, this looks good to
> me. Thanks!
Done and committed in r263634.
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
>>>