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

Force linker to link in exported symbols #95363

Closed
wants to merge 0 commits into from

Conversation

carbotaniuman
Copy link
Contributor

@carbotaniuman carbotaniuman commented Mar 27, 2022

The more surgical variant of #93791, which uses linker args -u on Unix and /INCLUDE on Windows to forcefully include exported symbols.

This allows statics like this to work (and get embedded in the final executable):

mod foo {
    #[used]
    static FOO: [u32; 10] = [1; 10];
}

@nbdd0121 I've copied your test wholesale, feel free to review. I'm not sure if this handles all the edge cases, and I'd love a test for crate-type=bin.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 27, 2022
@rust-highfive
Copy link
Collaborator

r? @oli-obk

(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 Mar 27, 2022
@rust-log-analyzer

This comment has been minimized.

@nbdd0121
Copy link
Contributor

-u have a very unfortunate effect that it will inhibit GC sections. For example, #[no_mangle] functions should be allowed to GCed if they are not reached from main in binaries. I have experimented with -u before trying while-archive and abandoned it for this reason.

I have been experimenting a more advanced approach for ELF targets using a customly generated object file and it seems working well; though I didn't make much progress due to lack of time recently.

@petrochenkov petrochenkov self-assigned this Mar 27, 2022
@petrochenkov
Copy link
Contributor

There's kind of a race condition between our work, because this is almost exactly what I planned to implement today-tomorrow to address the question (2) from #93901 (comment).

Except that I primarily care about dylibs (as described in the linked comment), so the issue with garbage-collecting #[no_mangle] stuff is irrelevant, nothing exported should be garbage-collected in that case.

using a customly generated object file

Generating a "root" object file with artificial references to all exported symbols may be the most general solution, because it looks like it should work for both dylib and executable cases (and would allow GCing no_mangles while keeping useds).
It is unfortunate if just passing -us is not equivalent to generating such references.

@carbotaniuman
Copy link
Contributor Author

carbotaniuman commented Mar 27, 2022

Hmm I see this issue: #84161 at the same time. Given -u inhibits garbage collecting symbols (does it do so on both MSVX and GCC/LD?).

Is there anything wrong with mapping #[used(linker)] to -u? That seems like it has the perfect semantics. I'm also wondering if we could pass -u only for statics for this case, which seems like the best of both worlds.

@carbotaniuman
Copy link
Contributor Author

I'm going to implement these semantics right now, but I'm sure they'll be wrong somewhere.

  1. All exposed exported symbols will be forcefully linked in. If this is a cdylib or a dylib, then functions are included, if not, then just statics, unless the RFC 2841 option is specified.
  2. All #[used(linker)] statics will be linked in. We could also have another used arg to say I want this to be linked in but possibly GCed.
  3. Everything else will be up to the linker.

I'm not actually going to touch the RFC 2841 part or dylibs, but I think those semantics should cover everything.

@carbotaniuman
Copy link
Contributor Author

Ok implemented the first part of the above. There's a lot of todos and fixmes thrown around here as there needs to be a refactor or two, but this uses implicit linker scripts (for GCC) and command files (for LINK) to forcibly link stuff. This is implemented for dylibs currently (and dylibs were the reason I had to switch to files and not args), but I've kept the whole-archive stuff in place so idk if it does anything.

@rust-log-analyzer

This comment has been minimized.

@nbdd0121
Copy link
Contributor

All exposed exported symbols will be forcefully linked in. If this is a cdylib or a dylib, then functions are included, if not, then just statics, unless the RFC 2841 option is specified.

This is just heuristics, not a comprehensive fix, isn't it?

Is there anything wrong with mapping #[used(linker)] to -u? That seems like it has the perfect semantics.

Mapping #[used(linker)] to -u is probably okay.

The best approach that I can think of so far is still a custom-generated object file. I think it could be a universal solution, without need of any heuristics or special handling of #[used(linker)], and without introducing extra unwanted semantics. I'll try to find some time next weekend to see if I can at least get a prototype version working.

@carbotaniuman
Copy link
Contributor Author

I feel like it's a comprehensive fix, but I'm not sure. I can't come up with a counterexample right now other than #[used(linker)]. I'm not sure how a custom object file could avoid special casing though, you'd still need to grab a list of symbols from somewhere?

@oli-obk oli-obk removed their assignment Mar 28, 2022
@nbdd0121
Copy link
Contributor

I feel like it's a comprehensive fix, but I'm not sure. I can't come up with a counterexample right now other than #[used(linker)].

For binaries, this would still cause some statics (#[no_mangle] or #[used(compiler)] but unreferenced) to be non-GC-able, and functions to be omitted (e.g. those KEEP-ed by linker scripts).

I'm not sure how a custom object file could avoid special casing though, you'd still need to grab a list of symbols from somewhere?

It'll just need to use the same list of symbols that is used in version scripts.

@bjorn3
Copy link
Member

bjorn3 commented Mar 28, 2022

For binaries, this would still cause some statics (#[no_mangle] or #[used(compiler)] but unreferenced) to be non-GC-able, and functions to be omitted (e.g. those KEEP-ed by linker scripts).

For executables this is ignored: https://github.com/rust-lang/rust/pull/95363/files#diff-3f87e69f5e21712681d6fca5b6b70542c10eaf49bf48ff16445ddd17f1ae6dadR666

@carbotaniuman
Copy link
Contributor Author

carbotaniuman commented Mar 28, 2022

For executables this is ignored: https://github.com/rust-lang/rust/pull/95363/files#diff-3f87e69f5e21712681d6fca5b6b70542c10eaf49bf48ff16445ddd17f1ae6dadR666

This wasn't exactly correct before, but it is now. We forcefully link all #[used] (even #[used(compiler)] but that's just because our semantics for that are weird rn) into an executable. We only forcefully link in exports if we're actually exporting things.

functions to be omitted (e.g. those KEEP-ed by linker scripts).

This has nothing to do with custom linker scripts I think, because those happen before any of this? The main use case I'm thinking of is this one and #47384 (comment), wherein symbols that are manually KEEPed aren't included. With these semantics, a #[used] solves that, although I do see an issue where maybe too much might be included.

@rust-log-analyzer

This comment has been minimized.

@nbdd0121
Copy link
Contributor

This has nothing to do with custom linker scripts I think, because those happen before any of this?

Let me give a concrete example, let's say we have

#[no_mangle]
#[link_section = ".foo"]
fn foo() {
  /* ... */
}

and

extern "C" {
    fn bar();
}

fn main() {
    unsafe { bar() };
}

in different rlibs, and have

.foo : {
    bar = .;
    KEEP(*(.foo));
}

in my linker script, then function foo should be included, but it wouldn't because there are no references to foo (in executable case).

@bjorn3
Copy link
Member

bjorn3 commented Mar 29, 2022

Would #85673 fix your usecase?

@nbdd0121
Copy link
Contributor

Well, if you force export symbols in executables then unused #[no_mangle] functions get exported too.

@carbotaniuman
Copy link
Contributor Author

Can I get a perf run now, or do I have to figure out this reproducible build error first?

@kaspar030
Copy link

GC'ing #[no_mangle] symbols is essential for embedded code (where every byte counts).

@petrochenkov
Copy link
Contributor

Can I get a perf run now, or do I have to figure out this reproducible build error first?

@bors try @rust-timer queue
(FWIW, I didn't have a chance to look at the updated changes yet.)

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 29, 2022
@bors
Copy link
Contributor

bors commented Mar 29, 2022

⌛ Trying commit dcb04599563adea275423429e713281ce536a71a with merge 56920da24004deccd76240cb6a49806fd8cf0369...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Apr 1, 2022

⌛ Trying commit 9a5e1ff2eead8ae7c03ce12420421fe7ff8197dc with merge 4ffb3425b6816cb1e2e5a9cb6ccd63c9ab15ff33...

@bors
Copy link
Contributor

bors commented Apr 1, 2022

☀️ Try build successful - checks-actions
Build commit: 4ffb3425b6816cb1e2e5a9cb6ccd63c9ab15ff33 (4ffb3425b6816cb1e2e5a9cb6ccd63c9ab15ff33)

@rust-timer
Copy link
Collaborator

Queued 4ffb3425b6816cb1e2e5a9cb6ccd63c9ab15ff33 with parent 99da9ae, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4ffb3425b6816cb1e2e5a9cb6ccd63c9ab15ff33): comparison url.

Summary:

  • Primary benchmarks: no relevant changes found. 1 results were found to be statistically significant but too small to be relevant.
  • Secondary benchmarks: no relevant changes found. 4 results were found to be statistically significant but too small to be relevant.
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 1 2 0 2 1
mean2 0.5% 0.3% N/A -0.3% 0.5%
max 0.5% 0.3% N/A -0.3% 0.5%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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 led to changes in compiler perf.

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

Footnotes

  1. number of relevant changes

  2. the arithmetic mean of the percent change

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 1, 2022
@petrochenkov petrochenkov removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 2, 2022
@petrochenkov
Copy link
Contributor

Performance is ok, but the run-make-fulldeps/reproducible-build test is still failing though.

@petrochenkov
Copy link
Contributor

I'll get to reviewing this next weekend most likely, I need to read #47384 and look at some code in rustc generating export lists, to understand what different cases exists.

I'm also still interested in landing some simple solution addressing only dylibs sooner to unblock stabilization of the bundle modifier.

@carbotaniuman
Copy link
Contributor Author

carbotaniuman commented Apr 2, 2022

Yeah I'm not sure what to do for run-make-fulldeps/reproducible-build.

The reason is I have to inject a synthetic file that didn't previously exist: forcelink.ld (ignore the -1-62 below, those were me trying to fix CI). So I grabbed the same tempdir that was used for export_symbols to place the file in, and that of courses uses a tempdir that changes every run.

The actual contents of the file are identical however as the function that creates it is deterministic, so I don't think the output is still reproducible. I don't know what this is testing, nor can I reasonably put forcelink.fd in a reproducible directory without breaking things. I also tested by building a cdylib with master on this exact test, and it fails in the exact same way.

-/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make-fulldeps/reproducible-build/reproducible-build/rustc1YSzXc/forcelink-1-62.ld: 2650171599958706936
+/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make-fulldeps/reproducible-build/reproducible-build/rustcjv1Ryr/forcelink-1-62.ld: 2650171599958706936

@petrochenkov
Copy link
Contributor

Marking as blocked on #95604 for now, with an intent to close when/if it lands.

@petrochenkov petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 10, 2022
Generate synthetic object file to ensure all exported and used symbols participate in the linking

Fix rust-lang#50007 and rust-lang#47384

This is the synthetic object file approach that I described in rust-lang#95363 (comment), allowing all exported and used symbols to be linked while still allowing them to be GCed.

Related rust-lang#93791, rust-lang#95363

r? `@petrochenkov`
cc `@carbotaniuman`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 11, 2022
Generate synthetic object file to ensure all exported and used symbols participate in the linking

Fix rust-lang#50007 and rust-lang#47384

This is the synthetic object file approach that I described in rust-lang#95363 (comment), allowing all exported and used symbols to be linked while still allowing them to be GCed.

Related rust-lang#93791, rust-lang#95363

r? ``@petrochenkov``
cc ``@carbotaniuman``
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 11, 2022
Generate synthetic object file to ensure all exported and used symbols participate in the linking

Fix rust-lang#50007 and rust-lang#47384

This is the synthetic object file approach that I described in rust-lang#95363 (comment), allowing all exported and used symbols to be linked while still allowing them to be GCed.

Related rust-lang#93791, rust-lang#95363

r? ```@petrochenkov```
cc ```@carbotaniuman```
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 17, 2022
Generate synthetic object file to ensure all exported and used symbols participate in the linking

Fix rust-lang#50007 and rust-lang#47384

This is the synthetic object file approach that I described in rust-lang#95363 (comment), allowing all exported and used symbols to be linked while still allowing them to be GCed.

Related rust-lang#93791, rust-lang#95363

r? `@petrochenkov`
cc `@carbotaniuman`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 18, 2022
Generate synthetic object file to ensure all exported and used symbols participate in the linking

Fix rust-lang#50007 and rust-lang#47384

This is the synthetic object file approach that I described in rust-lang#95363 (comment), allowing all exported and used symbols to be linked while still allowing them to be GCed.

Related rust-lang#93791, rust-lang#95363

r? `@petrochenkov`
cc `@carbotaniuman`
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 25, 2022
Generate synthetic object file to ensure all exported and used symbols participate in the linking

Fix rust-lang#50007 and rust-lang#47384

This is the synthetic object file approach that I described in rust-lang#95363 (comment), allowing all exported and used symbols to be linked while still allowing them to be GCed.

Related rust-lang#93791, rust-lang#95363

r? `@petrochenkov`
cc `@carbotaniuman`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work. 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.