search

rust-lang / wg-cargo-std-aware

Repo for working on "std aware cargo"
133 stars 8 forks source link

Cannot test bug fixes of dependencies of `libcore`/`libstd` on targets where `-Zbuild-std` is required. #77

Closed cr1901 closed 2 years ago

cr1901 commented 2 years ago

Problem

As of last week, CI for an msp430 firmware I test against nightly began failing. compiler-builtins, which is a dependency of libcore/libstd was updated in https://github.com/rust-lang/rust/commit/88f1bf73ee8466d07b1e38755de53a70fc292e20, and broke a few platforms, including msp430. I figured out the problem, opened an issue, and have attempted to create a local fix that I wish to test.

Unfortunately, I am not able to test locally, because it appears that there's no way to override Rust's internal Cargo.lock for testing bug fixes to compiler-builtins. Targets which provide libstd/libcore don't have this problem because providing libstd/libcore means Rust thinks those dependencies are always up to date. My fix probably works, but I'm not comfortable submitting a PR until I have tested locally. Is it possible to provide provisions for overriding Rust's Cargo.lock and internal cargo manifests for the purpose of testing bug fixes?

I've elaborated on the problem in another issue. I'm reproducing it here for convenience:

Issue #441

I have a fix locally for #441. Unfortunately I can't get -Zbuild-std=core to choose the correct version of compiler_builtins with my changes, so that I can test... compilation of compiler_builtins with my changes.

patching compiler-builtins

diff --git a/Cargo.toml b/Cargo.toml
index d4348fb..61cfab6 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -80,3 +80,6 @@ panic = 'abort'

 [profile.dev]
 panic = 'abort'
+
+[patch.crates-io]
+compiler_builtins = { version="0.1.52", path = '.' }

This patch appears to do nothing (although I may have specified it incorrectly, tbf):

william@xubuntu-dtrain:~/Projects/toolchains/compiler-builtins$ cargo +msp430-fix build --target=msp430-none-elf -Zbuild-std=core
   Compiling compiler_builtins v0.1.52
   Compiling compiler_builtins v0.1.53 (/home/william/Projects/toolchains/compiler-builtins)
warning: an associated function with this name may be added to the standard library in the future
 --> src/float/pow.rs:8:19
  |
8 |     let mut pow = i32::abs_diff(b, 0);
  |                   ^^^^^^^^^^^^^
  |
  = note: `#[warn(unstable_name_collisions)]` on by default
  = warning: once this associated item is added to the standard library, the ambiguity may cause an error or change in behavior!
  = note: for more information, see issue #48919 <https://github.com/rust-lang/rust/issues/48919>
  = help: call with fully qualified syntax `Int::abs_diff(...)` to keep using the current
method
  = help: add `#![feature(int_abs_diff)]` to the crate attributes to enable `num::<impl i32>::abs_diff`

LLVM ERROR: Cannot select: t14: i16,ch = AtomicLoad<(load unordered (s16) from %ir.188)> t13:1, t13, /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/compiler_builtins-0.1.52/src/mem/impls.rs:65:29 @[ /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/compiler_builtins-0.1.52/src/mem/impls.rs:109:13 @[ /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/compiler_builtins-0.1.52/src/mem/mod.rs:25:5 ] ]
  t13: i16,ch = load<(dereferenceable load (s16) from %ir.28)> t12, FrameIndex:i16<24>, undef:i16, /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/compiler_builtins-0.1.52/src/mem/impls.rs:65:69 @[ /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/compiler_builtins-0.1.52/src/mem/impls.rs:109:13 @[ /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/compiler_builtins-0.1.52/src/mem/mod.rs:25:5 ] ]
    t11: i16 = FrameIndex<24>
    t5: i16 = undef
In function: memcpy
error: could not compile `compiler_builtins`
warning: build failed, waiting for other jobs to finish...
LLVM ERROR: Cannot select: t14: i16,ch = AtomicLoad<(load unordered (s16) from %ir.188)> t13:1, t13, src/mem/impls.rs:65:29 @[ src/mem/impls.rs:109:13 @[ src/mem/mod.rs:25:5 ] ]
  t13: i16,ch = load<(dereferenceable load (s16) from %ir.28)> t12, FrameIndex:i16<24>, undef:i16, src/mem/impls.rs:65:69 @[ src/mem/impls.rs:109:13 @[ src/mem/mod.rs:25:5 ] ]
    t11: i16 = FrameIndex<24>
    t5: i16 = undef
