[IR] Fix a memory leak if Function::dropAllReferences() is followed by setHungoffOperand

[taolq]

Summary

This patch fixes a memory leak if Function::dropAllReferences() is followed by setHungoffOperand (e.g. setPersonality)
If NumUserOperands changes from 3 to 0 before calling allocHungoffUselist() to allocate memory,
the memory leaks which are allocated when NumUserOperands is changed from 0 to 3.
e.g.

llvm::Function* func = ...;
func->setPersonalityFn(foo);  // (1). call allocHungoffUselist() to allocate memory for uses
func->deleteBody();  // (2). call dropAllReferences(), and it changes NumUserOperands from 3 to 0
// (3). at this point, NumUserOperands is 0, the next line will allocate memory by allocHungoffUselist()
func->setPersonalityFn(bar);  // (4). call allocHungoffUselist(), so memory allocated in (1) leaks.

[MaskRay]

[IR] Fix a memory leak in Function::dropAllReferences()

Thanks for the patch. I wonder whether it's slightly unfair to call it a memory leak, but I do agree that dropAllReferences should be changed, as it fails to maintain a constraint: when NumUserOperands==0, the hung-off operand list should not be allocated.

It seems unfair because all call sites, dropAllReferences is followed by the deletion of the Function, and there will be no leak.
However, if we call setHungoffOperand (like setPersonality), there will be a leak.

This patch makes the API most robust.

llvm/lib/IR/Function.cpp

541

Change this to PointerType::get(getContext(), 0) after getInt1PtrTy removal.

Add a comment The code needs to match allocHungoffUselist. or similar.

[MaskRay]

This patch fixes a memory leak in Function::dropAllReferences().

Consider This patch fixes a memory leak if Function::dropAllReferences() is followed by setHungoffOperand (e.g. setPersonality)

[dexonsmith]

Seems unfortunate to add calls to ConstantPointerNull::get() when all of the callers are about to call delete.

Can we instead somehow enforce that delete is always called right after? Perhaps add an assertion that this is so?

[dexonsmith]

In D156618#4632179, @dexonsmith wrote:

Seems unfortunate to add calls to ConstantPointerNull::get() when all of the callers are about to call delete.

Can we instead somehow enforce that delete is always called right after? Perhaps add an assertion that this is so?

Or, looking back at the motivation (calling deleteBody()), can we have two entry points?

• One prepares for deletion (99% of callers), skips CPN::get()
• One makes it a prototype (deleteBody), calls CPN::get()

[taolq]

In D156618#4632192, @dexonsmith wrote:

In D156618#4632179, @dexonsmith wrote:

Seems unfortunate to add calls to ConstantPointerNull::get() when all of the callers are about to call delete.

Can we instead somehow enforce that delete is always called right after? Perhaps add an assertion that this is so?

Or, looking back at the motivation (calling deleteBody()), can we have two entry points?

• One prepares for deletion (99% of callers), skips CPN::get()
• One makes it a prototype (deleteBody), calls CPN::get()

Thanks for the comment. I think it is a little redundant. Since we already has dector to release the memory, another entry points seems redundant.
So I'd like to reserve the only prototype version.

[taolq]

rebase to main

[dexonsmith]

In D156618#4636483, @taolq wrote:

In D156618#4632192, @dexonsmith wrote:

In D156618#4632179, @dexonsmith wrote:

Seems unfortunate to add calls to ConstantPointerNull::get() when all of the callers are about to call delete.

Can we instead somehow enforce that delete is always called right after? Perhaps add an assertion that this is so?

Or, looking back at the motivation (calling deleteBody()), can we have two entry points?

• One prepares for deletion (99% of callers), skips CPN::get()
• One makes it a prototype (deleteBody), calls CPN::get()

Thanks for the comment. I think it is a little redundant. Since we already has dector to release the memory, another entry points seems redundant.
So I'd like to reserve the only prototype version.

What is "dector"?

[taolq]

In D156618#4637306, @dexonsmith wrote:

In D156618#4636483, @taolq wrote:

In D156618#4632192, @dexonsmith wrote:

In D156618#4632179, @dexonsmith wrote:

Seems unfortunate to add calls to ConstantPointerNull::get() when all of the callers are about to call delete.

Can we instead somehow enforce that delete is always called right after? Perhaps add an assertion that this is so?

Or, looking back at the motivation (calling deleteBody()), can we have two entry points?

• One prepares for deletion (99% of callers), skips CPN::get()
• One makes it a prototype (deleteBody), calls CPN::get()

Thanks for the comment. I think it is a little redundant. Since we already has dector to release the memory, another entry points seems redundant.
So I'd like to reserve the only prototype version.

What is "dector"?

I made a typo. I mean dtor, destructor.

[dexonsmith]

I looked at the documentation for User::dropAllReferences(), which this overrides. That has strengthened my opinion so I'm requesting changes.

Here are the docs:

