Skip to content

Mangle rustc_std_internal_symbols functions #127173

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

Merged
merged 9 commits into from
Mar 18, 2025

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Jun 30, 2024

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

@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.

@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.

@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.

@bors
Copy link
Collaborator

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.

@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.

@fee1-dead
Copy link
Member

r? compiler

@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
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
Collaborator

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
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.

@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.

@bjorn3 bjorn3 force-pushed the mangle_rustc_std_internal_symbol branch from 266c7c2 to 9c91546 Compare July 5, 2024 13:35
@addisoncrump
Copy link

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.

@RalfJung
Copy link
Member

@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.

@jieyouxu
Copy link
Member

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.

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.

@addisoncrump
Copy link

addisoncrump commented Mar 20, 2025

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.

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 🙃

it's impossible to find and has no visibility

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.

@addisoncrump
Copy link

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

@Freax13
Copy link
Contributor

Freax13 commented Mar 22, 2025

This PR broke panic handlers that also happen to have #[no_mangle] on them:

#[panic_handler]
#[no_mangle]
fn panic(info: &PanicInfo) -> ! {
    todo!()
}
error: linking with `rust-lld` failed: exit status: 1
  |
  = note:  "rust-lld" "-flavor" "gnu" "--script=linker.ld" "--gc-sections" "/tmp/nix-shell-268986-0/rustcOzjusq/symbols.o" "<4 object files omitted>" "--as-needed" "-Bstatic" "/home/freax13/code/bootloader/target/x86_64-bootloader/release/deps/{libxmas_elf-66bc2f3042db6d0a.rlib,libzero-4ac7a3e5a275490c.rlib,libx86_64-071c4d87b3f41b86.rlib,libvolatile-3494b28e1872b638.rlib,libbitflags-c139daa6402e315d.rlib,libbit_field-44e684cd92ae48a9.rlib,libusize_conversions-f8d542993493606b.rlib,libfixedvec-047d254b673107be.rlib,libbootloader-23a5442f6f2666f3.rlib,librlibc-fbafd460f1ad3018.rlib,librustc_std_workspace_core-8032938bf1041ab0.rlib,libcore-6b618e1a3fa2eac5.rlib,libcompiler_builtins-e7ef8faa94df0138.rlib}.rlib" "-L" "/tmp/nix-shell-268986-0/rustcOzjusq/raw-dylibs" "-Bdynamic" "--eh-frame-hdr" "-z" "noexecstack" "-L" "/home/freax13/code/bootloader/target/x86_64-bootloader/release/build/bootloader-b78ad0a097d73cb6/out" "-o" "/home/freax13/code/bootloader/target/x86_64-bootloader/release/deps/bootloader-29b7d286398a0461" "--gc-sections" "-O1"
  = note: some arguments are omitted. use `--verbose` to show all linker arguments
  = note: rust-lld: error: undefined symbol: __rustc::rust_begin_unwind
          >>> referenced by panicking.rs:75 (src/panicking.rs:75)
          >>>               core-6b618e1a3fa2eac5.core.ba490f4106a6f0a6-cgu.12.rcgu.o:(core::panicking::panic_fmt::hf5a770ebcc6a8913) in archive /home/freax13/code/bootloader/target/x86_64-bootloader/release/deps/libcore-6b618e1a3fa2eac5.rlib

Removing the #[no_mangle] attribute makes it work again. Is this considered a bug?

rust-osdev/bootloader v0.9 is affected by this issue: rust-osdev/bootloader#499.

@tgross35
Copy link
Contributor

This PR broke panic handlers that also happen to have #[no_mangle] on them:

This was brought up on Zulip, would you mind following up there? #t-compiler > no_mangle rust_begin_unwind

@zmodem
Copy link
Contributor

zmodem commented Apr 1, 2025

fwiw, chromium is also broken by this due to defining the allocator functions in c++: https://crbug.com/407024458

@bjorn3
Copy link
Member Author

bjorn3 commented Apr 1, 2025

