🐛 Symbol multiply defined since nightly-2023-12-02 with custom builtins.

[ithinuel]

cargo build --release fails when building an rp2040 application with lto=fat.

Code

To reproduce:

git clone https://github.com/ithinuel/sandbox-rp2040 -b rust-regression

cd sandbox-rp2040
cargo build --release

I expected to see the project build successfully.

Instead, it fails with:

warning: Linking globals named '__addsf3': symbol multiply defined!

error: failed to load bitcode of module "rp2040_hal-99af59d59c714fe0.rp2040_hal.6c650a4395998581-cgu.0.rcgu.o": 

warning: `rust-regression` (bin "rust-regression") generated 1 warning
error: could not compile `rust-regression` (bin "rust-regression") due to 1 previous error; 1 warning emitted

bisect-rustc

searched nightlies: from nightly-2023-12-01 to nightly-2023-12-03
regressed nightly: nightly-2023-12-02
searched commit range: 87e1447...8c2b577
regressed commit: 8c2b577

bisected with cargo-bisect-rustc v0.6.7

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

cargo bisect-rustc --start=2023-12-01 --end=2023-12-03 -vv --target thumbv6m-none-eabi -- build --release 

@rustbot modify labels: +regression-from-stable-to-nightly -regression-untriaged

Background

The rp2040 mcu has in-ROM implementation of some builtins. The rp2040-hal implements the necessary magic in order to use these rather than the regular compiler-builtins that would otherwise consume flash are induce unecessary execution latency.
See here for memcpy, memset (and a few other) and here for float related builtins.

Such magic can be disabled with --features disable-intrinsics which allows to build on nightly-2023-12-02 at the cost of flash storage & execution speed.

initial report

• Bare template crashes during release build with lto = 'fat' in nightly rp-rs/rp2040-project-template#71

[ithinuel]

cc @DianQK working on another regression by the same commit:

• Windows x64 no-std build broken with errors related to compiler-builtins #118559
• Restore #![no_builtins] crates participation in LTO. #113923

[Dirbaio]

I'm not sure if this is a Rust bug, I don't think defining the builtins from a user crate like that was ever allowed. It was already broken before with build-std + lto=fat.

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@ithinuel Would it be possible to reduce the sample code provided and include more debug information about the linking issue? Just as an additional help figuring out this issue. Thanks.

@rustbot label -I-prioritize +P-high +E-needs-mcve

[DianQK]

I'm not sure if this is a Rust bug, I don't think defining the builtins from a user crate like that was ever allowed. It was already broken before with build-std + lto=fat.

In fact, compiler-builtins provide customization options through weak linkage at https://github.com/rust-lang/compiler-builtins/blob/2731a4837bfe091a0ba3e449e3ebeff73a6d2562/Cargo.toml#L69-L78.

I tried it but I got a new error:

warning: __aeabi_uidiv changed binding to STB_WEAK
error: symbol '__aeabi_uidiv' is already defined
warning: __aeabi_idiv changed binding to STB_WEAK
error: symbol '__aeabi_idiv' is already defined

I am investigating this issue while looking for a suitable solution. LLVM provides a symbol override method for linking IRs, which is one possible way.

@rustbot label +A-linkage
@rustbot claim

[ithinuel]

@apiraino the sample provided on the rust-regression branch is already pretty minimal.
This should generate a viable binary.
I'm not wellversed enough on linker options to improve that debug-ability but any recommendation is welcome.

@DianQK That is an interesting feature but it doesn't seem to be enabled by default. Does this mean that to get this to work as expected, we'd have to rely on nighlty's -Z build-std=core,compiler-builtins ?

[gwilymk]

We're seeing the same issue with STB_WEAK in the agb crate: agbrs/agb#521

[DianQK]

@apiraino the sample provided on the rust-regression branch is already pretty minimal. This should generate a viable binary. I'm not wellversed enough on linker options to improve that debug-ability but any recommendation is welcome.

fn main() {
}

#[no_mangle]
pub fn __addsf3() {
    return;
}

Then just run rustc +nightly -Clto=fat main.rs.

@rustbot label -E-needs-mcve

@DianQK That is an interesting feature but it doesn't seem to be enabled by default. Does this mean that to get this to work as expected, we'd have to rely on nighlty's -Z build-std=core,compiler-builtins ?

I'm sorry, I'm not sure. If no_std is used, this should be resolved by adding compiler-builtins to the project.

[DianQK]

To solve this issue, I need to first merge llvm/llvm-project#75696.

After that, change the LTO of rust to llvm-link user.bc --ignore-if-conflict compiler-builtins.bc.

[DianQK]

Adding weak will solve part of the issue.

The following error is due to LLVM IR not recognizing symbols defined in ASM.

warning: __aeabi_uidiv changed binding to STB_WEAK
error: symbol '__aeabi_uidiv' is already defined

Refer: llvm/llvm-project#75696 (comment).

