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: multiple definitions of function #123289

Closed
Mark-Simulacrum opened this issue Mar 31, 2024 · 11 comments
Closed

regression: multiple definitions of function #123289

Mark-Simulacrum opened this issue Mar 31, 2024 · 11 comments
Assignees
Labels
A-linkage Area: linking into static, shared libraries and binaries T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@Mark-Simulacrum
Copy link
Member

[INFO] [stdout]   = note: /usr/bin/ld: /opt/rustwide/target/debug/deps/libavif_parse-3944f88254156a01.rlib(avif_parse-3944f88254156a01.avif_parse.f31058791515eadf-cgu.3.rcgu.o): in function `avif_parse':
[INFO] [stdout]           /opt/rustwide/cargo-home/registry/src/index.crates.io-6f17d22bba15001f/avif-parse-0.13.2/src/c_api.rs:27: multiple definition of `avif_parse'; /opt/rustwide/target/debug/deps/libavif_parse-8988254f09aeef86.rlib(avif_parse-8988254f09aeef86.avif_parse.d6c78051b3aeaa35-cgu.3.rcgu.o):/opt/rustwide/cargo-home/registry/src/index.crates.io-6f17d22bba15001f/avif-parse-1.0.0/src/c_api.rs:27: first defined here
[INFO] [stdout]           /usr/bin/ld: /opt/rustwide/target/debug/deps/libavif_parse-3944f88254156a01.rlib(avif_parse-3944f88254156a01.avif_parse.f31058791515eadf-cgu.3.rcgu.o): in function `core::ptr::const_ptr::<impl *const T>::is_null::runtime_impl':
[INFO] [stdout]           /opt/rustwide/cargo-home/registry/src/index.crates.io-6f17d22bba15001f/avif-parse-0.13.2/src/c_api.rs:50: multiple definition of `avif_data_free'; /opt/rustwide/target/debug/deps/libavif_parse-8988254f09aeef86.rlib(avif_parse-8988254f09aeef86.avif_parse.d6c78051b3aeaa35-cgu.3.rcgu.o):/opt/rustwide/cargo-home/registry/src/index.crates.io-6f17d22bba15001f/avif-parse-1.0.0/src/c_api.rs:50: first defined here
[INFO] [stdout]           collect2: error: ld returned 1 exit status
@Mark-Simulacrum Mark-Simulacrum added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Mar 31, 2024
@Mark-Simulacrum Mark-Simulacrum added this to the 1.78.0 milestone Mar 31, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 31, 2024
@saethlin saethlin added the A-linkage Area: linking into static, shared libraries and binaries label Mar 31, 2024
@saethlin saethlin self-assigned this Mar 31, 2024
@saethlin saethlin removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 31, 2024
@saethlin
Copy link
Member

saethlin commented Mar 31, 2024

avif-decode has two different versions of avif-parse in its dependency tree, and avif-parse exports the symbols avif_parse and avif_data_free. I think this crate has only ever managed to link due to optimistic Linux linker optimization shenaniganry.

The change bisects to #122233. I think there's a CGU partitioning that causes the build to successfully link, and that PR perturbed enough code to alter how CGUs get merged. The crate still builds on nightly and on stable if you set -Ccodegen-units=9 or any greater value, but fails to build on nightly and on stable with -Ccodegen-units=8 or any lesser value.

@saethlin
Copy link
Member

@estebank fresh example of where it would be great to have better linker diagnostics

@Mark-Simulacrum
Copy link
Member Author

Sounds good. I think there's no true regression here in that case, dropping various labels and I suspect we can just close (will do that unless we want to track it as diagnostics, but doesn't seem worth it to me)

@saethlin
Copy link
Member

Esteban was musing about writing better linker diagnostics, and now that I've assigned myself to this I'll be better able to find it if I do cook up something later.

I agree this is better closed.

@estebank
Copy link
Contributor

estebank commented Apr 1, 2024

@saethlin keep me posted on whatever ticket for the diagnostics part of this you create, so I can help.

@bjorn3
Copy link
Member

bjorn3 commented Apr 1, 2024

Would using --whole-archive on all rlibs have ensured that it caused a linker error independent of how the CGUs are partitioned?

@saethlin
Copy link
Member

saethlin commented Apr 1, 2024

I tried building [email protected] with

RUSTFLAGS="-Clink-arg=-Wl,--whole-archive -Ccodegen-units=128" cargo +nightly b

And I don't get a linker error. Even with that flag there seems to be a CGU threshold that makes the linker accept the inputs.

@bjorn3
Copy link
Member

bjorn3 commented Apr 1, 2024

-Clink-arg is passed after rlibs are mentioned in the linker commandline:

// Can be used for arbitrary order-independent options.
// In practice may also be occasionally used for linking native libraries.
// Passed after compiler-generated options to support manual overriding when necessary.
add_user_defined_link_args(cmd, sess);
. --whole-archive is a position-dependent linker argument. It applies to all archives after --whole-archive until the next --no-whole-archive. You did have to use -Zpre-link-arg and check that rustc doesn't emit any --no-whole-archive to reset a --whole-archive it emitted for a single staticlib. Or you need to modify rustc directly and change the false here to a true:
cmd.link_staticlib_by_path(&rlib_path, false);

@saethlin
Copy link
Member

saethlin commented Apr 1, 2024

If I modify the compiler as you described I can indeed get the multiple definition error regardless of CGUs.

I can't build anything at all with that -Zpre-link-arg because of symbol conflicts between compiler_builtins and libc:

/usr/bin/ld: /usr/lib/gcc/x86_64-pc-linux-gnu/13.2.1/libgcc.a(_negdi2.o): in function `__negti2':
(.text+0x0): multiple definition of `__negti2'; /home/ben/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcompiler_builtins-5b3e30276043b6e7.rlib(45c91108d938afe8-negti2.o):/cargo/registry/src/index.crates.io-6f17d22bba15001f/compiler_builtins-0.1.108/./lib/builtins/negti2.c:19: first defined here

@anisse
Copy link

anisse commented Jun 11, 2024

I just got hit by this issue. Project built and ran with no issue with rust 1.77.2, and now fails to link with rust 1.78.0:

[...]
/home/anisse/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wgpu-core-0.16.1/src/command/render.rs:2575: multiple definition of `wgpu_render_pass_execute_bundles'; /home/anisse/dev/gears-repo/target/release/deps/libwgpu_core-3c5c7590aab874f9.rlib(wgpu_core-3c5c7590aab874f9.wgpu_core.e28489e2a4961374-cgu.02.rcgu.o):/home/anisse/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wgpu-core-0.18.1/src/command/render.rs:2832: first defined here
[...]

Looking at the cargo tree output, it's obvious that the duplicate wgpu versions might not be what I want. Indirect dep is being bumped, but not ready yet.

I'm not sure if this warrants a separate issue, but this is definitely an unexpected regression. I'll leave it as-is because the fix is trivial on my side (just use older wgpu version aligned with pixels').

anisse added a commit to anisse/gears that referenced this issue Jun 11, 2024
Since rust 1.78.0, we have a linker error because of the different
wgpu versions in the dependency tree:
rust-lang/rust#123289

This worksaround this issue, and should also be more correct, because we
might not want to have different wgpus in parallel.
anisse added a commit to anisse/gears that referenced this issue Jun 11, 2024
Since rust 1.78.0, we have a linker error because of the different
wgpu versions in the dependency tree:
rust-lang/rust#123289

This worksaround this issue, and should also be more correct, because we
might not want to have different wgpus in parallel.
@saethlin
Copy link
Member

This is an equivalent situation; on your project's commit 4cc41a902f1b1671d98103d7972672357716d9b8, 1.77.2 will manage to link it but applying the above change to link rlibs with --whole-archive causes it to not link.

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

6 participants