Right, I did at some point see that Chromium defined them in C++, but completely forgot about it. Are you able to fix this on your end before the next stable release of rustc? If not I could add a temporary exception for mangling of the allocator symbols. This exception will have to be removed at some point though.

@RalfJung
Copy link
Member

RalfJung commented Apr 1, 2025

Can't Chromium define chromium_rust_alloc etc symbols in C++, and then in Rust have a #[global_allocator] that dispatches to those symbols?

The __rust_alloc etc symbols are a rustc implementation detail and should never be implemented by anything else.

@zmodem
Copy link
Contributor

zmodem commented Apr 1, 2025

Are you able to fix this on your end before the next stable release of rustc? If not I could add a temporary exception for mangling of the allocator symbols. This exception will have to be removed at some point though.

If the fix Ralf suggested works out, I think we're good.

Can't Chromium define chromium_rust_alloc etc symbols in C++, and then in Rust have a #[global_allocator] that dispatches to those symbols?

I'm not an expert on our Rust build, but yes that sounds like a good solution. Based on https://source.chromium.org/chromium/chromium/src/+/main:build/rust/std/remap_alloc.cc;l=61 it sounds like that was already the plan, but we didn't get to it yet :)

@anforowicz
Copy link
Contributor

Can't Chromium define chromium_rust_alloc etc symbols in C++, and then in Rust have a #[global_allocator] that dispatches to those symbols?

IIUC #[global_allocator] will be analyzed and used when rustc drives the final linking step (*). This is not what happens in Chromium where clang drives the final linking for most executables.

(*) Because linking is the only step when rustc can find the single crate that provides a #[global_allocator]. When compiling other crates rustc won't be aware that this attribute is present in some other crate.

The __rust_alloc etc symbols are a rustc implementation detail and should never be implemented by anything else.

FWIW having Chromium provide a weak definition of __rust_alloc has been discussed as a work around in #73632.

Are you able to fix this on your end before the next stable release of rustc? If not I could add a temporary exception for mangling of the allocator symbols. This exception will have to be removed at some point though.

I don't know. Is there another mechanism that Chromium can use to ask rustc to use a Chromium-specific allocator (PartitionAlloc) when rustc is used to build rlibs, but the final linking is driven by clang? If not, then we would appreciate a temporary exemption to unblock rolling Rust toolchain to a newer version in Chromium.

