[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

533–543

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

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

672

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

llvm/lib/IR/Function.cpp

521

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

566–570

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/include/llvm/IR/Function.h

 //===- llvm/Function.h - Class to represent a single function ---*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
void CheckLazyArguments() const {
     if (hasLazyArguments())
       BuildLazyArguments();
   }
 
   void BuildLazyArguments() const;
 
   void clearArguments();
 
+  void deleteBodyImpl(bool ShouldDrop);
+
   /// Function ctor - If the (optional) Module argument is specified, the
   /// function is automatically inserted into the end of the function list for
   /// the module.
   ///
   Function(FunctionType *Ty, LinkageTypes Linkage, unsigned AddrSpace,
            const Twine &N = "", Module *M = nullptr);
 
 public:
public:
   /// copyAttributesFrom - copy all additional attributes (those not needed to
   /// create a Function) from the Function Src to this one.
   void copyAttributesFrom(const Function *Src);
 
   /// deleteBody - This method deletes the body of the function, and converts
   /// the linkage to external.
   ///
   void deleteBody() {
-    dropAllReferences();
+    deleteBodyImpl(/*ShouldDrop=*/false);
     setLinkage(ExternalLinkage);
   }
 
   /// removeFromParent - This method unlinks 'this' from the containing module,
   /// but does not delete it.
   ///
   void removeFromParent();
 
/// @}
   /// zero.  Then everything is deleted for real.  Note that no operations are
   /// valid on an object that has "dropped all references", except operator
   /// delete.
   ///
   /// Since no other object in the module can have references into the body of a
   /// function, dropping all references deletes the entire body of the function,
   /// including any contained basic blocks.
   ///
-  void dropAllReferences();
+  void dropAllReferences() {
+    deleteBodyImpl(/*ShouldDrop=*/true);
+  }
 
   /// hasAddressTaken - returns true if there are any uses of this function
   /// other than direct calls or invokes to it, or blockaddress expressions.
   /// Optionally passes back an offending user for diagnostic purposes,
   /// ignores callback uses, assume like pointer annotation calls, references in
   /// llvm.used and llvm.compiler.used variables, operand bundle
   /// "clang.arc.attachedcall", and direct calls with a different call site
   /// signature (the function is implicitly casted).

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
 //
 //===----------------------------------------------------------------------===//
 //
if (!Name.empty())
       A.setName(Name);
   }
 
   setValueSubclassData(getSubclassDataFromValue() & ~(1 << 0));
   assert(!hasLazyArguments());
   Src.setValueSubclassData(Src.getSubclassDataFromValue() | (1 << 0));
 }
 
-// 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() {
+void Function::deleteBodyImpl(bool ShouldDrop) {
   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()) {
+    if (ShouldDrop) {
+      // Drop uses of any optional data (real or placeholder).
       User::dropAllReferences();
       setNumHungOffUseOperands(0);
+    } else {
+      // The code needs to match Function::allocHungoffUselist().
+      auto *CPN = ConstantPointerNull::get(PointerType::get(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);
 }