build-std: wasm32-unknown-unknown failing with overflow-checks=true and LTO

[athei]

Problem

It seems that recompiling libstd/libcore with overflow checks isn't something you want to to. At least it leads to some annoying bugs:
rust-lang/rust#78744
rust-lang/rust#91158

I wonder if recompiling the standard library with overflow checks is even useful. It seems that we are perfectly fine with enabling overflow-checks=true in some workspace and then link against the precompiled version of libstd (which isn't compiled without these checks). Please correct me if I am wrong here.

Proposed Solution

The -Z build-std feature should always disable overflow-checks when building the standard library no matter what is defined in the Cargo.toml of the workspace in question.

Notes

Is there some workaround where I can make the std build use a different profile than the workspace?

[ehuss]

That appears to be an issue that should just be fixed in the standard library, is there some reason that can't be done? Or is this maybe related to rust-lang/wg-cargo-std-aware#53?

[athei]

That appears to be an issue that should just be fixed in the standard library, is there some reason that can't be done?

I think we still want to compile the standard library without overflow checks even if we could get rid of this bug. How can this be solved in the library itself?

[ehuss]

Why do you think it would be good to not include overflow checks? I believe overflow checks are now enabled for all targets distributed for rustup except apple I think. I would think if someone is building with overflow checks, they would want them enabled everywhere, including std.

How can this be solved in the library itself?

It would require someone doing some investigation in to the root cause. If it is because of rust-lang/wg-cargo-std-aware#53, it may be possible to use target-specific features in the standard library's Cargo.toml to enable mem where necessary.

[athei]

Why do you think it would be good to not include overflow checks? I believe overflow checks are now enabled for all targets distributed for rustup except apple I think. I would think if someone is building with overflow checks, they would want them enabled everywhere, including std.

Ahh okay this is valueable information. I wasn't sure whether this is actually something intended. I believe they are also disabled for the wasm target which is the target I am seeing my error for.

It would require someone doing some investigation in to the root cause. If it is because of rust-lang/wg-cargo-std-aware#53, it may be possible to use target-specific features in the standard library's Cargo.toml to enable mem where necessary.

I applied the fix from this issue (depending on rlibc) and it also fixed the problem for me. However, I still think it is a different bug. Because the memcpy function was never missing for me. It was there (taken from the compiler_builtins project) but the memcpy function itself had missing symbols in it (core::panicking::panic) which was inserted by overflow-checks = true. The rlibc fix only works because there is no instrumented arithmetic in the rlibc version.

My bug only happens when a lot of prerequisites are fulfilled:

• target must be wasm32-unknown-unknown
• overflow checks must be enabled
• link time optimization must be enabled
• core must be recompiled using -Z build-std
• The compiler needs to insert a call to memcpy

This makes it difficult to pin down where this error was actually introduced. I therefore opened an issue just in the rust repo but I assume it never gets any traction because I need to address a more specific project.

[bjorn3]

I think it is specifically compiler-builtins which must be built without debug assertions and overflow checks. The rust repo explicitly disables it: https://github.com/rust-lang/rust/blob/f1ce0e6a00593493a12e0e3662119786c761f375/Cargo.toml#L59 Copying the entire config with which compiler-builtins should be compiled under any circumstances:

[profile.release.package.compiler_builtins]
# The compiler-builtins crate cannot reference libcore, and it's own CI will
# verify that this is the case. This requires, however, that the crate is built
# without overflow checks and debug assertions. Forcefully disable debug
# assertions and overflow checks here which should ensure that even if these
# assertions are enabled for libstd we won't enable then for compiler_builtins
# which should ensure we still link everything correctly.
debug-assertions = false
overflow-checks = false

# For compiler-builtins we always use a high number of codegen units.
# The goal here is to place every single intrinsic into its own object
# file to avoid symbol clashes with the system libgcc if possible. Note
# that this number doesn't actually produce this many object files, we
# just don't create more than this number of object files.
#
# It's a bit of a bummer that we have to pass this here, unfortunately.
# Ideally this would be specified through an env var to Cargo so Cargo
# knows how many CGUs are for this specific crate, but for now
# per-crate configuration isn't specifiable in the environment.
codegen-units = 10000

[athei]

This worked. I just copied this to my downstream Cargo.toml. I get this error though:

warning: profile package spec `compiler_builtins` in profile `release` did not match any packages

Curious that it still works.

Should this be something that is integrated into the -Z build-std feature?

[bjorn3]

Should this be something that is integrated into the -Z build-std feature?

I think so.