Mangle rustc_std_internal_symbols functions #127173
Merged
+342
−76
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @fee1-dead rustbot has assigned @fee1-dead. Use |
rustbot
added
S-waiting-on-review
|
Some changes occurred in compiler/rustc_codegen_gcc |
This comment has been minimized.
This comment has been minimized.
bjorn3
force-pushed
the
mangle_rustc_std_internal_symbol
branch
from
aea9a3c to
db57b2a
Compare
This comment has been minimized.
This comment has been minimized.
bjorn3
force-pushed
the
mangle_rustc_std_internal_symbol
branch
from
db57b2a to
4859689
Compare
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #127162) made this pull request unmergeable. Please resolve the merge conflicts. |
bjorn3
force-pushed
the
mangle_rustc_std_internal_symbol
branch
from
4859689 to
10bad43
Compare
This comment has been minimized.
This comment has been minimized.
|
The Miri subtree was changed cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
|
r? compiler |
|
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
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.
|
☔ 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
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
force-pushed
the
mangle_rustc_std_internal_symbol
branch
from
e1d353a to
f8f4b88
Compare
This comment has been minimized.
This comment has been minimized.
bjorn3
force-pushed
the
mangle_rustc_std_internal_symbol
branch
from
f8f4b88 to
266c7c2
Compare
This comment has been minimized.
This comment has been minimized.
bjorn3
force-pushed
the
mangle_rustc_std_internal_symbol
branch
from
266c7c2 to
9c91546
Compare
|
☀️ Test successful - checks-actions |
|
This is an experimental post-merge analysis report. You can ignore it. Post-merge reportComparing 43a2e9d (base) -> 493c38b (this PR) Test differences
Additionally, 10 doctest diffs were found. These are ignored, as they are noisy. Job group index
|
|
Finished benchmarking commit (493c38b): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResults (primary 0.0%, 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.
Bootstrap: 774.655s -> 775.508s (0.11%) |
|
The job Click to see the possible cause of the failure (guessed by this bot)For more information how to resolve CI failures of this job, visit this link. |
RalfJung
reviewed
Mar 18, 2025
There was a problem hiding this comment.
The changes here look like they will end up calling mangle_internal_symbol around 10 times for every single time that a shim is invoked in Miri. That does not seem great?
There was a problem hiding this comment.
Right. Didn't think about that. I guess it would be possible to first check if the unmangled name is a substring and only then call mangle_internal_symbol.
There was a problem hiding this comment.
Having a cache for the mangled name in Miri would also work, and would avoid having to rely on any particular property of the mangled name.
RalfJung
reviewed
Mar 18, 2025
| let attrs = tcx.codegen_fn_attrs(def_id); | ||
| // FIXME use tcx.symbol_name(instance) instead |
There was a problem hiding this comment.
I'm not sure how to understand this comment. Is there some reason this can't just be changed now?
There was a problem hiding this comment.
iter_exported_symbols returns DefId's, not Instance's like tcx.symbol_name() needs.
There was a problem hiding this comment.
These are monomorphic symbols though, right? So we could use Instance::mono.
tgross35
added a commit
to tgross35/compiler-builtins
that referenced
this pull request
Mar 19, 2025
`builtins-test-intrinsics` has long included a call to an unmangled `rust_begin_unwind` (the name `rustc` gives the `#[panic_handler]`), since [1], which I believe was intended to ensure panic machinery links correctly. However, since [2], `rust_begin_unwind` is mangled and unavailable for calling directly, which explains the recent CI failures. Instead of calling the function directly, we can just panic; do so here. Additionally, put this call behind `black_box(false)` rather than unconditional, which means we can run the binary and ensure there are no runtime issues. [1]: rust-lang#360 [2]: rust-lang/rust#127173
tgross35
added a commit
to tgross35/compiler-builtins
that referenced
this pull request
Mar 19, 2025
`builtins-test-intrinsics` has long included a call to an unmangled `rust_begin_unwind` (the name `rustc` gives the `#[panic_handler]`), since [1], which I believe was intended to ensure panic machinery links correctly. However, since [2], `rust_begin_unwind` is mangled and unavailable for calling directly, which explains the recent CI failures. Instead of calling the function directly, we can just panic; do so here. Additionally, put this call behind `black_box(false)` rather than unconditional, which means we can run the binary and ensure there are no runtime issues. [1]: rust-lang#360 [2]: rust-lang/rust#127173
tgross35
added a commit
to tgross35/compiler-builtins
that referenced
this pull request
Mar 19, 2025
`builtins-test-intrinsics` has long included a call to an unmangled `rust_begin_unwind` (the name `rustc` gives the `#[panic_handler]`), since [1], which I believe was intended to ensure panic machinery links correctly. However, since [2], `rust_begin_unwind` is mangled and unavailable for calling directly, which explains the recent CI failures. Instead of calling the function directly, we can just panic; do so here. Additionally, put this call behind `black_box(false)` rather than unconditional, which means we can run the binary and ensure there are no runtime issues. [1]: rust-lang#360 [2]: rust-lang/rust#127173
tgross35
added a commit
to tgross35/compiler-builtins
that referenced
this pull request
Mar 19, 2025
`builtins-test-intrinsics` has long included a call to an unmangled `rust_begin_unwind` (the name `rustc` gives the `#[panic_handler]`), since [1], which I believe was intended to ensure panic machinery links correctly. However, since [2], `rust_begin_unwind` is mangled and unavailable for calling directly, which explains the recent CI failures. Instead of calling the function directly, we can just panic; do so here. Additionally, put this call behind `black_box(false)` rather than unconditional, which means we can run the binary and ensure there are no runtime issues. [1]: rust-lang#360 [2]: rust-lang/rust#127173
tgross35
added a commit
to tgross35/compiler-builtins
that referenced
this pull request
Mar 19, 2025
`builtins-test-intrinsics` has long included a call to an unmangled `rust_begin_unwind` (the name `rustc` gives the `#[panic_handler]`), since [1], which I believe was intended to ensure panic machinery links correctly. However, since [2], `rust_begin_unwind` is mangled and unavailable for calling directly, which explains the recent CI failures. Instead of calling the function directly, we can just panic; do so here. Additionally, put this call behind `black_box(false)` rather than unconditional, which means we can run the binary and ensure there are no runtime issues. [1]: rust-lang#360 [2]: rust-lang/rust#127173
tgross35
added a commit
to tgross35/compiler-builtins
that referenced
this pull request
Mar 19, 2025
`builtins-test-intrinsics` has long included a call to an unmangled `rust_begin_unwind` (the name `rustc` gives the `#[panic_handler]`), since [1], which I believe was intended to ensure panic machinery links correctly. However, since [2], `rust_begin_unwind` is mangled and unavailable for calling directly, which explains the recent CI failures. Instead of calling the function directly, we can just panic; do so here. Additionally, put this call behind `black_box(false)` rather than unconditional, which means we can run the binary and ensure there are no runtime issues. [1]: rust-lang#360 [2]: rust-lang/rust#127173
tgross35
added a commit
to tgross35/compiler-builtins
that referenced
this pull request
Mar 19, 2025
`builtins-test-intrinsics` has long included a call to an unmangled `rust_begin_unwind` (the name `rustc` gives the `#[panic_handler]`), since [1], which I believe was intended to ensure panic machinery links correctly. However, since [2], `rust_begin_unwind` is mangled and unavailable for calling directly, which explains the recent CI failures. Instead of calling the function directly, we can just panic; do so here. Additionally, put this call behind `black_box(false)` rather than unconditional, which means we can run the binary and ensure there are no runtime issues. [1]: rust-lang#360 [2]: rust-lang/rust#127173
tgross35
added a commit
to tgross35/compiler-builtins
that referenced
this pull request
Mar 19, 2025
`builtins-test-intrinsics` has long included a call to an unmangled `rust_begin_unwind` (the name `rustc` gives the `#[panic_handler]`), since [1], which I believe was intended to ensure panic machinery links correctly. However, since [2], `rust_begin_unwind` is mangled and unavailable for calling directly, which explains the recent CI failures. Instead of calling the function directly, we can just panic; do so here. Additionally, put this call behind `black_box(false)` rather than unconditional, which means we can run the binary and ensure there are no runtime issues. [1]: rust-lang#360 [2]: rust-lang/rust#127173
tgross35
added a commit
to tgross35/compiler-builtins
that referenced
this pull request
Mar 19, 2025
Since [1] this symbol is mangled, meaning it is not easy to call directly. A better fix will come in [2] but for now, just disable that portion of the test. [1]: rust-lang/rust#127173 [2]: rust-lang#802
tgross35
added a commit
to rust-lang/compiler-builtins
that referenced
this pull request
Mar 19, 2025
Since [1] this symbol is mangled, meaning it is not easy to call directly. A better fix will come in [2] but for now, just disable that portion of the test. [1]: rust-lang/rust#127173 [2]: #802
|
Oh man, this broke some things for us. We have a pipeline where we need to link a static Rust library to a full Rust program (we had been doing symbol renaming to get this functional) as we need different sanitization levels between the two. cc @tokatoka @rmalmain, this is our CI failure w/ libafl_libfuzzer. |
|
@addisoncrump It sounds like you are deep in unspecified territory and are bypassing things that have been explicitly introduced to avoid unstable implementation details leaking out. This is definitely off-label usage of rustc. Might be worth having a discussion on Zulip or so to figure out if there isn't a less cursed way to support your usecase. |
Please don't comment on an already merged PR, it's impossible to find and has no visibility. Consider opening an issue or zulip thread if you want to discuss your use case. |
Yup, we slammed this one together because of some niche linkage requirements from a sanitizer. I'll ask on Zulip, but we had previously discussed this with others and this was the best conclusion we came up with 🙃
Sure, we figured this change was generally accepted and our use case is outside of what is considered normally permissible. Just left a comment as that: a comment. No action required, we work around it. |
|
I've opened a Zulip thread for this discussion: https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Linking.20Rust.20libraries.20with.20different.20sanitizers |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
try-job: aarch64-gnu-debug
try-job: aarch64-apple
try-job: x86_64-apple-1
try-job: x86_64-mingw-1
try-job: i686-mingw-1
try-job: x86_64-msvc-1
try-job: i686-msvc-1
try-job: test-various
try-job: armhf-gnu