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

David Blaikie dblaikie at gmail.com
Tue Jan 13 11:16:40 PST 2015 

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

>
> > On 2015-Jan-13, at 11:02, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Tue, Jan 13, 2015 at 10:57 AM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> >
> > > On 2015-Jan-13, at 10:55, David Blaikie <dblaikie at gmail.com> wrote:
> > >
> > >
> > >
> > > 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).
> >
> > I was looking at `DIBuilder::finalize()`, which follows the pattern
> > I just described.
> >
> > Not sure I follow there either.
> >
> > The TempBlah MDNode*s are the actual MDNodes being replaced, not
> pointers to nodes that point at the temporaries - indeed all of those are
> owning pointers (they are only ever assigned the result of
> MDNode::getTemporary) & could be std::unique_ptrs. Then in finalize we
> build a DIDescriptor (losing type safety of ownership here) which nown
> 'owns' the pointer and we call DIDescriptor::replaceAllUsesWith which then
> replaces it and deletes the temporary node.
>
> (adding back llvm-commits)
>
> I read the code carelessly, and assumed it was all done like
> subprograms:
>
>     for (unsigned i = 0, e = SPs.getNumElements(); i != e; ++i) {
>       DISubprogram SP(SPs.getElement(i));
>       if (MDNode *Temp = SP.getVariablesNodes()) {
>         SmallVector<Metadata *, 4> Variables;
>         for (Metadata *V : PreservedVariables.lookup(SP))
>           Variables.push_back(V);
>         DIArray AV = getOrCreateArray(Variables);
>         DIType(Temp).replaceAllUsesWith(AV);
>       }
>     }
>
> You're right -- *almost* all cases are done like you said.
>

Heh, fair enough - I only looked at one or two places & assumed it was all
the same. Just got lucky I guess.

Anyway.


>
> >
> > But maybe I'm confused/we're talking past each other?
> >
> > - David
> >
> >
> > Looked like the same pattern is done two different ways :/.
> >
> > >
> > > 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/b604a8ba/attachment.html>

More information about the llvm-commits mailing list