RUST_PROGRAMS uses wrong linker in cross builds

RESOLVED FIXED in Firefox 55

Status

Firefox Build System
General
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: rillian, Assigned: froydnj)

Tracking

unspecified
mozilla55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(4 attachments)

Building a stand-alone executable with RUST_PROGRAMS doesn't work with fennec:
```
/home/giles/firefox/obj-arm-linux-androideabi/hello/./armv7-linux-androideabi/release/deps/hello-3a9706a50eb8db9a.0.o: error adding symbols: File in wrong format
collect2: error: ld returned 1 exit status
```
Created attachment 8825146 [details]
Full error message
(Reporter)

Updated

a year ago
Blocks: 1135640
(Assignee)

Updated

a year ago
Attachment #8825146 - Attachment mime type: text/x-log → text/plain
(Assignee)

Comment 2

a year ago
Hm, this looks like Cargo is invoking the wrong compiler...not that we've told it any differently.

Looking at http://doc.crates.io/environment-variables.html , though, I don't see any way to tell Cargo to use a different compiler for the linking step, or any way to pass in extra flags (e.g. for the sysroot).  http://doc.crates.io/config.html says that you can at least define the linker differently for each target, which is nice, but that would require the linker always being in a fixed place relative to Cargo.toml, which doesn't work for our builds.  There also doesn't appear to be a way to pass linker-specific flags from the config file, AFAICS.

Alex, am I missing something in the above description?  We would need to add some features to Cargo for cross-building binaries, correct?
Flags: needinfo?(acrichton)
(Assignee)

Comment 3

