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

Don't internalize symbols used by global_asm #97900

Closed
wants to merge 1 commit into from

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Jun 8, 2022

Symbols that are only reachable from the current CGU have their
visibility changed to internal to allow LLVM to better optimize them.

However this doesn't work for symbols that are used in global_asm!:
the symbol name is inserted into the assembly code as text, which LLVM
is not aware of. In particular, LLVM may delete the symbol in the
MergeFunctions pass if it can't see any other uses of it.

The solution here is to ensure that symbols used by global_asm! are
always marked with external linkage.

Fixes #96797

cc @nbdd0121

Symbols that are only reachable from the current CGU have their
visibility changed to internal to allow LLVM to better optimize them.

However this doesn't work for symbols that are used in `global_asm!`:
the symbol name is inserted into the assembly code as text, which LLVM
is not aware of. In particular, LLVM may delete the symbol in the
MergeFunctions pass if it can't see any other uses of it.

The solution here is to ensure that symbols used by `global_asm!` are
always marked with external linkage.

Fixes rust-lang#96797
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 8, 2022
@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 8, 2022
Copy link
Contributor

@nbdd0121 nbdd0121 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic LGTM.

Would it make more sense to gather this info during collection though? Given that this info is needed regardless, it might be easier just to do it in collect_items_rec in collector.rs.

@Amanieu
Copy link
Member Author

Amanieu commented Jun 9, 2022

I'm actually looking at a potential fix in LLVM instead. Marking as draft for now.

@Amanieu Amanieu marked this pull request as draft June 9, 2022 23:42
@estebank
Copy link
Contributor

Ping me if the LLVM patch doesn't gain traction, although I think someone else in t-compiler should review this.

@Amanieu
Copy link
Member Author

Amanieu commented Jun 14, 2022

Closing in favor of LLVM patch: https://reviews.llvm.org/D127751

@Amanieu Amanieu closed this Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Functions referenced by global_asm merged into other functions by LLVM
5 participants