[llvm] r217514 - Fix comments of createReplaceableForwardDecl() and createForwardDecl().

[David Blaikie]

On Wed, Sep 10, 2014 at 9:28 AM, Frédéric Riss <friss at apple.com> wrote:

>
> On 10 Sep 2014, at 18:21, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Wed, Sep 10, 2014 at 9:03 AM, Frederic Riss <friss at apple.com> wrote:
>
>> Author: friss
>> Date: Wed Sep 10 11:03:14 2014
>> New Revision: 217514
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=217514&view=rev
>> Log:
>> Fix comments of createReplaceableForwardDecl() and createForwardDecl().
>>
>> Noticed while trying to understand how the merge of forward decalred types
>> and defintions work.
>>
>> Reviewers: echristo, dblaikie, aprantl
>>
>> Subscribers: llvm-commits
>>
>> Differential Revision: http://reviews.llvm.org/D5291
>>
>> Modified:
>>     llvm/trunk/lib/IR/DIBuilder.cpp
>>
>> Modified: llvm/trunk/lib/IR/DIBuilder.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DIBuilder.cpp?rev=217514&r1=217513&r2=217514&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/IR/DIBuilder.cpp (original)
>> +++ llvm/trunk/lib/IR/DIBuilder.cpp Wed Sep 10 11:03:14 2014
>> @@ -879,8 +879,7 @@ DIBasicType DIBuilder::createUnspecified
>>    return DIBasicType();
>>  }
>>
>> -/// createForwardDecl - Create a temporary forward-declared type that
>> -/// can be RAUW'd if the full type is seen.
>> +/// createForwardDecl - Create a permanent forward-declared type.
>>  DICompositeType
>>  DIBuilder::createForwardDecl(unsigned Tag, StringRef Name, DIDescriptor
>> Scope,
>>                               DIFile F, unsigned Line, unsigned
>> RuntimeLang,
>> @@ -914,7 +913,7 @@ DIBuilder::createForwardDecl(unsigned Ta
>>    return RetTy;
>>  }
>>
>> -/// createForwardDecl - Create a temporary forward-declared type that
>> +/// createReplaceableForwardDecl - Create a temporary forward-declared
>> type that
>>  /// can be RAUW'd if the full type is seen.
>>
>
> FWIW (either just for your information, or something you can enshrine in a
> comment): Because this creates a temporary (perhaps we should replace
> "replaceable" with "temporary") node, it /must/ be RAUW'd at some point,
> sooner or later. It isn't optional.
>
>
>
> Interesting. I was just working on another patch that adds this kind of
> temporaries for function and variable forward definitions (for the missed
> imported decl and my default argument attributes). The comment of
> MDNode::getTemporary() reads:
>
>   /// getTemporary - Return a temporary MDNode, for use in constructing
>   /// cyclic MDNode structures. A temporary MDNode is not uniqued,
>   /// may be RAUW'd, and must be manually deleted with deleteTemporary.
>
> I had read the ‘must be manually deletes’ part as ‘if you want to get rid
> of if, please call deleteTemporary’ but you’re saying that these nodes must
> not survive the code generation or something will go wrong?
>

Thanks for quoting the docs - I had forgotten the specifics (you might want
to look at the commit history around here - I had to make changes to these
forward decls to fix memory leaks, reading up on that may help you avoid
similar memory leaks as you implement similar functionality).

The crux is the thing must be manually deleted with deleteTemporary at some
point... I don't know if that means it can survive codegen or not - if you
had a hook to delete it later, maybe that would be OK (I mean it is OK -
ultimately when we used to leak these forward decl nodes, we still got the
right metadata out - so I guess if we could hook into it later and delete
it after we finish using the IR), but in reality I think that means
replacing the temporary nodes, explicitly deleting them (deleteTemporary)
and moving on.


>
> Fred
>
>
>  DICompositeType DIBuilder::createReplaceableForwardDecl(
>>      unsigned Tag, StringRef Name, DIDescriptor Scope, DIFile F, unsigned
>> Line,
>> @@ -942,7 +941,7 @@ DICompositeType DIBuilder::createReplace
>>    MDNode *Node = MDNode::getTemporary(VMContext, Elts);
>>    DICompositeType RetTy(Node);
>>    assert(RetTy.isCompositeType() &&
>> -         "createForwardDecl result should be a DIType");
>> +         "createReplaceableForwardDecl result should be a DIType");
>>    if (!UniqueIdentifier.empty())
>>      retainType(RetTy);
>>    return RetTy;
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140910/069bedb7/attachment.html>