[llvm] r225721 - IR: Fix unit test memory leak reported by ASan

[Duncan P. N. Exon Smith]

> On 2015-Jan-13, at 10:23, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Mon, Jan 12, 2015 at 5:04 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> > On 2015-Jan-12, at 15:02, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Mon, Jan 12, 2015 at 2:46 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> > Author: dexonsmith
> > Date: Mon Jan 12 16:46:15 2015
> > New Revision: 225721
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=225721&view=rev
> > Log:
> > IR: Fix unit test memory leak reported by ASan
> >
> > http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/603/steps/check-llvm%20asan/logs/stdio
> >
> > Thanks Alexey for pointing me to this!
> >
> > Modified:
> >     llvm/trunk/unittests/IR/MetadataTest.cpp
> >
> > Modified: llvm/trunk/unittests/IR/MetadataTest.cpp
> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/MetadataTest.cpp?rev=225721&r1=225720&r2=225721&view=diff
> > ==============================================================================
> > --- llvm/trunk/unittests/IR/MetadataTest.cpp (original)
> > +++ llvm/trunk/unittests/IR/MetadataTest.cpp Mon Jan 12 16:46:15 2015
> > @@ -314,6 +314,7 @@ TEST_F(MDNodeTest, handleChangedOperandR
> >    Metadata *Ops3[] = {N2};
> >    MDNode *N3 = MDNode::get(Context, Ops3);
> >    Temp3->replaceAllUsesWith(N3);
> > +  delete Temp3;
> >
> > unique_ptr?
> 
> Good idea.  r225749 (and I used it in the next test I added).
> 
> >
> > (perhaps, eventually, we could change MDNode::getTemporary to return unique_ptr - or we could change it now, make the existing callers release() and fix them whenever we feel like it)
> 
> I'm not sure that'll be worth it (although I'm not particularly
> opposed).  Reason being that (outside of unit tests) these are
> typically dropped into MDNodes and not stored anywhere, then fixed
> up later.
> 
> What do you mean by fixed up later? Given that the temporary nodes have to be manually managed by the caller (previously you had to call deleteTemporary, now you can just use delete directly) I would imagine all users would have a side table of pointers to these nodes so they could clean them up. At least I think that's true for debug info's temporary nodes... -

So right now DIBuilder keeps track of nodes that *point* at
temporary nodes, not the temporary nodes themselves.  The temporary
nodes are found again as operands of the tracked nodes.

> OK, so DIDescriptor likes to be a bit 'relaxed' and store both owning and non-owning pointers in the DbgNode (& callers just have to know that they must RAUW it, and leak if they don't) - but that seems a sub-optimal API that might be improved by forcing unique_ptr through things... maybe.

This isn't just a `DIDescriptor` thing -- the only place the nodes
are actually stored right now are in `MDOperand`, which definitely
does not own its operands right now (although I have thought about
adding reference counting there).

However, as I said before, I'm not really opposed.  Outside of
debug info -- the only schema with long-lived forward declarations
-- I think returning `std::unique_ptr<>` makes a ton of sense.
(Although their only apparent use outside of debug info it to set
up self-references to defeat uniquing entirely, and we have a
better solution for that now.)

(Besides, we can probably design away the long-lived forward
declarations.  Aside from supporting cycles, their main use right
now is to delay uniquing.  It would be pretty easy to support this
more directly (create a distinct node, update its operands, and
then turning on uniquing -- the only part without API is that
last step).  Even without new API, if the tracked node itself was
a forward declaration and *it* was the one that got RAUW'ed, we'd
be set.)

>  
>   Given test coverage + ASan bots, I'm not sure
> explicitly tracking them makes sense (I mean, that's the whole
> reason the leak detector was deleted).
> 
> >
> >
> >    // !4 = !{!1}
> >    Metadata *Ops4[] = {N1};
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits