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 invocation for wasm32-unknown-unknown incompatibly changed between 1.66.1 and 1.67.1 #108910

Open
pnkfelix opened this issue Mar 8, 2023 · 8 comments
Labels
A-linkage Area: linking into static, shared libraries and binaries C-bug Category: This is a bug. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Mar 8, 2023

I ran this sequence of commands via rustup to install 1.66.1 and 1.67.1, as well as the target support for wasm32-unknown-unknown for each, and then tried to compile a cdylib for wasm32-unknown-unknown while also overriding the linker.

% rustup update 1.66.1
[...]
% rustup update 1.67.1
[...]
% rustup target add --toolchain 1.66.1 wasm32-unknown-unknown
[...]
% rustup target add --toolchain 1.67.1 wasm32-unknown-unknown
[...]
% echo > /tmp/hello.rs
% rustc +1.66.1 /tmp/hello.rs --crate-type cdylib --target wasm32-unknown-unknown -C linker=clang && echo success
success
% rustc +1.67.1 /tmp/hello.rs --crate-type cdylib --target wasm32-unknown-unknown -C linker=clang
error: linking with `clang` failed: exit status: 1
  |
  = note: "clang" "-Wl,-z" "-Wl,stack-size=1048576" "-Wl,--stack-first" "-Wl,--allow-undefined" "-Wl,--fatal-warnings" "-Wl,--no-demangle" "--target=wasm32-unknown-unknown" "-Wl,--no-entry" "hello.hello.4592d75c-cgu.0.rcgu.o" "hello.395fat3noigyayzs.rcgu.o" "-L" "/home/pnkfelix/.rustup/toolchains/1.67.1-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib" "/home/pnkfelix/.rustup/toolchains/1.67.1-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib/libstd-4cc67ecdd38ec816.rlib" "/home/pnkfelix/.rustup/toolchains/1.67.1-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib/libpanic_abort-d7aed05895fd3acd.rlib" "/home/pnkfelix/.rustup/toolchains/1.67.1-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib/libdlmalloc-d614b689e44911c3.rlib" "/home/pnkfelix/.rustup/toolchains/1.67.1-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib/librustc_demangle-b67bdb405beac3e8.rlib" "/home/pnkfelix/.rustup/toolchains/1.67.1-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib/libstd_detect-d35bb83dee339ae2.rlib" "/home/pnkfelix/.rustup/toolchains/1.67.1-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib/libhashbrown-a91c75972ec9987b.rlib" "/home/pnkfelix/.rustup/toolchains/1.67.1-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib/libminiz_oxide-cebf340576826f4d.rlib" "/home/pnkfelix/.rustup/toolchains/1.67.1-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib/libadler-1dfefb8109e16570.rlib" "/home/pnkfelix/.rustup/toolchains/1.67.1-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib/librustc_std_workspace_alloc-fb56ca5e4fa7e885.rlib" "/home/pnkfelix/.rustup/toolchains/1.67.1-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib/libunwind-7758a353f6e6c303.rlib" "/home/pnkfelix/.rustup/toolchains/1.67.1-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib/libcfg_if-9fa968ba9e6d4f22.rlib" "/home/pnkfelix/.rustup/toolchains/1.67.1-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib/liblibc-6769bef7a67a1976.rlib" "/home/pnkfelix/.rustup/toolchains/1.67.1-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib/liballoc-5ef00dd564498f26.rlib" "/home/pnkfelix/.rustup/toolchains/1.67.1-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib/librustc_std_workspace_core-a025c7699298bef7.rlib" "/home/pnkfelix/.rustup/toolchains/1.67.1-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib/libcore-028eb657858f49ce.rlib" "/home/pnkfelix/.rustup/toolchains/1.67.1-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib/libcompiler_builtins-f9be42f11058bb31.rlib" "-L" "/home/pnkfelix/.rustup/toolchains/1.67.1-x86_64-unknown-linux-gnu/lib/rustlib/wasm32-unknown-unknown/lib" "-o" "hello.wasm" "-Wl,--gc-sections" "-shared" "-nodefaultlibs"
  = note: clang: warning: argument unused during compilation: '-shared' [-Wunused-command-line-argument]
          wasm-ld-14: error: cannot open crt1.o: No such file or directory
          clang: error: linker command failed with exit code 1 (use -v to see invocation)

I expected to see this happen: a command sequence that worked under 1.66.1 to continue working under 1.67.1

Instead, this happened: A linker failure that is somewhat inscrutable to a layperson.


Note: There is a relatively easy workaround available. Under 1.66.1 when passing -C linker=clang, the linker invocation also included the argument -nostartfiles. For some reason, that argument has gone away under 1.67.1 with this invocation sequence, but one can re-add it manually by passing -C link-arg=-nostartfiles into rustc.

