[modules] PR24954

[Vassil Vassilev via cfe-commits]

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
>