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

[David Blaikie]

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... - 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.


>   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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150113/3c3af412/attachment.html>