/// Drop all references to operands.
///
/// This function is in charge of "letting go" of all objects that this User
/// refers to.  This allows one to 'delete' a whole class at a time, even
/// though there may be circular references...  First all references are
/// dropped, and all use counts go to zero.  Then everything is deleted for
/// real.  Note that no operations are valid on an object that has "dropped
/// all references", except operator delete.

I don't think it's appropriate for the Function override to behave differently.

Instead, it looks like Function::deleteBody() should stop calling dropAllReferences(). Probably a private helper could be extracted from dropAllReferences() since you'd want to do almost all the same things (except, don't call User::dropAllReferences(), and do add references to ptr null).

In D156618#4637792, @taolq wrote:

What is "dector"?

I made a typo. I mean dtor, destructor.

Thanks, I probably should have guessed that!

[taolq]

add a new function to implement deleteBody, do not call User::dropAllReference

[taolq]

In D156618#4639976, @dexonsmith wrote:

I looked at the documentation for User::dropAllReferences(), which this overrides. That has strengthened my opinion so I'm requesting changes.

Here are the docs:

/// Drop all references to operands.
///
/// This function is in charge of "letting go" of all objects that this User
/// refers to.  This allows one to 'delete' a whole class at a time, even
/// though there may be circular references...  First all references are
/// dropped, and all use counts go to zero.  Then everything is deleted for
/// real.  Note that no operations are valid on an object that has "dropped
/// all references", except operator delete.

I don't think it's appropriate for the Function override to behave differently.

Instead, it looks like Function::deleteBody() should stop calling dropAllReferences(). Probably a private helper could be extracted from dropAllReferences() since you'd want to do almost all the same things (except, don't call User::dropAllReferences(), and do add references to ptr null).

In D156618#4637792, @taolq wrote:

What is "dector"?

I made a typo. I mean dtor, destructor.

Thanks, I probably should have guessed that!

Okay, I got your point. How about this version? Look forward for your reviewing.

[dexonsmith]

llvm/include/llvm/IR/Function.h

119 ↗

(On Diff #556785)

I think you should add a parameter bool ShouldDrop so that you can share logic between dropAllReferences() and deleteBody().

672 ↗

(On Diff #556785)

This can be deleteBodyImpl(/*ShouldDrop*/=false).

llvm/lib/IR/Function.cpp

528

This can move to the header and just call deleteBlodyImpl(/*ShouldDrop=*/true).

567–571

This behaviour can depend on ShouldDrop.

[taolq]

share logic between dropAllReferences() and deleteBody()

[taolq]

Thanks for the comments. They have been resolved.

[dexonsmith]

LGTM! Thanks.

llvm/lib/IR/Function.cpp

 //===- Function.cpp - Implement the Global object classes -----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //

 // dropAllReferences() - This function causes all the subinstructions to "let
 // go" of all references that they are maintaining.  This allows one to
 // 'delete' a whole class at a time, even though there may be circular
 // references... first all references are dropped, and all use counts go to
 // zero.  Then everything is deleted for real.  Note that no operations are
 // valid on an object that has "dropped all references", except operator
 // delete.
 //
 void Function::dropAllReferences() {
   setIsMaterializable(false);
 
   for (BasicBlock &BB : *this)
     BB.dropAllReferences();
 
   // Delete all basic blocks. They are now unused, except possibly by
   // blockaddresses, but BasicBlock's destructor takes care of those.
   while (!BasicBlocks.empty())
     BasicBlocks.begin()->eraseFromParent();
 
   // Drop uses of any optional data (real or placeholder).
   if (getNumOperands()) {
-    User::dropAllReferences();
-    setNumHungOffUseOperands(0);
+    auto *CPN = ConstantPointerNull::get(Type::getInt1PtrTy(getContext(), 0));
+    Op<0>().set(CPN);
+    Op<1>().set(CPN);
+    Op<2>().set(CPN);
     setValueSubclassData(getSubclassDataFromValue() & ~0xe);
   }
 
   // Metadata is stored in a side-table.
   clearMetadata();
 }
 
 void Function::addAttributeAtIndex(unsigned i, Attribute Attr) {
   AttributeSets = AttributeSets.addAttributeAtIndex(getContext(), i, Attr);
 }
 
 void Function::addFnAttr(Attribute::AttrKind Kind) {
   AttributeSets = AttributeSets.addFnAttribute(getContext(), Kind);
 }
 
 void Function::addFnAttr(StringRef Kind, StringRef Val) {
   AttributeSets = AttributeSets.addFnAttribute(getContext(), Kind, Val);
 }
 
 void Function::addFnAttr(Attribute Attr) {
   AttributeSets = AttributeSets.addFnAttribute(getContext(), Attr);
 }
 
 void Function::addFnAttrs(const AttrBuilder &Attrs) {
   AttributeSets = AttributeSets.addFnAttributes(getContext(), Attrs);
 }
 
 void Function::addRetAttr(Attribute::AttrKind Kind) {
   AttributeSets = AttributeSets.addRetAttribute(getContext(), Kind);
 }
 
 void Function::addRetAttr(Attribute Attr) {
   AttributeSets = AttributeSets.addRetAttribute(getContext(), Attr);
 }