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

linker: More systematic handling of CRT objects #71769

Merged
merged 1 commit into from
May 20, 2020

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented May 1, 2020

Document which kinds of crt0.o-like objects we link and in which cases, discovering bugs in process.
src/librustc_target/spec/crt_objects.rs is the place to start reading from.

This PR also automatically contains half of the -static-pie support (#70740), because that's one of the six cases that we need to consider when linking CRT objects.

This is a breaking change for custom target specifications that specify CRT objects.

Closes #30868

@rust-highfive
Copy link
Collaborator

r? @eddyb

(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 May 1, 2020
let mut user_defined_link_args = sess.opts.cg.link_args.iter().chain(attr_link_args);
if sess.relocation_model() == RelocModel::Pic
&& !sess.crt_static(Some(crate_type))
&& !user_defined_link_args.any(|x| x == "-static")
Copy link
Contributor Author

@petrochenkov petrochenkov May 1, 2020

Choose a reason for hiding this comment

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

The only case of rustc interpreting user-provided raw linker arguments is removed here.
First, -static wins over -pie so we could still pass -pie if the user passed -static with the same effect.
Second, it was a hack, this static guessing wasn't done consistently in other places, equivalent cases like -Wl,-static weren't recognized, and we shouldn't be interpreting raw linker args in general.

(CrtObjectKind::DynamicPicExe, &["Scrt1.o"]),
(CrtObjectKind::StaticNoPicExe, &["Scrt1.o"]),
(CrtObjectKind::StaticPicExe, &["Scrt1.o"]),
]),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea which libc flavor Fuchsia uses, just kept the status quo behavior.

@bors
Copy link
Contributor

bors commented May 3, 2020

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

@petrochenkov
Copy link
Contributor Author

petrochenkov commented May 10, 2020

@smaeul @shizmob @mixi (people who participated in #40113 and related PRs), which of you I can ping on PRs/issues like this one?

I have a question.
From the paper trail (#71586 (comment)) it looks like it's strongly preferable to use a musl-aware system gcc as a linker if it's available, and rely on it for pulling CRT objects. (And system libc.a as well, but it's a separate question.)
At the same time, for backward compatibility in some cases we have to fall back to gcc with -nostdlib and manually linking CRT objects distributed with rustc.

What would be a good and safe approximation for "musl-aware system gcc is available" that rustc could use to apply the fallback only when it's really necessary?

  • host == target (both musl) seems like a good heuristic
  • anything else?

@smaeul
Copy link
Contributor

smaeul commented May 11, 2020

Checking to see if $GCC -dumpmachine contains "-musl" may be somewhat less fragile, as it would handle musl-to-musl cross compiling, and would handle the user overriding the linker (say, to a real musl cross toolchain when cross compiling glibc-to-musl).

@petrochenkov
Copy link
Contributor Author

Checking to see if $GCC -dumpmachine contains "-musl" may be somewhat less fragile

Clarification: without expensive environment probing like running processes (but checking files is ok).

Ideally such check is performed by a build system, and the result is cached, and then passed to the compiler.
Cargo cannot do such things unfortunately, but it would be useful for other things as well (e.g. detecting support of -static-pie and -no-pie).

and would handle the user overriding the linker

Explicitly passed -C linker may be a good heuristic actually.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented May 11, 2020

So, I've measured the slowdown from the gcc -dumpmachine probing on a subset of UI tests in rustc (./x.py test --bless --stage 1 src/test/ui --test-args f, that's at least 3206 invocations of rustc, some don't do linking though).
The measurements were done on Windows, which is known for its slow process creation, so the slowdown on Linux should be smaller.

No probing (3 runs):

real    1m46.719s
real    1m47.304s
real    1m49.113s

Probing (3 runs):

real    1m50.650s
real    1m51.125s
real    1m50.946s

That's ~3% slower.

Should be pretty acceptable if it only happens in setups in which other heuristics fail.
I'll try to implement it in a separate PR.

In this PR some FIXMEs and comments need to be updated before it's ready.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 12, 2020
@petrochenkov
Copy link
Contributor Author

r? @cuviper
eddyb is nowhere in sight since april

@rust-highfive rust-highfive assigned cuviper and unassigned eddyb May 13, 2020
@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 13, 2020
@cuviper
Copy link
Member

cuviper commented May 20, 2020

I don't have deep knowledge of all of these targets, but it looks reasonable to me.

@bors r+

@bors
Copy link
Contributor

bors commented May 20, 2020

📌 Commit 49eb35c has been approved by cuviper

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 20, 2020
@bors
Copy link
Contributor

bors commented May 20, 2020

⌛ Testing commit 49eb35c with merge 64ad709...

@bors
Copy link
Contributor

bors commented May 20, 2020

☀️ Test successful - checks-azure
Approved by: cuviper
Pushing 64ad709 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 20, 2020
@bors bors merged commit 64ad709 into rust-lang:master May 20, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 4, 2020
rustc_target: Remove `pre_link_args_crt`

To regain some more control over the definition of `+crt-static` (rust-lang#71586).

After rust-lang#71769 this target option wasn't used anywhere except for VxWorks, and I suspect that for VxWorks its use may be redundant as well.
match kind {
LinkOutputKind::DynamicPicExe if !pic_exe_supported => LinkOutputKind::DynamicNoPicExe,
LinkOutputKind::StaticPicExe if !static_pic_exe_supported => LinkOutputKind::StaticNoPicExe,
LinkOutputKind::StaticDylib if !static_dylib_supported => LinkOutputKind::DynamicDylib,
Copy link

@tchebb tchebb Jul 30, 2020

Choose a reason for hiding this comment

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

Am I missing something, or is this logic inverted from what it should be? If crt_static() returns true, aren't we only allowed to create a DynamicDylib when crt_static_allows_dylibs is also true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case is here for consistency, but we should never hit it right now because if invalid_output_for_target is true then we skip the crate type and never get to linking.
But if we didn't skip it (like we don't skip PIC executables when +crt-static is enabled but not supported), then we would fall back from StaticDylib to DynamicDylib.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom target files don't support (pre|post)_link_objects_(dll|exe)
8 participants