To solve this issue, we should use the naked function. I tried to file the rp-rs/rp-hal#728 PR. I'm not familiar with the program. I'm just verifying that this can compile successfully.

[kleisauke]

Since nightly-2023-12-02, I have encountered a similar issue involving x86_64-pc-windows-gnullvm, where ___chkstk_ms is defined within a static archive file. This leads to a duplicate symbol error when linking that static archive into a C codebase.

The original reproducer is somewhat complex (it involves building librsvg and dependencies), but I managed to reduce it to this Dockerfile:

Details

# Build with:
# docker build -t llvm-mingw .
FROM mstorsjo/llvm-mingw:latest

# Path settings
ENV RUSTUP_HOME="/usr/local/rustup" \
    CARGO_HOME="/usr/local/cargo" \
    PATH="/usr/local/cargo/bin:$PATH"

# Install curl
RUN apt-get update -qq && \
    apt-get install -qqy --no-install-recommends curl && \
    apt-get clean -y && \
    rm -rf /var/lib/apt/lists/*

# Install Rust
RUN curl https://sh.rustup.rs -sSf  | sh -s -- -y \
      --no-modify-path \
      --profile minimal \
      --target x86_64-pc-windows-gnullvm \
      --default-toolchain nightly \
      --component rust-src

WORKDIR /build

# Special flags for Rust
ENV CARGO_PROFILE_RELEASE_DEBUG=false \
    CARGO_PROFILE_RELEASE_CODEGEN_UNITS=1 \
    CARGO_PROFILE_RELEASE_INCREMENTAL=false \
    CARGO_PROFILE_RELEASE_LTO=true \
    CARGO_PROFILE_RELEASE_OPT_LEVEL=z \
    CARGO_PROFILE_RELEASE_PANIC=abort

RUN cargo new foo --lib --vcs none && \
    cd foo && \
    echo "pub fn foo() {}" > src/lib.rs && \
    cargo rustc --release --crate-type=staticlib --target=x86_64-pc-windows-gnullvm -Zbuild-std=std,panic_abort -Zbuild-std-features=compiler-builtins-weak-intrinsics && \
    llvm-nm -u target/x86_64-pc-windows-gnullvm/release/libfoo.a | grep -c ___chkstk_ms

Doing:

@@ -18,7 +18,7 @@ RUN curl https://sh.rustup.rs -sSf  | sh -s -- -y \
       --no-modify-path \
       --profile minimal \
       --target x86_64-pc-windows-gnullvm \
-      --default-toolchain nightly \
+      --default-toolchain nightly-2023-12-01 \
       --component rust-src
 
 WORKDIR /build

Makes the build pass.

While investigating this further, I managed to resolve this issue with this commit:
kleisauke@a7c2947

This solution involves not treating weak symbols as built-in functions during LTO, but I'm not sure if that would be a correct fix.

[ia0]

In fact, compiler-builtins provide customization options through weak linkage

Thanks! This is a good work-around for my use-case, which is that I build a staticlib crate libfoo.a and get duplicate symbols on the compiler builtins (like __adddf3) when linking in the final binary. If I add this dependency to the crate foo, then it links without error:

compiler_builtins = { version = "0.1.105", features = ["weak-intrinsics"] }

When comparing nm libfoo.a between old (before 2023-12-02), new (after 2023-12-02), and fix version (after 2023-12-02 with weak symbols), I get:

00000000 T __adddf3 # old
00000001 T __adddf3 # new
00000001 W __adddf3 # fix

I'm not sure what a size of zero means. Essentially the symbol was already defined before when it was working, but it was empty (IIUC) so probably was being merged without complaining? Then it got some size and failed. And with weak it's simply overwritten or merged.

[DianQK]

I tried a way to do it without the naked function at llvm/llvm-project#76040. I'm not sure it's appropriate to do so, though.

[DianQK]

I'm not sure what a size of zero means. Essentially the symbol was already defined before when it was working, but it was empty (IIUC) so probably was being merged without complaining? Then it got some size and failed. And with weak it's simply overwritten or merged.

I'm not sure if there is a issue. Can you provide a reproduction?

[ia0]

I'm not sure if there is a issue. Can you provide a reproduction?

Here's a tarball to reproduce: 118609.tar.gz

tar xzvf 118609.tar.gz

Building with an "old" nightly works.

% ./118609/bar/build.sh +nightly-2023-11-14
[...]
00000000 T __adddf3
[...]

Building with a "new" nightly fails.

% ./118609/bar/build.sh +nightly-2023-12-15
[...]
00000001 T __adddf3
[...]
  = note: rust-lld: error: duplicate symbol: __fixdfti
[...]
          rust-lld: error: too many errors emitted, stopping now (use --error-limit=0 to see all errors)
[...]

Building with compiler_builtins/weak-intrinsics works.

% ./118609/bar/build.sh +nightly-2023-12-15 --features=compiler_builtins/weak-intrinsics
[...]
00000001 W __adddf3
[...]

Building with compiler_builtins default features fails.

% ./118609/bar/build.sh +nightly-2023-12-15 --features=compiler_builtins
[...]
00000001 T __adddf3
[...]
  = note: rust-lld: error: duplicate symbol: __gesf2
[...]
          rust-lld: error: too many errors emitted, stopping now (use --error-limit=0 to see all errors)
[...]

Note that the duplicate symbols are provided in a seemingly non-deterministic order, so the first one may be different when reproducing.

[ia0]

Something to notice (maybe it's useful to debug) is that you have to put the compiler_builtins dependency in the top-level crate of the static library (i.e. foo). If it's only an indirect dependency of foo (say some qux on which foo depends) then linking of bar with libfoo.a still fails with duplicate symbols. You can also see that in this case, when building libfoo.a, the __adddf3 symbol is not weak, although compiler_builtins is in the dependency path with the weak-intrinsics feature enabled.

[DianQK]

Something to notice (maybe it's useful to debug) is that you have to put the compiler_builtins dependency in the top-level crate of the static library (i.e. foo). If it's only an indirect dependency of foo (say some qux on which foo depends) then linking of bar with libfoo.a still fails with duplicate symbols. You can also see that in this case, when building libfoo.a, the __adddf3 symbol is not weak, although compiler_builtins is in the dependency path with the weak-intrinsics feature enabled.

This is similar to an issue I saw earlier, but I can't find it at the moment. Can you tell us why you created the rust static library? Why can't you just add it as a dependency?

This builtin symbol should become a local symbol. I'm not sure why it's a global symbol now. There is a workaround that changes the symbols of the object file to local via a symbol export file.

[ia0]

Can you tell us why you created the rust static library? Why can't you just add it as a dependency?

The static library doesn't need to be written in Rust, it just happens to be in that case. And another reason I can't add it as a dependency is that it's not always the same library (it's not even taken from a finite set, it can be anything exporting the right symbols).

For more context

Wasefire provides a platform, written in Rust, on which WebAssembly applets can run. There is an option to actually bypass WebAssembly and run an applet natively. In that case, the applet is compiled to a static library for the same target architecture as the platform. Applets may be written in any language that targets WebAssembly in the general case, or the target architecture of the platform for the native case.

There is a workaround that changes the symbols of the object file to local via a symbol export file.

Could you tell more about this? Can this be specifically applied to compiler_builtins given that I need the exported symbols from my static library (I also need the "imported"/undefined symbols because the library calls into the platform)?

[kleisauke]

It looks like the compiler-rt builtins provided by LLVM are always compiled with -fno-lto.
https://github.com/llvm/llvm-project/blob/llvmorg-17.0.6/compiler-rt/cmake/Modules/AddCompilerRT.cmake#L183-L189

Perhaps Rust should do the same thing in this case? I think that would make it possible for non-LTO projects to link to static Rust archives built under FatLTO (i.e. the case mentioned in comment #118609 (comment)).

[bjorn3]

That is what we did before #113923. The problem with that is that compiler-builtins tends to depend on libcore, whose symbols will be made private by LTO resulting in a linker error, unless it is compiled in exactly the right way to avoid this dependency.

[jannic]

Not surprisingly, this now also affects beta. (1.76.0-beta.1)

[DianQK]

There is a workaround that changes the symbols of the object file to local via a symbol export file.

Could you tell more about this? Can this be specifically applied to compiler_builtins given that I need the exported symbols from my static library (I also need the "imported"/undefined symbols because the library calls into the platform)?

I apologize for my late response. This is only available in Mach-O. But you can use objcopy --localize-symbol=__adddf3 input.o output.o.

[ia0]

Thanks for the fix!

[danakj]

Thanks for addressing this, it was also blocking Rust rolls in Chrome, due to collisions between C++ and Rust, which we were trying to find ways to work around.

https://bugs.chromium.org/p/chromium/issues/detail?id=1509926

[DianQK]

Thanks for addressing this, it was also blocking Rust rolls in Chrome, due to collisions between C++ and Rust, which we were trying to find ways to work around.

https://bugs.chromium.org/p/chromium/issues/detail?id=1509926

So the issue was initially undefined symbols then become multiply defined symbols? Since revert is now an undefined symbol issue again?
I just want to make sure that there are still issues to be addressed here.

[danakj]

Hey Dian, good question, the bug has a lot going on. The problem was missing symbols when LTO is enabled on Android. (Note: we are using lld as the linker, and linker-driven thin LTO.) https://bugs.chromium.org/p/chromium/issues/detail?id=1509926#c9 was my best guess at what was wrong, but might be way off.

The problem changed later to duplicates only because of how we tried to work around the problem, by playing with --whole-archive on the clang compiler-builtins archive, so that's not related really.

Since revert, I expect there's no problem at the moment, but we'll see how the next roll attempt goes. I will let you know here if there's still something wrong after the revert, but expect otherwise.