Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Register LLVM passes with the correct LLVM pass manager. #31176

Merged
merged 1 commit into from
Jan 25, 2016

Conversation

frewsxcv
Copy link
Member

Register LLVM passes with the correct LLVM pass manager.

LLVM was upgraded to a new version in this commit:

f9d4149

which was part of this pull request:

#26025

Consider the following two lines from that commit:

f9d4149#diff-a3b24dbe2ea7c1981f9ac79f9745f40aL462

f9d4149#diff-a3b24dbe2ea7c1981f9ac79f9745f40aL469

The purpose of these lines is to register LLVM passes. Prior to the that
commit, the passes being handled were assumed to be ModulePasses (a
specific type of LLVM pass) since they were being added to a ModulePass
manager. After that commit, both lines were refactored (presumably in an
attempt to DRY out the code), but the ModulePasses were changed to be
registered to a FunctionPass manager. This change resulted in
ModulePasses being run, but a Function object was being passed as a
parameter to the pass instead of a Module, which resulted in
segmentation faults.

In this commit, I changed relevant sections of the code to check the
type of the passes being added and register them to the appropriate pass
manager.

Closes #31067

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Aatch (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@frewsxcv
Copy link
Member Author

Thanks @eddyb and @ubsan for helping me with this!

@frewsxcv frewsxcv mentioned this pull request Jan 25, 2016
LLVM was upgraded to a new version in this commit:

rust-lang@f9d4149

which was part of this pull request:

rust-lang#26025

Consider the following two lines from that commit:

rust-lang@f9d4149#diff-a3b24dbe2ea7c1981f9ac79f9745f40aL462

rust-lang@f9d4149#diff-a3b24dbe2ea7c1981f9ac79f9745f40aL469

The purpose of these lines is to register LLVM passes. Prior to the that
commit, the passes being handled were assumed to be ModulePasses (a
specific type of LLVM pass) since they were being added to a ModulePass
manager. After that commit, both lines were refactored (presumably in an
attempt to DRY out the code), but the ModulePasses were changed to be
registered to a FunctionPass manager. This change resulted in
ModulePasses being run, but a Function object was being passed as a
parameter to the pass instead of a Module, which resulted in
segmentation faults.

In this commit, I changed relevant sections of the code to check the
type of the passes being added and register them to the appropriate pass
manager.

Closes rust-lang#31067
@shahn
Copy link
Contributor

shahn commented Jan 25, 2016

Yes please! I think this has implications much broader than known here, I've been trying to work on getting gcov profiling to work but inserting the gcov profiling pass always segfaulted for me which gets fixed with this commit as well.

@dotdash
Copy link
Contributor

dotdash commented Jan 25, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Jan 25, 2016

📌 Commit d942621 has been approved by dotdash

@bors
Copy link
Contributor

bors commented Jan 25, 2016

⌛ Testing commit d942621 with merge c22cb53...

bors added a commit that referenced this pull request Jan 25, 2016
Register LLVM passes with the correct LLVM pass manager.

LLVM was upgraded to a new version in this commit:

f9d4149

which was part of this pull request:

#26025

Consider the following two lines from that commit:

f9d4149#diff-a3b24dbe2ea7c1981f9ac79f9745f40aL462

f9d4149#diff-a3b24dbe2ea7c1981f9ac79f9745f40aL469

The purpose of these lines is to register LLVM passes. Prior to the that
commit, the passes being handled were assumed to be ModulePasses (a
specific type of LLVM pass) since they were being added to a ModulePass
manager. After that commit, both lines were refactored (presumably in an
attempt to DRY out the code), but the ModulePasses were changed to be
registered to a FunctionPass manager. This change resulted in
ModulePasses being run, but a Function object was being passed as a
parameter to the pass instead of a Module, which resulted in
segmentation faults.

In this commit, I changed relevant sections of the code to check the
type of the passes being added and register them to the appropriate pass
manager.

Closes #31067
@bors bors merged commit d942621 into rust-lang:master Jan 25, 2016
@frewsxcv frewsxcv deleted the incorrect-pass-kind branch January 25, 2016 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants