[llvm-commits] [llvm] r96609 - in /llvm/trunk: include/llvm/Metadata.h lib/VMCore/LLVMContextImpl.h lib/VMCore/Metadata.cpp

[Jeffrey Yasskin]

On Thu, Feb 18, 2010 at 12:53 PM, Devang Patel <dpatel at apple.com> wrote:
> Author: dpatel
> Date: Thu Feb 18 14:53:16 2010
> New Revision: 96609
>
> URL: http://llvm.org/viewvc/llvm-project?rev=96609&view=rev
> Log:
> Destroy MDNodes gracefully while deleting llvm context.

This change doesn't accomplish that. When one of an MDNode's operands
turns to NULL, it's removed from the MDNodeSet, which means the "while
(!MDNodeSet.empty())" loop never executes.

I'm working on a real fix, and from reading the code, the
replaceAllOperandsWithNull() step may not be necessary. Just calling
MDNode::destroy() looks like it should work fine, even with operands
still present and with cycles in the MDNode graph. What am I missing?

Btw, I'm looking for memory leaks using valgrind-3.5.0 installed from
macports. I found this one with `valgrind --dsymutil=yes
--leak-check=full unittests/VMCore/Debug/VMCoreTests`

> Modified:
>    llvm/trunk/include/llvm/Metadata.h
>    llvm/trunk/lib/VMCore/LLVMContextImpl.h
>    llvm/trunk/lib/VMCore/Metadata.cpp
>
> Modified: llvm/trunk/include/llvm/Metadata.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Metadata.h?rev=96609&r1=96608&r2=96609&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/Metadata.h (original)
> +++ llvm/trunk/include/llvm/Metadata.h Thu Feb 18 14:53:16 2010
> @@ -75,6 +75,7 @@
>   MDNode(const MDNode &);                // DO NOT IMPLEMENT
>   void operator=(const MDNode &);        // DO NOT IMPLEMENT
>   friend class MDNodeOperand;
> +  friend class LLVMContextImpl;
>
>   /// NumOperands - This many 'MDNodeOperand' items are co-allocated onto the
>   /// end of this MDNode.
> @@ -106,6 +107,9 @@
>   // Replace each instance of F from the operand list of this node with T.
>   void replaceOperand(MDNodeOperand *Op, Value *NewVal);
>   ~MDNode();
> +    // replaceAllOperandsWithNull - This is used while destroying llvm context to
> +  // gracefully delete all nodes. This method replaces all operands with null.
> +  void replaceAllOperandsWithNull();
>
>  protected:
>   explicit MDNode(LLVMContext &C, Value *const *Vals, unsigned NumVals,
>
> Modified: llvm/trunk/lib/VMCore/LLVMContextImpl.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/VMCore/LLVMContextImpl.h?rev=96609&r1=96608&r2=96609&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/VMCore/LLVMContextImpl.h (original)
> +++ llvm/trunk/lib/VMCore/LLVMContextImpl.h Thu Feb 18 14:53:16 2010
> @@ -229,13 +229,23 @@
>       if (I->second->use_empty())
>         delete I->second;
>     }
> -    MDNodeSet.clear();
>     AlwaysOpaqueTy->dropRef();
>     for (OpaqueTypesTy::iterator I = OpaqueTypes.begin(), E = OpaqueTypes.end();
>         I != E; ++I) {
>       (*I)->AbstractTypeUsers.clear();
>       delete *I;
>     }
> +    // Destroy MDNode operands first.
> +    for (FoldingSetIterator<MDNode> I = MDNodeSet.begin(), E = MDNodeSet.end();
> +         I != E;) {
> +      MDNode *N = &(*I);
> +      ++I;
> +      N->replaceAllOperandsWithNull();
> +    }
> +    while (!MDNodeSet.empty()) {
> +      MDNode *N = &(*MDNodeSet.begin());
> +      N->destroy();
> +    }
>   }
>  };
>
>
> Modified: llvm/trunk/lib/VMCore/Metadata.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/VMCore/Metadata.cpp?rev=96609&r1=96608&r2=96609&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/VMCore/Metadata.cpp (original)
> +++ llvm/trunk/lib/VMCore/Metadata.cpp Thu Feb 18 14:53:16 2010
> @@ -257,6 +257,13 @@
>     ID.AddPointer(getOperand(i));
>  }
>
> +// replaceAllOperandsWithNull - This is used while destroying llvm context to
> +// gracefully delete all nodes. This method replaces all operands with null.
> +void MDNode::replaceAllOperandsWithNull() {
> +  for (MDNodeOperand *Op = getOperandPtr(this, 0), *E = Op+NumOperands;
> +       Op != E; ++Op)
> +    replaceOperand(Op, 0);
> +}
>
>  // Replace value from this node's operand list.
>  void MDNode::replaceOperand(MDNodeOperand *Op, Value *To) {
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>