(Chromium doesn't use the stable release of rustc, because Chromium uses a recent nightly release of clang and therefore needs to also use a recent version of rustc so that LTO can see the same version of LLVM IR.)

@RalfJung
Copy link
Member

RalfJung commented Apr 1, 2025

If you're using a nightly rustc, maybe we can gate this temporary work-around on a -Z flag.

But yeah someone will have to push on #73632 to solve this properly. That might make a good project goal for 2025h2, if the Chromium team is willing to put in the work. :)

@bjorn3
Copy link
Member Author

bjorn3 commented Apr 1, 2025

#[global_allocator] has actually been expanding to direct definitions of __rust_alloc and co. Only when #[global_allocator] is not used, do __rust_alloc and co get generated right before linking.

@zmodem
Copy link
Contributor

zmodem commented Apr 2, 2025

If you're using a nightly rustc, maybe we can gate this temporary work-around on a -Z flag.

That would be very helpful for us. Thanks!

github-merge-queue bot pushed a commit to model-checking/kani that referenced this pull request Apr 2, 2025
Culprit PRs:
- rust-lang/rust#138384
- rust-lang/rust#127173
- rust-lang/rust#138659,

Resolves #3961, #3976

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 and MIT licenses.
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Apr 2, 2025
…bol, r=wesleywiser,jieyouxu

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

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
tgross35 added a commit to tgross35/rust that referenced this pull request Jun 3, 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#127173
[2]: rust-lang/compiler-builtins#802
tautschnig pushed a commit to model-checking/verify-rust-std that referenced this pull request Jun 17, 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#127173
[2]: rust-lang/compiler-builtins#802
github-merge-queue bot pushed a commit to bazelbuild/rules_rust that referenced this pull request Jul 8, 2025
This adds a version of the allocator support libraries implemented in
rust, to account for the new internal allocator symbol mangling applied
by rustc (rust-lang/rust#127173).

This mechanism relies on unstable language features and requires a
nightly rustc from 2025-04-05 or later. It is designed to be compatible
with:
* all allocator flavors (the default and global)
* all flavors of `no_std`
* both situations for when a `rust_library` is used as a dependency of a
`cc_binary` and when the `experimental_use_cc_common_link` setting is
used.

Rustc generates references to internal allocator symbols when building
rust libraries. At link time, rustc generates
the definitions of these symbols. When rustc is not used as the final
linker, we need to generate the definitions
ourselves. This happens for example when a `rust_library` is used as a
dependency of a `rust_binary`, or when the
`experimental_use_cc_common_link` setting is used.

For older versions of rustc, the allocator symbol definitions can be
provided via the `rust_toolchain`'s
`allocator_library` or `global_allocator_library` attributes, with
sample targets like
`//ffi/cc/allocator_library` and `//ffi/cc/global_allocator_library`.

Recent versions of rustc started mangling these allocator symbols
(rust-lang/rust#127173). The
mangling uses a scheme that is specific to the exact version of the
compiler. This makes the cc allocator library
definitions ineffective. To work around this, we provide rust versions
of the symbol definitions annotated with an
unstable language attribute that instructs rustc to mangle them
consistently. Because of the usage of unstable language features, this
is only compatible with nightly versions of the compiler.

Since the new symbol definitions are written in rust, we cannot just
attach them as attributes on the `rust_toolchain`
as the old cc versions, as that would create a build graph cycle: 
1. any `rust_library` depends on a `rust_toolchain`,
2. the `rust_toolchain` depends on the allocator library, 
3. so the allocator library cannot just be a `rust_library` directly.

The bootstrapping cycle can be avoided by defining a separate internal
"initial" rust toolchain specifically for
building the rust allocator libraries, and use a transition to attach
the generated allocator libraries to the "main" rust
toolchain. But that duplicates the whole subgraph of the build around
the rust toolchains, repository and supporting
tools used for them.

Instead, we define a new custom `rust_allocator_libraries` rule, which
builds the new rust allocator libraries and exposes them via a new
`AllocatorLibrariesInfo` provider. Since we cannot attach such a target
as an attribute to the `rust_toolchain`, we attach it as a new
`allocator_libraries` common attribute to the rust rules. We ported the
cc versions into the new rust implementations of the (default) allocator
and global allocator flavors in `//ffi/rs/allocator_library` and
`//ffi/rs/global_allocator_library`.

The rust standard libraries themselves have references to the allocator
libraries. For correct linking order of the standard libraries, we need
to establish the new rust allocator libraries as link-time dependencies
of the standard libraries. The specific set of linker inputs depends on
the no_std flavor and the choice of (default) allocator vs. global
allocator. We establish the linking dependencies by extending the
`rust_toolchain` `_make_libstd_and_allocator_ccinfo` feature to let us
inject a custom allocator library as a standard library dependency and
using that in the `rust_allocator_libraries` implementation to generate
the final standard libraries linker graph.

The new functionality is opt-in and gated via:
1. the new setting
`//rust/settings:experimental_use_allocator_libraries_with_mangled_symbols`,
which supplies the default value for the repository, and
2. the new `rust_toolchain` attribute
`experimental_use_allocator_libraries_with_mangled_symbols`, which lets
the user override the choice of using the new-style rust-based allocator
symbols, or the old-style cc-based ones for a specific toolchain.

For testing, I created variants for the cc_common.link / no_std /
global_allocator test matrix. I found it useful to be able to declare
`target_settings` via the `rust.repository_set` rule, similarly to the
`target_compatible_with`. This lets us include additional toolchains in
the same repository that are distinguished by the setting, like in:
https://github.com/bazelbuild/rules_rust/pull/3403/files#diff-164dd740b73baa94f9a84ed0d4a7ec29f8adeb34ff8430492747e2443f7f39d3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-run-make Area: port run-make Makefiles to rmake.rs merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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
Development

Successfully merging this pull request may close these issues.