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.

Sign up for GitHub

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

Jump to bottom

Mangle rustc_std_internal_symbols functions #127173

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Mangle rustc_std_internal_symbols functions #127173

wants to merge 6 commits into from

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Jun 30, 2024 •
edited
Loading

This reduces the risk of issues when using a staticlib or rust dylib compiled with a different rustc version in a rust program. Currently this will either (in the case of staticlib) cause a linker error due to duplicate symbol definitions, or (in the case of rust dylibs) cause rustc_std_internal_symbols functions to be silently overridden. As rust gets more commonly used inside the implementation of libraries consumed with a C interface (like Spidermonkey, Ruby YJIT (curently has to do partial linking of all rust code to hide all symbols not part of the C api), the Rusticl OpenCL implementation in mesa) this is becoming much more of an issue. With this PR the only symbols remaining with an unmangled name are rust_eh_personality (LLVM doesn't allow renaming it) and __rust_no_alloc_shim_is_unstable.

Helps mitigate #104707

@rustbot
Copy link
Collaborator

rustbot commented Jun 30, 2024

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added 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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 30, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 30, 2024

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3 bjorn3 force-pushed the mangle_rustc_std_internal_symbol branch from aea9a3c to db57b2a Compare June 30, 2024 16:50
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3 bjorn3 force-pushed the mangle_rustc_std_internal_symbol branch from db57b2a to 4859689 Compare June 30, 2024 17:27
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jun 30, 2024

☔ The latest upstream changes (presumably #127162) made this pull request unmergeable. Please resolve the merge conflicts.

@bjorn3 bjorn3 force-pushed the mangle_rustc_std_internal_symbol branch from 4859689 to 10bad43 Compare June 30, 2024 19:57
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot
Copy link
Collaborator

rustbot commented Jun 30, 2024

The Miri subtree was changed

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@fee1-dead
Copy link
Member

r? compiler

@rustbot rustbot assigned fmease and unassigned fee1-dead Jul 1, 2024
@bjorn3 bjorn3 mentioned this pull request Jul 1, 2024
Merged
@bjorn3 bjorn3 marked this pull request as draft July 1, 2024 17:41
@bjorn3
Copy link
Member Author

bjorn3 commented Jul 1, 2024

Opened rust-lang/miri#3724 with some miri changes that can be made independently of this PR.

bors added a commit to rust-lang/miri that referenced this pull request Jul 2, 2024
@bors
Auto merge of #3724 - bjorn3:use_symbol_name_query, r=RalfJung
4b454fd 
Use the symbol_name query instead of trying to infer from the link_name attribute

This prevents the calculated name from going out of sync with exported_symbols. It also avoids having to special case the panic_impl lang item.

It also makes it easier to fix miri with rust-lang/rust#127173.
@bors
Copy link
Contributor

bors commented Jul 3, 2024

☔ The latest upstream changes (presumably #125507) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung pushed a commit to RalfJung/rust that referenced this pull request Jul 4, 2024
@bors
Auto merge of rust-lang#3724 - bjorn3:use_symbol_name_query, r=RalfJung
4a7d29f 
Use the symbol_name query instead of trying to infer from the link_name attribute

This prevents the calculated name from going out of sync with exported_symbols. It also avoids having to special case the panic_impl lang item.

It also makes it easier to fix miri with rust-lang#127173.
@bjorn3 bjorn3 force-pushed the mangle_rustc_std_internal_symbol branch from e1d353a to f8f4b88 Compare July 5, 2024 11:55
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3 bjorn3 force-pushed the mangle_rustc_std_internal_symbol branch from f8f4b88 to 266c7c2 Compare July 5, 2024 12:56
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3 bjorn3 force-pushed the mangle_rustc_std_internal_symbol branch from 266c7c2 to 9c91546 Compare July 5, 2024 13:35
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 10, 2024
@bors
Auto merge of rust-lang#127173 - bjorn3:mangle_rustc_std_internal_sym…
e1a56e0 
…bol, r=<try>

Mangle rustc_std_internal_symbols functions

This reduces the risk of issues when using a staticlib or rust dylib compiled with a different rustc version in a rust program. Currently this will either (in the case of staticlib) cause a linker error due to duplicate symbol definitions, or (in the case of rust dylibs) cause rustc_std_internal_symbols functions to be silently overridden. As rust gets more commonly used inside the implementation of libraries consumed with a C interface (like Spidermonkey, Ruby YJIT (curently has to do partial linking of all rust code to hide all symbols not part of the C api), the Rusticl OpenCL implementation in mesa) this is becoming much more of an issue. With this PR the only symbols remaining with an unmangled name are rust_eh_personality (LLVM doesn't allow renaming it) and `__rust_no_alloc_shim_is_unstable`.

Helps mitigate rust-lang#104707
@bors
Copy link
Contributor

bors commented Aug 10, 2024

⌛ Trying commit e449918 with merge e1a56e0...

@bors
Copy link
Contributor

bors commented Aug 10, 2024

☀️ Try build successful - checks-actions
Build commit: e1a56e0 (e1a56e052808ee443f261e7967696645e2371b8a)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e1a56e0): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 1.9% [1.9%, 1.9%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -0.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.8% [-0.8%, -0.8%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.8% [-0.8%, -0.8%] 1

Cycles

Results (primary 2.6%, secondary 2.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 2.6% [2.6%, 2.6%] 1
Regressions ❌
(secondary)		 2.7% [2.1%, 3.3%] 5
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 2.6% [2.6%, 2.6%] 1

Binary size

Results (primary 0.1%, secondary 0.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.1% [0.0%, 0.1%] 19
Regressions ❌
(secondary)		 0.1% [0.1%, 0.2%] 40
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.1% [0.0%, 0.1%] 19

Bootstrap: 760.23s -> 758.994s (-0.16%)
Artifact size: 339.28 MiB -> 339.31 MiB (0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 10, 2024
@bjorn3 bjorn3 marked this pull request as ready for review August 10, 2024 17:54
@bjorn3
Copy link
Member Author

bjorn3 commented Aug 10, 2024

The miri issue has been fixed. Ready for review.

@bjorn3 bjorn3 mentioned this pull request Sep 26, 2024
Merged
@bjorn3 bjorn3 mentioned this pull request Oct 17, 2024
Open
@wesleywiser
Copy link
Member

r? rust-lang/compiler

@rustbot rustbot assigned fee1-dead and unassigned fmease Oct 31, 2024
@wesleywiser
Copy link
Member

That re-rolled the original reviewer.

r? rust-lang/compiler

@rustbot rustbot assigned wesleywiser and unassigned fee1-dead Oct 31, 2024
@bjorn3 bjorn3 force-pushed the mangle_rustc_std_internal_symbol branch from e449918 to 4e87ee1 Compare November 1, 2024 11:24
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3 bjorn3 force-pushed the mangle_rustc_std_internal_symbol branch 2 times, most recently from d0b59d6 to 39a1059 Compare November 1, 2024 12:52
@bors
Copy link
Contributor

bors commented Nov 2, 2024

☔ The latest upstream changes (presumably #132512) made this pull request unmergeable. Please resolve the merge conflicts.

@bjorn3 bjorn3 force-pushed the mangle_rustc_std_internal_symbol branch from 39a1059 to 43723e1 Compare November 3, 2024 14:58
@bors
Copy link
Contributor

bors commented Nov 4, 2024

☔ The latest upstream changes (presumably #132581) made this pull request unmergeable. Please resolve the merge conflicts.

@bjorn3 bjorn3 force-pushed the mangle_rustc_std_internal_symbol branch from 43723e1 to 87cb63d Compare November 7, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@wesleywiser wesleywiser

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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@bjorn3 @rustbot @rust-log-analyzer @bors @fee1-dead @rust-timer @wesleywiser @fmease