a year ago
(Or rather, cross-building binaries in Gecko's, ah, unique use-case.)
Oh you should be able to set the linker by setting an environment variable like:

    CARGO_TARGET_I686_UNKNOWN_LINUX_GNU_LINKER=gcc

or any other executable/target like here: https://github.com/rust-lang/cargo/blob/46bba437205f69c40b2376036aee60ede4fe52bd/src/ci/docker/cross/Dockerfile#L4-L18

You can also configure it via .cargo/config through:

    [target.i686-unknown-linux-gnu]
    linker = 'gcc'

Either way should work.

I also believe you should be able to pass a file that's in PATH or a relative path to a different location, either should suffice. Would that work for this use case?
Flags: needinfo?(acrichton)
(Assignee)

Comment 5

a year ago
(In reply to Alex Crichton [:acrichto] from comment #4)
> I also believe you should be able to pass a file that's in PATH or a
> relative path to a different location, either should suffice. Would that
> work for this use case?

Thanks, that is helpful.  Neither of those methods directly provide for passing additional command-line flags to the linker, correct?  You'd have to point them at wrapper script that added the correct options?
Flags: needinfo?(acrichton)
Correct yeah, a wrapper script will be needed to provide more options. You may also be able to get away with `RUSTFLAGS` to pass `-C link-args=...`, but that may have mixed mileage depending on what you want to do.
Flags: needinfo?(acrichton)
I know I've griped about this before, but I wish Cargo had more flexible support here. It's kind of a pain to have to specify a wrapper script instead of being able to specify linker+linker args.
(Assignee)

Comment 8

a year ago
I think I have a fix for this.
Assignee: nobody → nfroyd
(Assignee)

Comment 9

a year ago
Created attachment 8861169 [details] [diff] [review]
part 1 - define a RUST_TARGET_UPPERCASE config variable

We need this for forming various Cargo environment variables.
Attachment #8861169 - Flags: review?(giles)
(Assignee)

Comment 10

a year ago
Created attachment 8861170 [details] [diff] [review]
part 2 - turn CARGO_BUILD into a callable function

This change paves the way for injecting target- or host-specific
environment variables for a particular `cargo build` invocation.  Doing
this is not strictly necessary: all of our current `cargo build`
invocations use mostly target-specific environment
variables (e.g. PKG_CONFIG_ALLOW_CROSS).  But separating things out
makes the code notationally cleaner and also avoids weirdness when
target==host.
Attachment #8861170 - Flags: review?(giles)
(Assignee)

Comment 11

a year ago
Created attachment 8861171 [details] [diff] [review]
part 3 - use an alternate linker for Cargo invocations

For linking static libraries, rustc will use whatever `cc` it finds (or
the equivalent on Windows).  For our purposes, however, `cc` is not what
we use to link and we may have additional options we would like to pass
to the linker.

To do this, we need to tell Cargo about our alternate linker (currently
only used for target compilations, on the theory that the host compiler
rustc finds is probably good enough) and we also need to pass our linker
options into the process.  We do this with environment variables, which
is not a great solution, but works surprisingly well.

This alternate linker is disabled for ASan builds due to peculiar
crashes when running Rust build scripts and for Windows, because we
don't do any interesting cross-compiling there.

This sequence of patches enables me to define a small RUST_PROGRAMS entry and
have the executables built on an Android cross build.
Attachment #8861171 - Flags: review?(giles)
(Reporter)

Updated

a year ago
Attachment #8861169 - Flags: review?(giles) → review+
Comment on attachment 8861170 [details] [diff] [review]
part 2 - turn CARGO_BUILD into a callable function

Review of attachment 8861170 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the documentation and good commit messages!

::: config/rules.mk
@@ +977,2 @@
>  	$(CARGO) build $(cargo_build_flags)
> +endef

I would have tried a target-specific CARGO_ENV (or CARGO_EXTRA_ENV?) here, just so the invocation line is shorted. But this works too, and maybe makes evaluation time more obvious?
Attachment #8861170 - Flags: review?(giles) → review+
Comment on attachment 8861171 [details] [diff] [review]
part 3 - use an alternate linker for Cargo invocations

Review of attachment 8861171 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but please consider moving `s/-/_/g` into python.

::: config/rules.mk
@@ +976,5 @@
>  	$(1) \
>  	$(CARGO) build $(cargo_build_flags)
>  endef
>  
> +cargo_target_for_env_var := $(subst -,_,$(RUST_TARGET_UPPERCASE))

Might as well do this part in rust.configure if this is the only use. It's a similar transformation, to make it look like an env variable name instead of a value.
Attachment #8861171 - Flags: review?(giles) → review+
(Assignee)

Comment 14

a year ago
(In reply to Ralph Giles (:rillian) | needinfo me from comment #12)
> ::: config/rules.mk
> @@ +977,2 @@
> >  	$(CARGO) build $(cargo_build_flags)
> > +endef
> 
> I would have tried a target-specific CARGO_ENV (or CARGO_EXTRA_ENV?) here,
> just so the invocation line is shorted. But this works too, and maybe makes
> evaluation time more obvious?

Yeah, I think evaluation time here is more obvious, and if we want to try and draw better lines about host-specific vs. target-specific Cargo environment variables, I think passing in the extra environment variables this way is more flexible.

(In reply to Ralph Giles (:rillian) | needinfo me from comment #13)
> ::: config/rules.mk
> @@ +976,5 @@
> >  	$(1) \
> >  	$(CARGO) build $(cargo_build_flags)
> >  endef
> >  
> > +cargo_target_for_env_var := $(subst -,_,$(RUST_TARGET_UPPERCASE))
> 
> Might as well do this part in rust.configure if this is the only use. It's a
> similar transformation, to make it look like an env variable name instead of
> a value.

Good point, and less of a footgun for other people who might use this variable later, I suppose.  Do you have preferences on the name (RUST_TARGET_ENV_NAME?) or is that just pointless bikeshedding?
Flags: needinfo?(giles)
It's pointless bikeshedding. :) RUST_TARGET_ENV_NAME is fine. I'd also be fine with just RUST_TARGET. I can't think of anything that's really distinguishing and not RUST_TARGET_TRIPLE_CONVERTED_TO_A_SHELL_VARIABLE_NAME, so we're relying on some context anyway.
Flags: needinfo?(giles)

Comment 16

a year ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acd67da3b84a
part 1 - define a RUST_TARGET_ENV_NAME config variable; r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbbceb56e0d9
part 2 - turn CARGO_BUILD into a callable function; r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc34b2a31c4b
part 3 - use an alternate linker for Cargo invocations; r=rillian
https://hg.mozilla.org/mozilla-central/rev/acd67da3b84a
https://hg.mozilla.org/mozilla-central/rev/fbbceb56e0d9
https://hg.mozilla.org/mozilla-central/rev/fc34b2a31c4b
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

a year ago
Depends on: 1362516

Updated

5 months ago
Depends on: 1418598

Updated

2 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.