This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by taolq on Jul 30 2023, 7:19 AM.

Details

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.

Diff Detail

Event Timeline

taolq created this revision.Jul 30 2023, 7:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2023, 7:19 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
taolq requested review of this revision.Jul 30 2023, 7:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2023, 7:19 AM
taolq retitled this revision from fix memory leak in Function::dropAllReferences() to Fix a memory leak in Function::dropAllReferences().Jul 30 2023, 7:45 AM
taolq edited the summary of this revision. (Show Details)
taolq added reviewers: vsk, loladiro, lattner, dexonsmith.
taolq added subscribers: kazu, mtrofin.
Enna1 retitled this revision from Fix a memory leak in Function::dropAllReferences() to [IR] Fix a memory leak in Function::dropAllReferences().
Enna1 added a subscriber: Enna1.Aug 30 2023, 7:03 PM
MaskRay accepted this revision.EditedAug 30 2023, 7:51 PM

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

This revision is now accepted and ready to land.Aug 30 2023, 7:51 PM

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)

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?

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 retitled this revision from [IR] Fix a memory leak in Function::dropAllReferences() to [IR] Fix a memory leak if Function::dropAllReferences() is followed by setHungoffOperand .Sep 1 2023, 4:24 AM
taolq edited the summary of this revision. (Show Details)
taolq added a comment.Sep 3 2023, 8:00 PM

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 updated this revision to Diff 555716.Sep 4 2023, 5:52 AM
taolq retitled this revision from [IR] Fix a memory leak if Function::dropAllReferences() is followed by setHungoffOperand to [IR] Fix a memory leak if Function::dropAllReferences() is followed by setHungoffOperand.

rebase to main

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 added a comment.Sep 4 2023, 11:25 PM

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 requested changes to this revision.Sep 6 2023, 11:57 AM

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

What is "dector"?

I made a typo. I mean dtor, destructor.

Thanks, I probably should have guessed that!

This revision now requires changes to proceed.Sep 6 2023, 11:57 AM
taolq updated this revision to Diff 556785.Sep 14 2023, 8:18 AM

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

taolq added a comment.Sep 14 2023, 8:20 AM

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

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 added inline comments.Sep 14 2023, 8:54 AM
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 updated this revision to Diff 557044.Sep 19 2023, 7:48 AM

share logic between dropAllReferences() and deleteBody()

taolq marked 5 inline comments as done.Sep 19 2023, 7:49 AM

Thanks for the comments. They have been resolved.

dexonsmith accepted this revision.Sep 19 2023, 8:16 AM

LGTM! Thanks.

This revision is now accepted and ready to land.Sep 19 2023, 8:16 AM
This revision was automatically updated to reflect the committed changes.
taolq marked an inline comment as not done.