The questions I have are:

  1. Why did this change between 1.66 and 1.67? Was it an expected change in behavior? (Why is this change good? Its not noted in the release notes that I saw.)
  2. Should we bring back the old behavior, at least in a limited set of cases?
  3. Is there a different invocation our users should be using when they override the linker, e.g. perhaps they should be using -C linker-flavor=xxx to tell rustc that it needs to pass along parameters compatible with using clang as a linker? (But if that is the case, I do not see clang listed as one of the available values for linker-flavor)
  4. Should we start investing in post-processing the linker-errors from rustc itself -- and still spit out those linker errors (because in the general case, the user is going to need to see them in order to make sense of what to do in reaction), but potentially also provide hints as to things to look into (e.g. in this case, maybe the crt1.o in the error could be used as a hint to look at the -nostartfiles option as something to pass to the linker).
@pnkfelix pnkfelix added C-bug Category: This is a bug. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. labels Mar 8, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Mar 8, 2023
@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-linkage Area: linking into static, shared libraries and binaries O-wasm Target: WASM (WebAssembly), http://webassembly.org/ labels Mar 8, 2023
@cuviper
Copy link
Member

cuviper commented Mar 8, 2023

Under 1.66.1 when passing -C linker=clang, the linker invocation also included the argument -nostartfiles. For some reason, that argument has gone away under 1.67.1 with this invocation sequence, but one can re-add it manually by passing -C link-arg=-nostartfiles into rustc.

I think that was #104137, which removed link_self_contained: LinkSelfContainedDefault::True from the common wasm_base.rs. When self_contained was true, fn add_order_independent_options called cmd.no_crt_objects(), which adds -nostartfiles.

@pnkfelix
Copy link
Member Author

pnkfelix commented Mar 8, 2023

Indeed, cargo-bisect-rustc reports that commit e69336e is to blame here, which is the rollup that included #104137.

So that seems like strong evidence for what the relevant change was here.

My next question is: What should we do about this?

Should we readd link_self_contained: LinkSelfContainedDefault::True to the wasm_base.rs?

@bjorn3
Copy link
Member

bjorn3 commented Mar 8, 2023

Explicitly adding -nostartfiles to the linker args makes more sense I think.

@apiraino
Copy link
Contributor

apiraino commented Mar 9, 2023

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 9, 2023
@pnkfelix
Copy link
Member Author

pnkfelix commented Mar 9, 2023

Explicitly adding -nostartfiles to the linker args makes more sense I think.

Sorry, @bjorn3 , I don't know how to interpret your suggestion.

In particular, who should be responsible for adding -nostartfiles?

  • The person who is overriding the link driver via the -C linker parameter? (In which case, should we have this in the release note for 1.67?)
  • The compiler itself?

@bjorn3
Copy link
Member

bjorn3 commented Mar 9, 2023

The compiler itself. The target specification has a list of linker arguments to pass. As I understand it we used to pass this before as side effect of using link-self-contained, but now it needs to be made explicit in the target spec.

@pnkfelix
Copy link
Member Author

pnkfelix commented Mar 9, 2023

The compiler itself. The target specification has a list of linker arguments to pass. As I understand it we used to pass this before as side effect of using link-self-contained, but now it needs to be made explicit in the target spec.

Okay, great. That sounds like you're in favor then of @cuviper 's idea of having an explicit flag in the target spec, as they mentioned on zulip over here, specifically a no_crt_objects flag.

(On the other hand, on that same zulip topic thread, @sunfishcode advised not adding a new flag, and "just" reverting the change to wasm_base.rs so that it returns to using link_self_contained: LinkSelfContainedDefault::True for now...)

@cuviper
Copy link
Member

cuviper commented Mar 9, 2023

Targets also have the ability to add explicit link args:

fn link_args(flavor: LinkerFlavor, args: &[&'static str]) -> LinkArgs {
let mut link_args = LinkArgs::new();
add_link_args(&mut link_args, flavor, args);
link_args
}
fn add_pre_link_args(&mut self, flavor: LinkerFlavor, args: &[&'static str]) {
add_link_args(&mut self.pre_link_args, flavor, args);
}
fn add_post_link_args(&mut self, flavor: LinkerFlavor, args: &[&'static str]) {
add_link_args(&mut self.post_link_args, flavor, args);
}

But given that we already have the Linker::no_crt_objects abstraction, I thought we might as well use a flag for that.

bors added a commit to rust-lang-ci/rust that referenced this issue Mar 29, 2023
…that-broke-wasm-linker-overriding, r=petrochenkov

Rollback part of pr 104137 that broke wasm linker overriding

This is a quick fix to address rust-lang#108910
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linkage Area: linking into static, shared libraries and binaries C-bug Category: This is a bug. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants