Page MenuHomePhabricator

Fix for memory leak reported by Valgrind
AcceptedPublic

Authored by mwezdeck on Jul 8 2020, 12:54 AM.

Details

Summary

If llvm so lib is dlopened and dlclosed several times, then memory leak can be observed, reported by Valgrind.

This patch fixes the issue.

Diff Detail

Event Timeline

mwezdeck created this revision.Jul 8 2020, 12:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2020, 12:54 AM
mwezdeck updated this revision to Diff 276340.Jul 8 2020, 1:48 AM
mwezdeck set the repository for this revision to rG LLVM Github Monorepo.Jul 8 2020, 2:15 AM

Can I kindly ask for review?

mwezdeck added a reviewer: tpr.Jul 8 2020, 6:03 AM
tpr added a reviewer: beanz.Jul 8 2020, 10:03 AM

+Chris Bieneman

I'm a bit worried that there is a reason why the removed code was there, rather than using a static initializer.

Does this add a static constructor?

beanz added a comment.Jul 8 2020, 12:53 PM

Does this add a static constructor?

Yes it does. std::recursive_mutex has non-trivial constructors and destructors.

The code that was there knowingly leaked the mutex to avoid the static constructors and destructors. It should be safe to destroy it in llvm_shutdown, but making it a global is bad.

mwezdeck updated this revision to Diff 276641.Jul 9 2020, 12:34 AM

Can anybody point me out why static constructors are bad? Is there some kind of idiom?

Any resource where I can read about that will be helpful.

Thanks.

I'm updating the patch

beanz added a comment.Jul 9 2020, 7:39 AM

There are a lot of downsides to global constructors. One is that they are difficult to debug because the code runs before main, so if anything goes wrong callstacks are odd, debuggers sometimes struggle to attach, and general headaches ensue.

Additionally global constructors are an always-paid performance hit. Local statics are only constructed if they are used, but global statics are always constructed, and transitively they construct anything else they use. This can severely impact the process launch time of a program and it is very difficult to optimize around (other than just not using global constructors). While process launch time may not be something most applications care about because it "only happens once" for a compiler that can be invoked tens or hundreds of thousands of times per day per user, that cost really adds up.

The last big issue with global constructors in LLVM is that LLVM is used as a library in many contexts, and global constructors (and generally global variables) cause lots of issues with that. One of the big problems with using global constructors in library code is that it is really easy to get multiple copies of the globals embedded into the same process. This generally happens when you have a single executable that links two libraries that both include parts of LLVM (in particular libLLVMSupport which you usually want to be able to inline/LTO across). There have been periodic pushes to clean up the global variables, and most of the ones in LLVM today are cl::opt objects which need a re-design to clean up.

beanz added a comment.Jul 9 2020, 7:41 AM

Logically your patch here looks fine. I'd rather see it use a std::lock_guard as the original code did, with a nested scope added, but that is pretty nitpicky.

Also the code doesn't conform to LLVM's coding standards for variable naming (https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).

mwezdeck updated this revision to Diff 276941.Jul 10 2020, 1:04 AM

Thank you very much for explanation. Indeed, the context where user loads an so file thousands times a day makes requirement for startup time very important.

Thanks again for poining this out to me.

I've updated the patch.

Can anybody review this change one more time? I will be really grateful.

Hi gentlemen,

Could you please review this change?

Thank you,
Maksym

Some thoughts:

  • Nit: you only need to call getManagedStaticMutex() once.
  • I would also recommend resetting the ManagedStaticMutex pointer to null after deleting the mutex to help with scary dangling pointer bugs.
  • Since you're deleting the mutex, then by definition this can only be called when there is a single thread touching LLVM. Please document this in the public header. This also means you don't have to acquire the mutex at all.

What happens when someone creates a new managed static after this? This should work. Can you reset the mutex_init_flag somehow?

What happens when someone creates a new managed static after this? This should work. Can you reset the mutex_init_flag somehow?

I think there may be issues beyond just this change that would prevent LLVM from functioning after llvm_shutdown is called. I wouldn't expect this change to make that suddenly work.

In terms of making the flag resettable, it would have to stop using llvm::once_flag. llvm::once_flag is an llvm implementation of std::once_flag for platforms that don't have the std version, and the std version is not resettable.

Ok, then please set the pointer to null - that will cause things to explode if someone tries to use things after llvm_shutdown. Thanks!

mwezdeck updated this revision to Diff 281147.Jul 28 2020, 2:09 AM

Second initialization is required to handle corner cases. It will not happen in real world application. There is call for ManagedStatic's function after llvm_shutdown in DynamicLibrary unit-test, this second initialization is for covering that.

Hello,

Can anybody from reviewers look into this change?

It would be really great if this patch can be merged this month.
Maksym

lattner accepted this revision.Aug 4 2020, 9:17 AM

Looks good, thanks!

This revision is now accepted and ready to land.Aug 4 2020, 9:17 AM

I think I can't push this change to repo by myself. Can I kindly ask somebody with access rights to push commit, to push this change to repo? I will be really grateful.

Thank you one more time.

Can anybody push this commit to repo?

Thanks,
Maksym

Having different behavior for tests V production worries me a bit.

How would people feel about this instead:

static std::recursive_mutex *getManagedStaticMutex() {
  static std::recursive_mutex m;
  return &m;
}

(& leave llvm_shutdown as-is)

No global ctor - lazily constructed. No global dtor... hmm, libstdc++ has a trivial dtor at least when I look at it one way (https://godbolt.org/z/ae6j7f) , but libc++'s is non-trivial.

Is the additional complexity worth saving this one global dtor? (assuming I'm even right that the approach I'm suggesting would address the issue under discussion in this review)

I certainly have some empathy for the consistency of avoiding all global ctors/dtors, but this one's a bit of a case of "who watches the watchmen" - can't really add another mutex to guard the initialization of this mutex. Could use atomic ops but that's tricky to get right/maintain.

We need probably opinion from other reviewers. @lattner , @beanz - what do you think about David's proposition?

mwezdeck updated this revision to Diff 290240.Sep 7 2020, 4:38 AM

I'm uploading new revision, according to suggestions above.