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

regression - nightly 2023-03-30 -> 2023-03-31 causes linker error when importing wasm_bindgen::JsValue in a proc_macro #111888

Closed
finnbear opened this issue May 23, 2023 · 14 comments · Fixed by #114470
Assignees
Labels
A-linkage Area: linking into static, shared libraries and binaries C-bug Category: This is a bug. P-high High 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

@finnbear
Copy link
Contributor

finnbear commented May 23, 2023

Notes for other investigators

(2023-07-19)

  1. stylist-rs has been updated with a workaround, so the reproduction described below will need adapting (e.g. check out that repo from around futursolo/stylist-rs@fbd788f or earlier, since that will not include the workaround added in Mitigate linker error from Rust compiler futursolo/stylist-rs#123 )

  2. This problem exhibits itself only with certain numbers of codegen-units. The default setting for codegen-units is host dependent (the default is chosen, in part, as a function of the number of cores the host has available, in order to parallelize the code-generation performed by LLVM). In my own experiments, the problem is masked (does not arise) for codegen-units={1,2,4,8}, and the problem does arise for codegen-units={16,32,64,128,256}. You are best off passing -Ccodegen-units explicit when investigating this, just to ward against that being an invisible factor in the experiments.

(bug report follows below)

Code

I tried this code:

[package]
name = "nightly_stylist"
version = "0.1.0"
edition = "2021"

[dependencies]
stylist-macros = "0.12"
fn main() {}

stylist-macros

I expected to see this happen: cargo check succeeds in debug mode.

Instead, this happened: cargo check fails in debug mode.

  = note: /usr/bin/ld: __wbindgen_realloc: undefined version: 
          /usr/bin/ld: __wbindgen_malloc: undefined version: 
          /usr/bin/ld: __wbindgen_free: undefined version: 
          /usr/bin/ld: __wbindgen_exn_store: undefined version: 
          /usr/bin/ld: failed to set dynamic section sizes: bad value
          collect2: error: ld returned 1 exit status

Version it worked on

rustc 1.70.0-nightly (ec2f40c6b 2023-03-30)
binary: rustc
commit-hash: ec2f40c6b04f0e9850dd1f454e8639d319f4ed9b
commit-date: 2023-03-30
host: x86_64-unknown-linux-gnu
release: 1.70.0-nightly
LLVM version: 16.0.0

Version with regression

rustc 1.70.0-nightly (5e1d3299a 2023-03-31)
binary: rustc
commit-hash: 5e1d3299a290026b85787bc9c7e72bcc53ac283f
commit-date: 2023-03-31
host: x86_64-unknown-linux-gnu
release: 1.70.0-nightly
LLVM version: 16.0.0

Cargo bisect-rustc

searched toolchains ec2f40c6b04f0e9850dd1f454e8639d319f4ed9b through 5e1d3299a290026b85787bc9c7e72bcc53ac283f


********************************************************************************
Regression in 22a7a19f9333bc1fcba97ce444a3515cb5fb33e6
********************************************************************************

*isn't immediately obvious why this change would cause the problem

searched nightlies: from nightly-2023-03-27 to nightly-2023-04-02
regressed nightly: nightly-2023-04-01
searched commit range: ec2f40c...5e1d329
regressed commit: 22a7a19

bisected with cargo-bisect-rustc v0.6.6

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start 2023-03-27 --end 2023-04-02 
@finnbear finnbear added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels May 23, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 23, 2023
@finnbear finnbear changed the title regression - nightly 2023-03-30 -> 2023-03-31 causes linker error when importing stylist-macros regression - nightly 2023-03-30 -> 2023-03-31 causes linker error when importing wasm_bindgen::JsValue in a proc_macro May 24, 2023
@finnbear
Copy link
Contributor Author

finnbear commented May 24, 2023

I forked stylist and minimized the reproducible example down to simply having a type (but not a value) with wasm_bindgen::JsValue in a proc_macro crate (and then adding that proc macro crate as a dependency of another crate).

You can find it here: https://github.com/finnbear/stylist-rs/tree/rustc (just run cargo check in packages/stylist)

Here is a workaround for the crate: futursolo/stylist-rs#121

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high +T-compiler

@rustbot rustbot added P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels May 24, 2023
@lqd
Copy link
Member

lqd commented May 29, 2023

TBH it's not clear to me how alignment checks would end up causing linker errors, but since this seems bisected to #98112, let's at the very least notify the PR's author @saethlin.

@saethlin
Copy link
Member

I already looked over this and was also mystified. I suppose I could take another look? I know almost completely nothing about wasm so I'm not hopeful.

@saethlin
Copy link
Member

Builds fine:

RUSTFLAGS=-Zmir-enable-passes=-CheckAlignment cargo +nightly build

Same linker error:

RUSTFLAGS=-Zmir-enable-passes=-CheckAlignment cargo +nightly build -Zbuild-std --target=x86_64-unknown-linux-gnu

As a mitigation for the linker error, users may put this in their Cargo.toml (for the workspace!)

[profile.dev.package.wasm-bindgen]
debug-assertions = false

That only helps with the alignment checks, -Zbuild-std is still not going to work.

@saethlin
Copy link
Member

saethlin commented May 29, 2023

I can build the reproducer, unmodified, if I use LLD.

[target.x86_64-unknown-linux-gnu]
linker = "clang"
rustflags = ["-Clink-arg=-fuse-ld=lld"]

Or

RUSTFLAGS="-Clinker=clang -Clink-arg=-fuse-ld=lld" cargo +nightly build

https://borngeek.com/2009/3/13/gcc-dynamic-section-sizes-error/ suggests this might be a link order issue. That would explain why LLD is fine with the link, but GNU's linker isn't.

Actually, that doesn't make much sense because our standard library is already at the end of the link. I'm out of ideas 🤷 I think this is somehow dependent on linker details that I don't know and I'm struggling to educate myself on.

@saethlin saethlin added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels May 29, 2023
@jyn514 jyn514 added O-wasm Target: WASM (WebAssembly), http://webassembly.org/ A-linkage Area: linking into static, shared libraries and binaries and removed regression-untriaged Untriaged performance or correctness regression. labels Jun 1, 2023
@saethlin
Copy link
Member

@JohnTheCoolingFan reported this same linker error from tealr_derive. It can be reproduced by creating an empty Cargo project, then running cargo add [email protected] --features=embed_compiler_from_download then cargo build. I have not managed to minimize this reproducer.

The errant symbol is bz_internal_error which is defined as:

#[no_mangle]
pub fn bz_internal_error(errcode: c_int) {
    panic!("bz internal error: {}", errcode);
}

This has the same sort of shape as the non-unwinding panics from the alignment checks and from the standard library UB-detecting assertions.

searched nightlies: from nightly-2023-01-01 to nightly-2023-06-09
regressed nightly: nightly-2023-03-12
searched commit range: ff4b772...8a73f50
regressed commit: e350fe4

bisected with cargo-bisect-rustc v0.6.6

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --start 2023-01-01 --end 2023-06-09 --script script --regress=error 

So this bisects to a rollup then can't find the unrolled perf builds. Manual trials identifies #108017 as the source of the regression.

@saethlin saethlin added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ labels Jun 10, 2023
JohnTheCoolingFan added a commit to JohnTheCoolingFan/scriplets that referenced this issue Jun 10, 2023
@pnkfelix
Copy link
Member

pnkfelix commented Jun 29, 2023

A tiny update: it seems like this problem goes away if you use -Ccodegen-units=1 (and for me, it also didn't reproduce for many other values for number-of-codegen-units; I won't bother enumerating those here).

This was somewhat of a surprise. I need to investigate more to figure out what relevance the codegen unit partitioning has to do with this issue.

If nothing else, it is a halfway decent fallback measure while we continue to investigate the problem.

@kurtbuilds
Copy link

kurtbuilds commented Jul 10, 2023

Wanted to add, that I also ran into this issue. For me, it also occurred while compiling a macro crate. I originally thought it was an issue with libc, as it happened after I ran a cargo update, and the issue was fixed by pinning libc to a slightly older version: rust-lang/libc#3297

There was a bunch of weird behavior I encountered when trying to uncover the issue. The libc issue was one. When I made other library upgrade changes, it would cause or fix the breakage.

I didn't dive too deeply, but it seems like I was still encountering this issue even when I set the compiler to 1.67. (See this Github Actions build and associated commit: https://github.com/kurtbuilds/oasgen/actions/runs/5513164141 ) So it's possible that going to an old version simply hid the issue, but didn't actually pinpoint where the issue was introduced.

Let me know if there's any other information I can provide to help isolate the issue.

I've seemingly been able to work around the issue by using lld, as described earlier: #111888 (comment) I didn't try the other solution about codegen-units.

langyo added a commit to celestia-island/hikari that referenced this issue Jul 17, 2023
langyo added a commit to celestia-island/hikari that referenced this issue Jul 17, 2023
@pnkfelix pnkfelix self-assigned this Jul 18, 2023
@pnkfelix
Copy link
Member

pnkfelix commented Jul 19, 2023

One MCVE for the stylist-macros build issue looks like this: Just a stylist-macros crate, with this Cargo.toml and lib.rs:

# mcve-stylist/Cargo.toml
[package]
name = "mcve-stylist"
version = "0.1.0"

[lib]
proc-macro = true

[dependencies]
wasm-bindgen = "0.2.83"
// mcve-stylist/src/lib.rs
extern crate wasm_bindgen;

So now I'm switching gears to see if I reduce wasm_bindgen to something smaller, so that we can have a small set of crates to talk about on their own here.

@pnkfelix
Copy link
Member

pnkfelix commented Jul 20, 2023

Okay got a pretty impressively tiny MCVE for wasm-bindgen too:

# wasm-bindgen/Cargo.toml
[package]
name = "wasm-bindgen"
version = "0.2.87"
// wasm-bindgen/src/lib.rs
#[no_mangle]
pub extern "C" fn __wbindgen_malloc(_size: usize, _align: usize) -> *mut u8 {
    loop {}
}

#[no_mangle]
pub unsafe extern "C" fn __wbindgen_realloc(
    _ptr: *mut u8,
    _old_size: usize,
    _new_size: usize,
    _align: usize,
) -> *mut u8 {
    loop {}
}

#[no_mangle]
pub unsafe extern "C" fn __wbindgen_free(_ptr: *mut u8, _size: usize, _align: usize) {
    loop {}
}

#[no_mangle]
pub unsafe extern "C" fn __wbindgen_exn_store(_idx: u32) {
    loop {}
}

In case its not clear, this seems like a pretty simple example that should easily work out of the box. (The use of proc-macro from the mcve-stylist part of the story seems like an important part of the reproduction.)

Still looking.

Update: Wow, is it somehow #[no_mangle] of all things that is causing the problem here? That is ... bizarre, to say the least.

@bjorn3
Copy link
Member

bjorn3 commented Jul 20, 2023

#99978 may be relevant. That issue is about not mentioning #[no_mangle] symbols in the version script such that they don't get exported. Also when applying -Wl,--whole-archive to the rlib containing the #[no_mangle] symbols the linker error is fixed, so it seems like the linker doesn't pull in archive members even when the version script mentions it.

Edit: For regular dylibs we mention all these symbols in symbols.o, but for proc macros we only list a couple of internal symbols.
Edit2: There is a special case for proc macros with the symbols.o handling to not mention any symbols beyond those required to handle circular dependencies. Fixing #99978 should fix this issue too by making the version script a subset of symbols.o again (apart from a couple of symbols defined outside of archives).

@pnkfelix
Copy link
Member

pnkfelix commented Jul 20, 2023

@saethlin wrote:

So this bisects to a rollup then can't find the unrolled perf builds. Manual trials identifies #108017 as the source of the regression.

As an addendum to this: #108017 (which was in a rollup, as mentioned) added the future-proofing safe-guard of adding --no-undefined-version to the linker invocation. We can add that same link-arg ourselves as part of further bisection that acts prior to PR #108017.

In my own MCVE, when I add that link-arg, I find that the code did compile circa 1.63 even with that --no-undefined-version flag. The flag stopped working sometime between nightly-2022-08-01 and nightly-2022-08-02. (I have not yet tracked down further than that, since its too old to have per-commit prebuilds archived.)

Update: oh, of course, it is almost certainly PR #99944 that injected the change in behavior mentioned in the previous paragraph.

badboy added a commit to mozilla/uniffi-rs that referenced this issue Jul 25, 2023
In m-c we're running into linking issues in `uniffi_macros`,
where it's missing symbols coming from `uniffi_core`:

    /bin/ld: uniffi_foreign_executor_callback_set: undefined version:
    /bin/ld: failed to set dynamic section sizes: bad value

That's most likely this issue on Rust:
rust-lang/rust#111888
It's called out that this likely actually broke because of another PR:
rust-lang/rust#99944

Despite this bug there's a bit of an issue in UniFFI to begin with:
We're exporting `extern "C"` functions from a crate that is a dependency
of some other crates, including `uniffi_macros`, and thus the symbols
land in a part where they are not needed.

However for `uniffi_meta` in particular we don't need much from
`uniffi_core`, so for now we just copy the necessary bits to get it all
working.
badboy added a commit to badboy/uniffi-rs that referenced this issue Jul 25, 2023
In m-c we're running into linking issues in `uniffi_macros`,
where it's missing symbols coming from `uniffi_core`:

    /bin/ld: uniffi_foreign_executor_callback_set: undefined version:
    /bin/ld: failed to set dynamic section sizes: bad value

That's most likely this issue on Rust:
rust-lang/rust#111888
It's called out that this likely actually broke because of another PR:
rust-lang/rust#99944

Despite this bug there's a bit of an issue in UniFFI to begin with:
We're exporting `extern "C"` functions from a crate that is a dependency
of some other crates, including `uniffi_macros`, and thus the symbols
land in a part where they are not needed.

However for `uniffi_meta` in particular we don't need much from
`uniffi_core`, so for now we just copy the necessary bits to get it all
working.
badboy added a commit to mozilla/uniffi-rs that referenced this issue Jul 25, 2023
In m-c we're running into linking issues in `uniffi_macros`,
where it's missing symbols coming from `uniffi_core`:

    /bin/ld: uniffi_foreign_executor_callback_set: undefined version:
    /bin/ld: failed to set dynamic section sizes: bad value

That's most likely this issue on Rust:
rust-lang/rust#111888
It's called out that this likely actually broke because of another PR:
rust-lang/rust#99944

Despite this bug there's a bit of an issue in UniFFI to begin with:
We're exporting `extern "C"` functions from a crate that is a dependency
of some other crates, including `uniffi_macros`, and thus the symbols
land in a part where they are not needed.

However for `uniffi_meta` in particular we don't need much from
`uniffi_core`, so for now we just copy the necessary bits to get it all
working.
badboy added a commit to mozilla/uniffi-rs that referenced this issue Jul 25, 2023
In m-c we're running into linking issues in `uniffi_macros`,
where it's missing symbols coming from `uniffi_core`:

    /bin/ld: uniffi_foreign_executor_callback_set: undefined version:
    /bin/ld: failed to set dynamic section sizes: bad value

That's most likely this issue on Rust:
rust-lang/rust#111888
It's called out that this likely actually broke because of another PR:
rust-lang/rust#99944

Despite this bug there's a bit of an issue in UniFFI to begin with:
We're exporting `extern "C"` functions from a crate that is a dependency
of some other crates, including `uniffi_macros`, and thus the symbols
land in a part where they are not needed.

However for `uniffi_meta` in particular we don't need much from
`uniffi_core`, so for now we just copy the necessary bits to get it all
working.
badboy added a commit to mozilla/uniffi-rs that referenced this issue Jul 25, 2023
In m-c we're running into linking issues in `uniffi_macros`,
where it's missing symbols coming from `uniffi_core`:

    /bin/ld: uniffi_foreign_executor_callback_set: undefined version:
    /bin/ld: failed to set dynamic section sizes: bad value

That's most likely this issue on Rust:
rust-lang/rust#111888
It's called out that this likely actually broke because of another PR:
rust-lang/rust#99944

Despite this bug there's a bit of an issue in UniFFI to begin with:
We're exporting `extern "C"` functions from a crate that is a dependency
of some other crates, including `uniffi_macros`, and thus the symbols
land in a part where they are not needed.

However for `uniffi_meta` in particular we don't need much from
`uniffi_core`, so for now we just copy the necessary bits to get it all
working.
badboy added a commit to mozilla/uniffi-rs that referenced this issue Jul 25, 2023
In m-c we're running into linking issues in `uniffi_macros`,
where it's missing symbols coming from `uniffi_core`:

    /bin/ld: uniffi_foreign_executor_callback_set: undefined version:
    /bin/ld: failed to set dynamic section sizes: bad value

That's most likely this issue on Rust:
rust-lang/rust#111888
It's called out that this likely actually broke because of another PR:
rust-lang/rust#99944

Despite this bug there's a bit of an issue in UniFFI to begin with:
We're exporting `extern "C"` functions from a crate that is a dependency
of some other crates, including `uniffi_macros`, and thus the symbols
land in a part where they are not needed.

However for `uniffi_meta` in particular we don't need much from
`uniffi_core`, so for now we just copy the necessary bits to get it all
working.
badboy added a commit to mozilla/uniffi-rs that referenced this issue Jul 26, 2023
In m-c we're running into linking issues in `uniffi_macros`,
where it's missing symbols coming from `uniffi_core`:

    /bin/ld: uniffi_foreign_executor_callback_set: undefined version:
    /bin/ld: failed to set dynamic section sizes: bad value

That's most likely this issue on Rust:
rust-lang/rust#111888
It's called out that this likely actually broke because of another PR:
rust-lang/rust#99944

Despite this bug there's a bit of an issue in UniFFI to begin with:
We're exporting `extern "C"` functions from a crate that is a dependency
of some other crates, including `uniffi_macros`, and thus the symbols
land in a part where they are not needed.

However for `uniffi_meta` in particular we don't need much from
`uniffi_core`, so for now we just copy the necessary bits to get it all
working.
@pnkfelix
Copy link
Member

pnkfelix commented Aug 4, 2023

See related discussion in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/linker.20errs.20from.20wasm.20exposed.20by.20mir.20align.20checks.20.23111888

(I have a branch with a commit that seems to fix this here: https://github.com/pnkfelix/rust/tree/dont-export-no-mangle-from-proc-macros-issue-99978 ; I hope to have a PR up today, once I get a regression test into place.)

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. P-high High 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

Successfully merging a pull request may close this issue.

9 participants