Page MenuHomePhabricator

[PassManagerBuilder] Remove global extension when a plugin is unloaded
ClosedPublic

Authored by EliaGeretto on Dec 28 2019, 5:49 AM.

Details

Summary

This commit fixes PR39321.

GlobalExtensions is not guaranteed to be destroyed when optimizer plugins are unloaded. If it is indeed destroyed after a plugin is dlclose-d, the destructor of the corresponding ExtensionFn is not mapped anymore, causing a call to unmapped memory during destruction.

This commit guarantees that extensions coming from external plugins are removed from GlobalExtensions when the plugin is unloaded if GlobalExtensions has not been destroyed yet.

Diff Detail

Event Timeline

EliaGeretto created this revision.Dec 28 2019, 5:49 AM

This looks like a better solution to the earlier reported bug. Thanks @EliaGeretto for working on this! I like the general approach. I only have two minor suggestions: (a) add some comments -- especially one that explains why the correct removal order is important for external plugins, (b) consider replacing an emptiness check with a targetted assert.

llvm/include/llvm/Transforms/IPO/PassManagerBuilder.h
200

While the documentation is sparse already, I feel it would be helpful to document the newly introduced GlobalExtensionID.

llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
216

Why do we check if GlobalExtensions is not empty? Should this condition not always be true assuming removeGlobalExtension was called with a valid and registered ExtensionID? If this is the case, would it not make more sense to 'assert' in case this invariant is broken rather than silently ignoring the request to remove an invalid ExtensionID?

Also, rather than checking if GlobalExtensionsNotEmpty holds, should we not check that the specific ExtensionID is in the GlobalExtensions vector?

EliaGeretto added inline comments.Dec 29 2019, 6:49 AM
llvm/include/llvm/Transforms/IPO/PassManagerBuilder.h
200

I will do this in the next revision.

llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
216

It was my mistake to use the GlobalExtensionsNotEmpty function. I am relying on the fact that isConstructed will return false if GlobalExtensions has already been destructed using destroy. In that case, nothing should be done; this is the case if LLVM is compiled without Polly. removeGlobalExtension is meant to do something only if GlobalExtensions was not destroyed yet; this is instead the case when compiling with Polly. I will use isConstructed directly and add a comment.

EliaGeretto edited the summary of this revision. (Show Details)Dec 29 2019, 6:50 AM

I have added comments to explain the reason of some of the changes and added an assert so that, if the removeGlobalExtension function is called with an invalid ID, the error will be more noticeable.

Please, let me know if you think the comments are clear enough.

EliaGeretto marked 4 inline comments as done.Dec 29 2019, 10:43 AM
grosser added a subscriber: philip.pfaffe.

This looks good from my side.

As this is touching some of the core parts of LLVM it would be good to get another review on this. Either @chandlerc or @philip.pfaffe maybe?

mehdi_amini accepted this revision.Jan 25 2020, 8:44 PM
mehdi_amini added inline comments.
llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
218

Nit: please use early-exit.

220

llvm::find_if(GlobalExtensions, ...

This revision is now accepted and ready to land.Jan 25 2020, 8:44 PM

removeGlobalExtension now early-exits when GlobalExtensions is not constructed and uses llvm::find_if to iterate over the collection.

EliaGeretto marked 2 inline comments as done.Jan 26 2020, 1:41 AM

@mehdi_amini Thank you for your review. I wanted to let you know that I do not have commit access, so someone else should commit this change for me. In addition, I think that this change could be backported to older LLVM releases, at least those that still get bugfixes. The bug addressed here is around since LLVM 5 it seems, as discussed here: https://bugs.llvm.org/show_bug.cgi?id=39321

@Meinersbur @serge-sans-paille I have quickly taken a look at the other pull request and yes, it seems a fix for the exact same problem, discussed in PR39321.

@Meinersbur I confirm it seems to solve the same issue, and I prefer the locality of this solution :-) Thanks @EliaGeretto

This revision was automatically updated to reflect the committed changes.

@serge-sans-paille The test was still failing on macOS.

http://lab.llvm.org:8080/green/job/clang-stage1-cmake-RA-incremental/9061/consoleFull

******************** TEST 'LLVM :: Feature/load_extension.ll' FAILED ********************
Script:
--
: 'RUN: at line 1';   /Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/clang-build/bin/opt /Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/test/Feature/load_extension.ll -load=/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/clang-build/./lib/Bye.dylib -goodbye -wave-goodbye -disable-output 2>&1 | /Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/clang-build/bin/FileCheck /Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/test/Feature/load_extension.ll
--
Exit Code: 1
Hide Details


Command Output (stderr):
--
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/test/Feature/load_extension.ll:3:10: error: CHECK: expected string not found in input
; CHECK: Bye
         ^
<stdin>:1:1: note: scanning from here
opt: CommandLine Error: Option 'attributor-max-iterations' registered more than once!
^
<stdin>:1:14: note: possible intended match here
opt: CommandLine Error: Option 'attributor-max-iterations' registered more than once!

I disabled it again.

commit 3524755a1a25c18ecf47e50e5186232da0751cfe (HEAD -> master, origin/master, origin/HEAD)
Author: Jan Korous <jkorous@apple.com>
Date:   Wed Feb 5 11:03:38 2020 -0800

    Revert "Activate extension loading test on Darwin now that the underlying fix has landed"

    This reverts commit 058070893428a480b76a137f647ae6b9c89ac2d4.

@serge-sans-paille The test was still failing on macOS.

As far as I can tell, it's no longer the case after the various patch update. I re-enabled the test to assert that in d00900801aae04e6dfde7eb4ca2811d9a6887cf8