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

Duncan P. N. Exon Smith dexonsmith at apple.com
Tue Jan 13 11:10:04 PST 2015


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

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





More information about the llvm-commits mailing list