In function: memcpy
warning: `compiler_builtins` (lib) generated 1 warning
error: build failed
william@xubuntu-dtrain:~/Projects/toolchains/compiler-builtins$

patching rust

diff --git a/Cargo.lock b/Cargo.lock
index 51ed441d0db..74623c9d602 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -676,16 +676,6 @@ dependencies = [
  "libc",
 ]

-[[package]]
-name = "compiler_builtins"
-version = "0.1.52"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "b6591c2442ee984e2b264638a8b5e7ae44fd47b32d28e3a08e2e9c3cdb0c2fb0"
-dependencies = [
- "cc",
- "rustc-std-workspace-core",
-]
-
 [[package]]
 name = "compiletest"
 version = "0.0.0"
diff --git a/library/backtrace b/library/backtrace
--- a/library/backtrace
+++ b/library/backtrace
@@ -1 +1 @@
-Subproject commit b02ed04a7e915659eea6fb1607df469b84a30638
+Subproject commit b02ed04a7e915659eea6fb1607df469b84a30638-dirty
diff --git a/library/std/Cargo.toml b/library/std/Cargo.toml
index 8f43e902a87..70147631270 100644
--- a/library/std/Cargo.toml
+++ b/library/std/Cargo.toml
@@ -16,7 +16,7 @@ panic_unwind = { path = "../panic_unwind", optional = true }
 panic_abort = { path = "../panic_abort" }
 core = { path = "../core" }
 libc = { version = "0.2.106", default-features = false, features = ['rustc-dep-of-std'] }
-compiler_builtins = { version = "0.1.52" }
+compiler_builtins = { path="/home/william/Projects/toolchains/compiler-builtins", version = "0.1.53" }
 profiler_builtins = { path = "../profiler_builtins", optional = true }
 unwind = { path = "../unwind" }
 hashbrown = { version = "0.11", default-features = false, features = ['rustc-dep-of-std'] }

I genuinely don't understand the resulting error. You say 0.1.53 meets the requirements to be chosen and simultaneously isn't acceptable. So which is it?:

william@xubuntu-dtrain:~/Projects/toolchains/compiler-builtins$ cargo +msp430-fix build --target=msp430-none-elf -Zbuild-std=core
    Updating crates.io index
error: failed to select a version for `compiler_builtins`.
    ... required by package `alloc v0.0.0 (/home/william/Projects/toolchains/build-llvm-toolchain/build-rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/alloc)`
    ... which satisfies path dependency `alloc` of package `panic_abort v0.0.0 (/home/william/Projects/toolchains/build-llvm-toolchain/build-rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/panic_abort)`
    ... which satisfies path dependency `panic_abort` of package `std v0.0.0 (/home/william/Projects/toolchains/build-llvm-toolchain/build-rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/std)`
    ... which satisfies path dependency `std` of package `proc_macro v0.0.0 (/home/william/Projects/toolchains/build-llvm-toolchain/build-rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/proc_macro)`
    ... which satisfies path dependency `proc_macro` (locked to 0.0.0) of package `test v0.0.0 (/home/william/Projects/toolchains/build-llvm-toolchain/build-rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/test)`
versions that meet the requirements `^0.1.40` are: 0.1.53, 0.1.52, 0.1.51, 0.1.50, 0.1.49, 0.1.48, 0.1.47, 0.1.46, 0.1.45, 0.1.44, 0.1.43, 0.1.42, 0.1.41, 0.1.40

the package `compiler_builtins` links to the native library `compiler-rt`, but it conflicts with a previous package which links to `compiler-rt` as well:
package `compiler_builtins v0.1.53 (/home/william/Projects/toolchains/compiler-builtins)`
    ... which satisfies path dependency `compiler_builtins` of package `std v0.0.0 (/home/william/Projects/toolchains/build-llvm-toolchain/build-rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/std)`
    ... which satisfies path dependency `std` of package `proc_macro v0.0.0 (/home/william/Projects/toolchains/build-llvm-toolchain/build-rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/proc_macro)`
    ... which satisfies path dependency `proc_macro` (locked to 0.0.0) of package `test v0.0.0 (/home/william/Projects/toolchains/build-llvm-toolchain/build-rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/test)`
Only one package in the dependency graph may specify the same links value. This helps ensure that only one copy of a native library is linked in the final binary. Try to adjust your dependencies so that only one package uses the links ='compiler_builtins' value. For more information, see https://doc.rust-lang.org/cargo/reference/resolver.html#links.

