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

rustc_target: Remove pre_link_args_crt #72782

Merged
merged 1 commit into from
Jun 4, 2020
Merged

Conversation

petrochenkov
Copy link
Contributor

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

After #71769 this target option wasn't used anywhere except for VxWorks, and I suspect that for VxWorks its use may be redundant as well.

@rust-highfive
Copy link
Collaborator

r? @varkor

(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 30, 2020
{
// FIXME: What this option does? Is it necessary given that we are already passing
// `-static` and `-nodefaultlibs`? VxWorks GNU toolchain has no public documentation.
self.cmd.arg("--static-crt");
Copy link
Contributor Author

@petrochenkov petrochenkov May 30, 2020

Choose a reason for hiding this comment

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

@BaoshanPang @bpangWR @n-salim @UmeshKalappa, could you comment on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

For VxWorks, we use "--static-crt" to tell the compiler driver that we want to use static C libraries no matter which kind of rust libraries will be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BaoshanPang
That's why I was mentioning -static and -nodefaultlibs, which rustc passes to gcc-like linkers, in case of VxWorks as well.
How does wr-c++ interpret those options?

If it's gcc-compatible, I'd expect -nodefaultlibs to remove libc from linking altogether and -static to provide the desired "link everything statically" semantic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BaoshanPang
One more question, since you added VxWorks support to https://github.com/rust-lang/libc as well.
Do I understand correctly that on VxWorks libc is linked into Rust programs due to this fallback clause?
https://github.com/rust-lang/libc/blob/master/src/unix/mod.rs#L351-L355

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait a second, was --static-crt introduced to the wr-c++ driver for rustc specifically by any chance?
To workaround the default behavior of #[link(name = "c")] linking libc dynamically?

Copy link
Contributor

@BaoshanPang BaoshanPang Jun 1, 2020

Choose a reason for hiding this comment

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

Wait a second, was --static-crt introduced to the wr-c++ driver for rustc specifically by any chance?
To workaround the default behavior of #[link(name = "c")] linking libc dynamically?

Yes, it was introduced for rustc. but it was not only for libc, it is also for only libraries used in VxWorks environement.

Copy link
Contributor

Choose a reason for hiding this comment

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

@BaoshanPang
One more question, since you added VxWorks support to https://github.com/rust-lang/libc as well.
Do I understand correctly that on VxWorks libc is linked into Rust programs due to this fallback clause?
https://github.com/rust-lang/libc/blob/master/src/unix/mod.rs#L351-L355

No, I was not aware of this part when I added the code for rust libc.

Copy link
Contributor

Choose a reason for hiding this comment

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

-static to provide the desired "link everything statically" semantic.

One of the combination we want to support is shared rust library with static VxWorks C library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BaoshanPang
I see, thanks for the information!
I'll keep the current logic as is then, since I cannot test the changes myself.

However, the recommended way to link to libc, libpthread and similar system libraries is still through #[link] attributes in https://github.com/rust-lang/libc, with cfg(target_feature = "crt-static") being used for switching between dynamic and static linking.
See https://github.com/rust-lang/libc/blob/master/src/unix/mod.rs#L295-L357 for examples from other targets.

Copy link
Contributor

Choose a reason for hiding this comment

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

@BaoshanPang
I see, thanks for the information!
I'll keep the current logic as is then, since I cannot test the changes myself.

However, the recommended way to link to libc, libpthread and similar system libraries is still through #[link] attributes in https://github.com/rust-lang/libc, with cfg(target_feature = "crt-static") being used for switching between dynamic and static linking.
See https://github.com/rust-lang/libc/blob/master/src/unix/mod.rs#L295-L357 for examples from other targets.

Thanks for the information, We may use this solution for the next release.

@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 Jun 2, 2020
@petrochenkov
Copy link
Contributor Author

Updated.

@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 Jun 3, 2020
@varkor
Copy link
Member

varkor commented Jun 3, 2020

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 3, 2020

📌 Commit b628358 has been approved by varkor

@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 Jun 3, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2020
Rollup of 6 pull requests

Successful merges:

 - rust-lang#72718 (Add regression test for rust-lang#72554)
 - rust-lang#72782 (rustc_target: Remove `pre_link_args_crt`)
 - rust-lang#72923 (Improve E0433, so that it suggests missing imports)
 - rust-lang#72950 (fix `AdtDef` docs)
 - rust-lang#72951 (Add Camelid per request)
 - rust-lang#72964 (Bump libc dependency to latest version (0.2.71))

Failed merges:

r? @ghost
@bors bors merged commit adc321a into rust-lang:master Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants