-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Add Linker Support For L4Re #54425
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
Add Linker Support For L4Re #54425
Conversation
r? @davidtwco (rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
This looks pretty good to me, I've got a question and a few minor nits.
L4Bender::split_cmd_args(&mut cmd, &l4_ld_opts); | ||
} | ||
|
||
L4Bender { cmd: cmd, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could you format this so the indentation is more consistent with other parts of the code.
L4Bender {
cmd,
sess,
hinted_static: false,
}
or
L4Bender { cmd, sess, hinted_static: false, }
@@ -12,17 +12,6 @@ use spec::{LinkArgs, LinkerFlavor, PanicStrategy, TargetOptions}; | |||
use std::default::Default; | |||
//use std::process::Command; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could you remove this commented import?
|
||
if let Ok(l4_ld_opts) = env::var("L4_LD_OPTIONS") { | ||
L4Bender::split_cmd_args(&mut cmd, &l4_ld_opts); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason that the flags -C link-arg
or -C link-args
to rustc
(in combination with RUSTFLAGS
for use with cargo build
, etc.) can't be used instead of introducing new environment variables and a function to split them?
Thanks for your comments!
+ if let Ok(l4bender_args) = env::var("L4_BENDER_ARGS") {
+ L4Bender::split_cmd_args(&mut cmd, &l4bender_args);
+ }
+
+ cmd.arg("--"); // separate direct l4-bender args from linker args
+
+ if let Ok(l4_ld_opts) = env::var("L4_LD_OPTIONS") {
+ L4Bender::split_cmd_args(&mut cmd, &l4_ld_opts);
+ }
Is there a reason that the flags `-C link-arg` or `-C link-args` to `rustc` (in
combination with `RUSTFLAGS` for use with `cargo build`, etc.) can't be used
instead of introducing new environment variables and a function to split them?
I need to position the arguments, so that a call looks roughly like this:
l4-bender -SPECIAL_L4BENDER_ARG1 … -- -MOSTLY_NORMAL_LD_COMMAND1 …
I don't see how I could pass a few options before the `--` and a few after;
maybe this works and I overlooked something?
|
I'm not sure how that works either. Otherwise, this LGTM but I'll ping @alexcrichton who is more familiar with this part of the code. |
These options can be passed using `-C link-arg`.
@davidtwco You were right. |
@humenda I'm not sure what the preferred approach here is. I've found an old post by @alexcrichton that suggests (to me) that we'd prefer to not support new environment variables or flags for this sort of thing. I'm not too sure though. Hopefully @alexcrichton would be able to elaborate more on a preferred approach here. That all said, I've found a handful of older commits that (to my untrained eye) seem like they might be relevant or useful and could give you a way to do this without a new flag or env variable:
|
Ah yeah sorry I've been slow to get here, the PR looks mostly good! I agree that we should avoid new env vars if possible (additionally avoiding trying to shell parse flags if we can). It looks like it should be possible to avoid here though, right? |
- I think there is an unstable `-Z pre-link-arg` flag.
The commit message says: `and can be used to pass extra arguments at the *beginning* of the linker invocation, before the Rust object files are passed.`
Looking at the code, it appears to me that the pre-link-args are indeed not the
first arguments to the linker invocation, for instance the CTRN-files are
passed before. Though this is not the case for l4re-uclibc, it seems very
fragile to bet on pre-link-args to be the first. The definition of it really
would need to be "before any linker arguments are passed."
- You could also try using `-C linker="l4-bender --l4-specific-flag" -Z linker-flavour=l4-bender`
I would like to, but the arguments look like this:
[l4-bender] --spec=path -Dlinker="ld -m …" -Dfoo=bar
There needs to be a way to mark argument boundaries, `-Dlinker="…"` needs to be
*one* argument and spaces in there would ideally be preserved.
The latest commit removed one of the variables, for the second I'm still not
sure how to properly deal with it.
|
Sorry for being slow to reply, but if linker arguments must be at the front, can't a shell script be used to make sure they're placed at the front? Otherwise the "pre" in "pre link args" I think means they'll always be sufficiently at the front |
Ping from triage @humenda: Some additional clarifications have been requested for your PR. |
Passing pre-link-args and link-args seems not possible with Cargo command-line flags ATM. There is |
L4Re uses a linker script called l4-bender used within the L4Re build system to cross-compile (/link) to L4Re. This PR adds l4-bender as another linker variant to Rustc.