failed to select a version for `compiler_builtins` which could resolve this conflict

Other Remarks

In addition, _libcore doesn't list compiler_builtins as a dependency, so why is -Zbuild-std=core trying to build it anyway?_

12101111 commented 2 years ago
  1. Editing Cargo.toml won't work as all dependencies are resolved from Cargo.lock. [patch.*] also don't work because of this. This is tracking in https://github.com/rust-lang/wg-cargo-std-aware/issues/7 . See also https://github.com/rust-lang/wg-cargo-std-aware/issues/61
  2. You should follow rustc dev guide to test your change
  3. rustc will inject core and compiler_builtins to dependency list for no_std crate: https://github.com/rust-lang/rust/blob/84826fec957aa17b0e068775c1c5574f707d43b0/compiler/rustc_builtin_macros/src/standard_library_imports.rs#L25
ehuss commented 2 years ago

Yea, it is not currently possible to customize the source. In addition to what @12101111 mentioned, I would personally just build rustc locally with a patch in Cargo.toml that points to your local fork of compiler_builtins. You should be able to test with that compiler.

Closing as this is tracked in other issues.

cr1901 commented 2 years ago

I would personally just build rustc locally with a patch in Cargo.toml that points to your local fork of compiler_builtins. You should be able to test with that compiler.

@ehuss In the "patching rust" section, this is what I attempted to do, but got a cryptic error message saying that "0.1.53 was simultaneously not selected as a version and links to a native library". If you want me to patch Rust locally, fine. But Rust/Cargo itself seems to not want me to patch it. How do I get past this error? What is wrong with my patch as shown in the patching Rust section. Does Cargo.lock even allow path dependencies? By deleting the compiler_builtins entry in Cargo.lock, I was hoping cargo would "do the right thing" and replace the dep with my locally copy. But I got this error instead:

william@xubuntu-dtrain:~/Projects/toolchains/compiler-builtins$ cargo +msp430-fix build --target=msp430-none-elf -Zbuild-std=core
    Updating crates.io index
error: failed to select a version for `compiler_builtins`.
    ... required by package `alloc v0.0.0 (/home/william/Projects/toolchains/build-llvm-toolchain/build-rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/alloc)`
    ... which satisfies path dependency `alloc` of package `panic_abort v0.0.0 (/home/william/Projects/toolchains/build-llvm-toolchain/build-rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/panic_abort)`
    ... which satisfies path dependency `panic_abort` of package `std v0.0.0 (/home/william/Projects/toolchains/build-llvm-toolchain/build-rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/std)`
    ... which satisfies path dependency `std` of package `proc_macro v0.0.0 (/home/william/Projects/toolchains/build-llvm-toolchain/build-rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/proc_macro)`
    ... which satisfies path dependency `proc_macro` (locked to 0.0.0) of package `test v0.0.0 (/home/william/Projects/toolchains/build-llvm-toolchain/build-rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/test)`
versions that meet the requirements `^0.1.40` are: 0.1.53, 0.1.52, 0.1.51, 0.1.50, 0.1.49, 0.1.48, 0.1.47, 0.1.46, 0.1.45, 0.1.44, 0.1.43, 0.1.42, 0.1.41, 0.1.40

the package `compiler_builtins` links to the native library `compiler-rt`, but it conflicts with a previous package which links to `compiler-rt` as well:
package `compiler_builtins v0.1.53 (/home/william/Projects/toolchains/compiler-builtins)`
    ... which satisfies path dependency `compiler_builtins` of package `std v0.0.0 (/home/william/Projects/toolchains/build-llvm-toolchain/build-rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/std)`
    ... which satisfies path dependency `std` of package `proc_macro v0.0.0 (/home/william/Projects/toolchains/build-llvm-toolchain/build-rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/proc_macro)`
    ... which satisfies path dependency `proc_macro` (locked to 0.0.0) of package `test v0.0.0 (/home/william/Projects/toolchains/build-llvm-toolchain/build-rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/test)`
Only one package in the dependency graph may specify the same links value. This helps ensure that only one copy of a native library is linked in the final binary. Try to adjust your dependencies so that only one package uses the links ='compiler_builtins' value. For more information, see https://doc.rust-lang.org/cargo/reference/resolver.html#links.

failed to select a version for `compiler_builtins` which could resolve this conflict
ehuss commented 2 years ago

You should be able to edit the root Cargo.toml and add something like the following to the [patch.crates-io] table:

[patch.crates-io]
compiler_builtins = { path = "../compiler-builtins" }

Then run cargo update -p compiler_builtins

cr1901 commented 2 years ago

You should be able to edit the root Cargo.toml and add something like the following to the [patch.crates-io] table:

This works for testing libcore/compiler-builtins, and I've confirmed my patch works at least w/ libcore.

Now, how do I tell cargo and/or the injection logic (local patch is fine) to use my local copy of compiler_builtins ~, so cargo doesn't try building two copies of compiler_builtins 0.1.53~? EDIT: I can't actually tell if two copies of compiler_builtins- one local, one on crates.io, are being compiled.

AT2XT Still Fails

Honestly, I'm not sure why the above patch works; core does not depend on compiler-builtins1. Is the compiler_builtins dep injected then? Regardless, cargo is honoring my patch.crates-io table in the Rust's own top-level Cargo.toml. But when I try to do the same for AT2XT, I get a warning:

diff --git a/Cargo.toml b/Cargo.toml
index bcb79e1..67fa3b1 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -52,3 +52,6 @@ lto = "fat"
 codegen-units = 1
 opt-level = "s"
 lto = "fat"
+
+[patch.crates-io]
+compiler_builtins = { version="0.1.53", path = "/home/william/Projects/toolchains/compiler-builtins" }
william@xubuntu-dtrain:~/Projects/embedded/msp430/AT2XT$ cargo update
    Updating crates.io index
warning: Patch `compiler_builtins v0.1.53 (/home/william/Projects/toolchains/compiler-builtins)` was not used in the crate graph.
Check that the patched package version and available features are compatible
with the dependency requirements. If the patch has a different version from
what is locked in the Cargo.lock file, run `cargo update` to use the new
version. This may also occur with an optional dependency that is not enabled.

And ultimately, when I try compiling AT2XT using my fresh new Rust with the patched Cargo.toml, I get a compiler error. I can't tell whether this patch made progress compared to a lack of patch because cargo doesn't appear to have --keep-going mode. It looks like we made progress compared to CI, however...

warning: Patch `compiler_builtins v0.1.53 (/home/william/Projects/toolchains/compiler-builtins)` was not used in the crate graph.
Check that the patched package version and available features are compatible
with the dependency requirements. If the patch has a different version from
what is locked in the Cargo.lock file, run `cargo update` to use the new
version. This may also occur with an optional dependency that is not enabled.
    Updating crates.io index
   Compiling compiler_builtins v0.1.53
   Compiling autocfg v0.1.7
   Compiling core v0.0.0 (/home/william/Projects/toolchains/build-llvm-toolchain/build-rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/core)
   Compiling rand_core v0.4.2
   Compiling semver-parser v0.7.0
   Compiling proc-macro2 v0.4.30
   Compiling libc v0.2.108
   Compiling unicode-xid v0.1.0
   Compiling syn v0.15.44
   Compiling msp430-rt v0.2.5
   Compiling msp430g2211 v0.2.1
   Compiling rand_core v0.3.1
   Compiling rand_jitter v0.1.4
   Compiling semver v0.9.0
   Compiling rand_isaac v0.1.1
   Compiling rand_xorshift v0.1.1
   Compiling rand_hc v0.1.0
   Compiling rand_pcg v0.1.2
   Compiling rand_chacha v0.1.1
   Compiling rand v0.6.5
   Compiling rustc_version v0.2.3
   Compiling bare-metal v0.2.5
   Compiling quote v0.6.13
   Compiling msp430-rt-macros v0.2.4
   Compiling rustc-std-workspace-core v1.99.0 (/home/william/Projects/toolchains/build-llvm-toolchain/build-rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/rustc-std-workspace-core)
   Compiling vcell v0.1.3
   Compiling msp430-atomic v0.1.3
   Compiling bitflags v1.3.2
   Compiling bit_reverse v0.1.8
   Compiling once_cell v1.8.0
   Compiling msp430 v0.2.2
   Compiling panic-msp430 v0.2.0
LLVM ERROR: Cannot select: t3: i16,ch = AtomicLoad<(load unordered (s16) from %ir.49)> t0, t2
  t2: i16,ch = CopyFromReg t0, Register:i16 %19
    t1: i16 = Register %19
In function: memcpy
error: could not compile `compiler_builtins`
warning: build failed, waiting for other jobs to finish...
error: build failed

