This is an archive of the discontinued LLVM Phabricator instance.

Force RegisterStandardPasses to construct std::function in the IPO library.
ClosedPublic

Authored by marsupial on May 24 2017, 1:29 PM.

Details

Summary

Fixes an issue using RegisterStandardPasses from a statically linked object before PassManagerBuilder::addGlobalExtension is called from a dynamic library.

RegisterStandardPasses used to accept a lambda, which when used from a dynamic library/plugin was allocated in the address space of the library.
If RegisterStandardPasses was used from the within the owning process first it would create the GlobalExtensions ManagedStatic.
GlobalExtensions would then exist in the ManagedStatic list before the DynamicLibrary::OpenedHandles Managed static.
So when the ManagedStatics were tore down, the memory holding the destructor of the lambda would have been released before GlobalExtensions attempts to use it to destruct the lambda.

To fix this we disallow using lambdas from RegisterStandardPasses.

Diff Detail

Event Timeline

marsupial created this revision.May 24 2017, 1:29 PM
marsupial updated this revision to Diff 100158.May 24 2017, 1:41 PM

Added test for this specific case (ManagedStatic).
The target_link_libraries is valid when building as components, but I'm unclear if it is ok for static builds or platforms other than *nix.

efriedma edited edge metadata.

This doesn't fix the crash I described in https://reviews.llvm.org/D30107#763683 .

What are the test results from running unittests/Support/DynamicLibrary/DynamicLibraryTests

Works fine for me.


Regardless, the patch seems like a good idea.

I'm sorry.
You're saying adding the changes to Hello.cpp,
make -j8 opt LLVMHello
bin/opt -load=lib/LLVMHello.so -S /dev/null
is still crashing?

This patch definitively fixes that problem here.

I'll try building clean with the instruction for polly (i.e. no cmake flags).
Could you do the same?

I'm sorry.
You're saying adding the changes to Hello.cpp,
make -j8 opt LLVMHello
bin/opt -load=lib/LLVMHello.so -S /dev/null
is still crashing?

This patch definitively fixes that problem here.

I'll try building clean with the instruction for polly (i.e. no cmake flags).
Could you do the same?

On Ubuntu 14.04. Built with no CMake flags, r303808, with this patch plus my changes to LLVMHello.

$ bin/opt -load=lib/LLVMHello.so -S /dev/null
; ModuleID = '/dev/null'
source_filename = "/dev/null"
Segmentation fault

addGlobalExtension is documented to be used by plugins, where you are using it for a statically linked lib.
There doesn't seem to be any great solution and I going back to how it was (leaking) is one of the worse options.

All I can see is:

  1. Add an argument to RegisterStandardPasses that will choose a call to addGlobalExtension or addExtension.
  2. Make RegisterManagedStatic and llvm_shutdown explicitly re-initialize/destroy the DynamicLibrary state.

Perhaps we can try to close this one out before moving on to the larger problem?

I'm not trying to pass the buck here, this has in fact lead to finding two interesting bugs, but it certainly seems there's an API misuse/deficiency that super-cedes this issue. Even if the ManagedStatic ordering is completely sorted out, how is Polly handling the valid is case of a user using llvm_shutdown. That is once llvm_shutdown has been GlobalExtensions will have been cleared and the passes no longer available.

addGlobalExtension is documented to be used by plugins, where you are using it for a statically linked lib.

polly has historically worked as a plugin. In this context, polly isn't technically a "plugin" in the sense that it's not dynamically loaded, but it works like a plugin in every other sense (using the same extension points a plugin would use to integrate with LLVM).

Add an argument to RegisterStandardPasses that will choose a call to addGlobalExtension or addExtension.

addGlobalExtension is static, addExtension is not; I don't follow how this would work.

Make RegisterManagedStatic and llvm_shutdown explicitly re-initialize/destroy the DynamicLibrary state.

This should work.

I'm not trying to pass the buck here, this has in fact lead to finding two interesting bugs, but it certainly seems there's an API misuse/deficiency that super-cedes this issue. Even if the ManagedStatic ordering is completely sorted out, how is Polly handling the valid is case of a user using llvm_shutdown. That is once llvm_shutdown has been GlobalExtensions will have been cleared and the passes no longer available.

That might be a problem...? I'm not sure how people use llvm_shutdown in practice. (For clang/opt/etc. this doesn't matter because we don't run any LLVM passes after the call to llvm_shutdown.)

polly has historically worked as a plugin. In this context, polly isn't technically a "plugin" in the sense that it's not dynamically loaded, but it works like a plugin in every other sense (using the same extension points a plugin would use to integrate with LLVM).

I get that but as per the comment above how does it handle the use case (regardless of this bug):

{
  llvm_shutdown_obj Shutdown;
  // Do some stuff (Polly passes available via RegisterStandardPasses)
}
// All passes added via RegisterStandardPasses have now been removed
{
  llvm_shutdown_obj Shutdown;
  // Do some other stuff (Polly passes no longer available)
}
marsupial updated this revision to Diff 100291.May 25 2017, 1:34 PM

