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

[David Blaikie]

On Tue, Jan 13, 2015 at 10:45 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

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

Not sure I follow here - DIDescriptor::replaceAllUsesWith replaces itself
(the DbgNode member, with a permanent copy). It doesn't look like it's
dealing with a node in an MDOperand.

Looking at CGDebugInfo::finalize I believe every DIDescriptor in
ObjCInterfaceCache, ReplaceMap, and FwDeclReplaceMap are DIDescriptor's
whos DbgNode is a temporary node and needs to be replaced and explicitly
deleted. So this is making DIDescriptor's DbgNode a 'maybe owning' smart
pointer (except the maybe-owning-ness is being tracked externally, so it
doesn't use an extra bit to store that knowledge internally).


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

Certainly agreed with this last bit, I think... not quite sure how that'll
all look, but yes - with better metadata functionality like explicit
distinctness we might be able to do away with some of this.

- David


>
> >
> >   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/0a825f49/attachment.html>