1329737 - RUST_PROGRAMS uses wrong linker in cross builds

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Ralph Giles (:rillian)]

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
```

Comment 1

[Ralph Giles (:rillian)]

Attachment: Full error message

Comment 2

[Nathan Froyd [:froydnj]]

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?

Comment 3

[Nathan Froyd [:froydnj]]

(Or rather, cross-building binaries in Gecko's, ah, unique use-case.)

Comment 4

[Alex Crichton [:acrichto]]

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?

Comment 5

[Nathan Froyd [:froydnj]]

(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?

Comment 6

[Alex Crichton [:acrichto]]

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.

Comment 7

[(not currently active) Ted Mielczarek]

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.

Comment 8

[Nathan Froyd [:froydnj]]

I think I have a fix for this.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 1 - define a RUST_TARGET_UPPERCASE config variable

We need this for forming various Cargo environment variables.

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: 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.

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: 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.

Comment 12

[Ralph Giles (:rillian)]

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?

Comment 13

[Ralph Giles (:rillian)]

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.

Comment 14

[Nathan Froyd [:froydnj]]

(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?

Comment 15

[Ralph Giles (:rillian)]

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.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/acd67da3b84a
https://hg.mozilla.org/mozilla-central/rev/fbbceb56e0d9
https://hg.mozilla.org/mozilla-central/rev/fc34b2a31c4b