This should fix the issue and adds a test case of what you're seeing.
Not sure if I want to be the one committing this as it's a bit ugly...

{
  llvm_shutdown_obj Shutdown;
  // Do some stuff (Polly passes available via RegisterStandardPasses)
}
// All passes added via RegisterStandardPasses have now been removed
{
  llvm_shutdown_obj Shutdown;
  // Do some other stuff (Polly passes no longer available)
}

Yes, this doesn't work at the moment.

The tricky part here is that we support both building both polly as a plugin and statically linking polly as part of the same build... but we could make it work, I guess, if people actually use llvm_shutdown_obj like this. (We could split the RegisterStandardPasses off into a separate object file, and only link it in when we're building the plugin.)

lib/Transforms/IPO/PassManagerBuilder.cpp
200

This change is assuming addGlobalExtension is special, i.e. destructors for other ManagedStatics won't run into this issue. That seems a little fragile... would it make sense to do this in ManagedStatic itself instead (i.e. ensure HandleSet is always constructed first/destroyed last)?

marsupial updated this revision to Diff 100352.May 25 2017, 6:29 PM
marsupial retitled this revision from Fix the ManagedStatic list ordering when using DynamicLibrary::addPermanentLibrary. to Add overloads for RegisterStandardPasses and PassManagerBuilder::addGlobalExtension.
marsupial edited the summary of this revision. (Show Details)
marsupial removed a reviewer: lhames.

This works here and is way less of a hack.

Overall I agree there should probably be a better mechanism to avoid this for other possibilities but that will require a lot more time.
If this is good enough maybe close it out and can report a bug on the underlying ManagedStatic issue.

efriedma added inline comments.May 26 2017, 11:51 AM
include/llvm/Transforms/IPO/PassManagerBuilder.h
183

Overloading like this seems a little dubious; if the ExtensionProc overload is the only one that's actually reliably usable, we shouldn't expose the ExtensionFn overload.

marsupial marked an inline comment as done.
marsupial retitled this revision from Add overloads for RegisterStandardPasses and PassManagerBuilder::addGlobalExtension to Force RegisterStandardPasses to construct std::function in the IPO library..
marsupial added inline comments.May 26 2017, 12:23 PM
include/llvm/Transforms/IPO/PassManagerBuilder.h
183

There is code that uses this via lambdas in LLVM.
Updated to limit RegisterStandardPasses to only accept a function pointer.

Formatting errors.

efriedma accepted this revision.Jun 7 2017, 2:50 PM

Oh, sorry, about the delay, I lost track of this.

LGTM.

This revision is now accepted and ready to land.Jun 7 2017, 2:50 PM
marsupial updated this revision to Diff 102357.Jun 13 2017, 9:40 AM

Conflict resolution & additional comment.

This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Jun 13 2017, 10:55 AM
llvm/trunk/include/llvm/Transforms/IPO/PassManagerBuilder.h
213 ↗(On Diff #102359)

I'm missing the *why* using a "real function" (I guess you meant "function pointer") matter?

marsupial updated this revision to Diff 102414.Jun 13 2017, 2:20 PM

Add explicit dependency for CMake when built with LLVM_ENABLE_MODULES.

marsupial updated this revision to Diff 102457.Jun 13 2017, 5:08 PM

Having trouble with the test part of this and CMake.

The idea of creating a shared lib with no dependencies on LLVM is breaking down for -DCMAKE_BUILD_TYPE=Debug and -DLLVM_ENABLE_MODULES=1.

marsupial updated this revision to Diff 102460.Jun 13 2017, 5:20 PM
marsupial updated this revision to Diff 102544.Jun 14 2017, 7:11 AM

Relocate tests into Transforms/IPO.

Formatting.

I'm missing the *why* using a "real function" (I guess you meant "function pointer") matter?

Don't understand this.
Is the comment really confusing/unhelpful?
Otherwise I'd kind of like to land this and move on.

I'm missing the *why* using a "real function" (I guess you meant "function pointer") matter?

Don't understand this.
Is the comment really confusing/unhelpful?

Yes. First "real" doesn't seem accurate. I'm unsure what it means.
Then I still have no idea why this patch is needed. I don't see any explanation anywhere.

From the summary:

Fixes an issue using RegisterStandardPasses from a statically linked object before PassManagerBuilder::addGlobalExtension is called from a dynamic library.

What would you like either the comment or the summary it to say?

From the summary:

Fixes an issue using RegisterStandardPasses from a statically linked object before PassManagerBuilder::addGlobalExtension is called from a dynamic library.

What would you like either the comment or the summary it to say?

"fix an issue" is not precise enough. What issue? How does it manifest? What is the way to solve it?

If you need examples about what I'm expecting to read from a commit message (same applies to comment in the code though), you may look at these commit messages:

marsupial edited the summary of this revision. (Show Details)Jun 14 2017, 5:17 PM

(If @efriedma thinks that the fix is the right one for the provided explanation that's fine with me)