_Why is my patch being honored when compiler_builtins is being compiled as part of the Rust compiler build, but not when I'm trying to build AT2XT_?

Addendum

  1. Interestingly enough, the library/core target also builds alloc, which does depend on compiler_builtins. So does the injection still happen? compiler_builtins is in the crate graph, but it's not at the top-level... wow this is confusin!
william@xubuntu-dtrain:~/Projects/toolchains/build-llvm-toolchain/build-rust$ RUSTFLAGS="-C opt-level=s" RUSTC_WRAPPER=/home/william/.cargo/bin/sccache LD_LIBRARY_PATH=/home/william/Projects/toolchains/build-llvm-toolchain/build-llvm/lib/ /home/william/Projects/toolchains/rust/x.py build --config config.toml --target msp430-none-elf --stage 1 library/core
Updating only changed submodules
Submodules updated in 0.01 seconds
    Finished dev [unoptimized] target(s) in 0.11s
Building stage0 std artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.10s
Copying stage0 std from stage0 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
Building stage0 compiler artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.15s
Copying stage0 rustc from stage0 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
Assembling stage1 compiler (x86_64-unknown-linux-gnu)
Building stage1 std artifacts (x86_64-unknown-linux-gnu -> msp430-none-elf)
   Compiling core v0.0.0 (/home/william/Projects/toolchains/rust/library/core)
   Compiling compiler_builtins v0.1.53 (/home/william/Projects/toolchains/compiler-builtins)
   Compiling rustc-std-workspace-core v1.99.0 (/home/william/Projects/toolchains/rust/library/rustc-std-workspace-core)
   Compiling alloc v0.0.0 (/home/william/Projects/toolchains/rust/library/alloc)
    Finished release [optimized] target(s) in 2.87s
Copying stage1 std from stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / msp430-none-elf)
Build completed successfully in 0:00:03
ehuss commented 2 years ago

Oh, I think I misled you a little, as I was thinking of just building your target with x.py instead of with -Z build-std. The patches don't carry over that way for -Z build-std. I don't think it is possible to use a patch with -Z build-std. I was just thinking to test your changes, you can patch it, and run x.py to build the toolchain for your msp430-none-elf target, and then use that without -Z build-std to test inside your project. It won't be exactly the same as -Z build-std, though, but it should be close.

cr1901 commented 2 years ago

and then use that without -Z build-std to test inside your project.

This works. Because this threw me off, I'll add this as a note to future-me (it's prob written down somewhere already):

If you want to test changes to libstd/libcore for a target besides your host, your invocation to build the compiler must look something like this1:

x.py build --config config.toml --target x86_64-unknown-linux-gnu,msp430-none-elf --stage 1 library/core

You must pass a comma-separated list of targets to get all the libstds and libcores for your desired target. If you pass targets one per invocation, such as:

x.py build --config config.toml --target x86_64-unknown-linux-gnu --stage 1 library/core
x.py build --config config.toml --target msp430-none-elf  --stage 1 library/core

the x.py build will "delete" (it's cached elsewhere) the previously-built target's libstd/libcore while building the current target's libstd/libcore. This will lead to cryptic "can't find std/core" errors

william@xubuntu-dtrain:~/Projects/embedded/msp430/AT2XT$ LD_LIBRARY_PATH=/home/william/Projects/toolchains/build-llvm-toolchain/build-llvm/lib/ cargo +msp430-fix build --release --target=msp430-none-elf
warning: Patch `compiler_builtins v0.1.53 (/home/william/Projects/toolchains/compiler-builtins)` was not used in the crate graph.
Check that the patched package version and available features are compatible
with the dependency requirements. If the patch has a different version from
what is locked in the Cargo.lock file, run `cargo update` to use the new
version. This may also occur with an optional dependency that is not enabled.
   Compiling autocfg v0.1.7
   Compiling rand_core v0.4.2
   Compiling semver-parser v0.7.0
   Compiling proc-macro2 v0.4.30
   Compiling libc v0.2.108
   Compiling unicode-xid v0.1.0
   Compiling syn v0.15.44
   Compiling msp430-rt v0.2.5
error[E0463]: can't find crate for `std`

error[E0463]: can't find crate for `core`

For more information about this error, try `rustc --explain E0463`.
error: could not compile `syn` due to previous error
warning: build failed, waiting for other jobs to finish...
error: build failed
  1. I still don't understand why passing library/core builds std and friends for x86_64, and alloc for msp430, but whatever. That's a problem for future-me.