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

create -Z force-allocator-shim codegen option #94389

Closed

Conversation

mhammerly
Copy link
Contributor

@mhammerly mhammerly commented Feb 26, 2022

Related to issue #73632 "Formal support for linking rlibs using a non-Rust linker" - a solution to that issue will involve something like this.

A related PR which has been open since July: #86844 "Support #[global_allocator] without the allocator shim" by @bjorn3. It'd suit my needs, but I had mostly finished this before I saw #86844 and thought putting another option up in a PR might spur the discussion forward.

Rust's behavior of bundling dependencies and the standard library into staticlib crates makes it difficult to gradually integrate Rust into large pre-existing C++ codebases. Some projects work around that by providing unbundled rlibs to their C++ linker, but doing so leaves them without an allocator shim. Issue #73632 discusses how to officially support this use case but a solution will probably still involve something like this PR: a way to generate an allocator shim for more crate types.

This PR

If -Z force-allocator-shim is provided to rustc then rustc will:

  • not skip choosing an allocator_kind in rustc_metadata/src/creader.rs
    • as a result, allocator shim codegen won't be skipped in rustc_codegen_ssa/src/base.rs
  • not skip adding the allocator shim CGU to the output archive

Notable TODOs:

  • Test coverage / documentation - is there more to do? A way not to use run-make?
  • Get feedback on the approach / what handling is needed for other crate types

Pointers and feedback greatly appreciated! I'm new to Rust and certainly to rustc.

Compared to PR #86844

PR #86844 would serve this use case by making an allocator shim unnecessary if #[global_allocator] is declared. #[global_allocator] will produce __rust_alloc and friends directly, and only if it isn't present will an allocator shim be generated to divert __rust_alloc and friends to the corresponding __rdl_* functions.

I can't evaluate which approach is nicer. PR #86844 is actually done so it's got a leg up on this PR, but this PR is backend-agnostic and hides the changed behavior behind a command line flag which seem like nice qualities.

There's discussion in issue #73632 about whether --emit=obj or emitting an rlib-like archive is better for that use case. I think #86844 would work with both strategies while this PR only works with an rlib-like archive.

No worries if #86844 is the right way forward - it still solves my problem and the fun I've had so far with this PR is its own reward :)

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 26, 2022
@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 @cjgillot (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 26, 2022
@@ -1033,6 +1033,8 @@ options! {
"emit bitcode in rlibs (default: yes)"),
extra_filename: String = (String::new(), parse_string, [UNTRACKED],
"extra data to put in each output filename"),
force_allocator_shim: Option<bool> = (None, parse_opt_bool, [TRACKED],
"force generation of an allocator shim"),
Copy link
Member

Choose a reason for hiding this comment

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

Please use -Z instead of -C to avoid immediately stabilizing this. This option is only useful when depending on an implementation detail (that rlibs are currently archives) anyway.

Copy link
Contributor Author

@mhammerly mhammerly Feb 26, 2022

Choose a reason for hiding this comment

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

Do you have a code pointer for how to make this a -Z option or gate it behind -Z unstable-features? I wasn't able to find that on my own (it's one of the TODOs in the original comment)

EDIT nevermind, reading options.rs again it's much easier to figure out than the codegen stuff was. updated

@mhammerly mhammerly changed the title create -C force-allocator-shim codegen option create -Z force-allocator-shim codegen option Feb 26, 2022
@cjgillot
Copy link
Contributor

@bjorn3, @pnkfelix as author & reviewer for #86844:

This PR provides an alternative implementation of what was done in #86844. As #86844 has been dormant for quite some time. Is it ready to merge? Are there remaining blockers? Should this PR be preferred as a stepping stone to be reverted when #86844 lands?

You are more qualified than I am to review the implementation. Could one of you take over?

@bjorn3
Copy link
Member

bjorn3 commented Feb 26, 2022

#86844 has been waiting on review and a decision about if this is the right thing to do. I personally favor my PR over this PR, but I won't veto this one if others prefer it.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@pnkfelix
Copy link
Member

pnkfelix commented Mar 1, 2022

#86844 has been waiting on review and a decision about if this is the right thing to do. I personally favor my PR over this PR, but I won't veto this one if others prefer it.

I'll up the priority on looking at both #86844 and #94389 and see if I can figure out which one is "obviously" better. If I cannot figure it out, then I'll figure out to whom to delegate that decision.

Thanks your patience here, @bjorn3

@rustbot claim

@rustbot rustbot assigned pnkfelix and unassigned cjgillot Mar 1, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2022
@pnkfelix
Copy link
Member

I'll up the priority on looking at both #86844 and #94389 and see if I can figure out which one is "obviously" better. If I cannot figure it out, then I'll figure out to whom to delegate that decision.

after reviewing the situation and discussing with @bjorn3 , I think we should focus on pushing PR #86844 through rather than landing this.

So, closing this PR.

Thank you so much for putting up this pull request, @mhammerly ! It really was good to have two concrete options to look at and think about.

@pnkfelix pnkfelix closed this Apr 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.

None yet

8 participants