Fix requires_lto targets needing lto set in cargo

[Flakebi]

View all comments

Targets that set requires_lto = true were not actually using lto when compiling with cargo by default. They needed an extra lto = true in Cargo.toml to work.

Fix this by letting lto take precedence over the embed_bitcode flag when lto is required by a target.

If both these flags would be supplied by the user, an error is generated. However, this did not happen when lto was requested by the target instead of the user.

Fixes #148514
Tracking issue: #135024

[rustbot]

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

Some changes occurred in src/doc/rustc/src/platform-support

cc @Noratrieb

[jieyouxu]

Hm, not entirely sure of the implications.
@rustbot reroll

[Flakebi]

The CI/tidy complain seems like a false-positive, the test uses --target, just not through //@ compile-flags:. Maybe that lint shouldn’t run on run-make-cargo tests?
(I’m happy to make the change, just want to get confirmation that it’s ok to do that.)

[jieyouxu]

The CI/tidy complain seems like a false-positive, the test uses --target, just not through //@ compile-flags:. Maybe that lint shouldn’t run on run-make-cargo tests? (I’m happy to make the change, just want to get confirmation that it’s ok to do that.)

Yes, I probably forgot to extend the tidy exception to run-make-cargo when I split the previous run-make test suite.

[bors]

☔ The latest upstream changes (presumably #150240) made this pull request unmergeable. Please resolve the merge conflicts.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Flakebi]

Rebased to fix conflicts with #150237, no other changes

[Flakebi]

I was too early claiming that it works, I tested some more (compiling amdgpu-rs examples) and it broke some of them (somehow it ignored CARGO_BUILD_RUSTFLAGS for obj_is_bitcode and EmitObj::Bitcode completely or in parts).
With EmitObj::ObjectCode(BitcodeSection::Full) everything seems to work.
So, this is pretty much the change from before now, but checking requires_lto instead of lto != No.

[bjorn3]

What error did you get when emitting raw bitcode files?

[Flakebi]

Here are the errors I get for two changes I tested.

Either of the below changes fails to compile examples/vector_copy due to a workaround for the .kd symbol.

Click to see detailed error

$ cd examples/vector_copy
$ CARGO_BUILD_RUSTFLAGS='-Ctarget-cpu=gfx1010 -Ctarget-feature=-xnack-support' cargo +stage1 build --release
   Compiling compiler_builtins v0.1.160 (/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/compiler-builtins/compiler-builtins)
   Compiling core v0.0.0 (/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/core)
   Compiling vector_copy v0.1.0 (/rusttest/amdgpu-rs/examples/vector_copy)
warning: unknown and unstable feature specified for `-Ctarget-feature`: `xnack-support`
  |
  = note: it is still passed through to the codegen backend, but use of this feature might be unsound and the behavior of this feature can change in the future
  = help: consider filing a feature request

error: linking with `rust-lld` failed: exit status: 1
  |
  = note:  "rust-lld" "-flavor" "gnu" "--version-script=/tmp/nix-shell.cewhbS/rustc2xMArM/list" "--no-undefined-version" "<1 object files omitted>" "--as-needed" "-L" "/tmp/nix-shell.cewhbS/rustc2xMArM/raw-dylibs" "-Bdynamic" "--eh-frame-hdr" "-z" "noexecstack" "-o" "/rusttest/amdgpu-rs/examples/vector_copy/target/amdgcn-amd-amdhsa/release/deps/vector_copy.elf" "--gc-sections" "-shared" "-O1" "--strip-debug"
  = note: some arguments are omitted. use `--verbose` to show all linker arguments
  = note: rust-lld: error: version script assignment of 'global' to symbol 'kernel.kd' failed: symbol not defined


warning: `vector_copy` (lib) generated 1 warning
error: could not compile `vector_copy` (lib) due to 1 previous error; 1 warning emitted

I’m fixing this by removing the .kd symbol workaround (wrapping the symbol_export::extend_exported_symbols call in if false) and adding link-args to not remove the .kd symbol.

Change 1

Using obj_is_bitcode=true instead of requires_lto in amdgpu:

The vector_copy example compiles successfully but trying to run fails because it compiled for the wrong target (requested gfx1010 but got gfx700).

$ cd examples/vector_copy
$ CARGO_BUILD_RUSTFLAGS='-Ctarget-cpu=gfx1010 -Ctarget-feature=-xnack-support -Clink-arg=--undefined-version -Clink-arg=--no-gc-sections' cargo +stage1 build --release
$ llvm-readelf -a target/amdgcn-amd-amdhsa/release/vector_copy.elf | rg target
amdhsa.target:   amdgcn-amd-amdhsa--gfx700

The println example fails to compile somewhat similar, apparently xnack flags are not consistent between compiled objects.

Click to see detailed error

$ cd examples/println
$ CARGO_BUILD_RUSTFLAGS='-Ctarget-cpu=gfx1010 -Ctarget-feature=-xnack-support' cargo +stage1 build --release
   Compiling compiler_builtins v0.1.160 (/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/compiler-builtins/compiler-builtins)
   Compiling core v0.0.0 (/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/core)
   Compiling rustflags v0.1.6
   Compiling amdgpu-device-libs-build v0.1.0 (/rusttest/amdgpu-rs/amdgpu-device-libs-build)
   Compiling println v0.1.0 (/rusttest/amdgpu-rs/examples/println)
   Compiling alloc v0.0.0 (/rust/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/src/rust/library/alloc)
   Compiling amdgpu-device-libs v0.1.0 (/rusttest/amdgpu-rs/amdgpu-device-libs)
warning: unknown and unstable feature specified for `-Ctarget-feature`: `xnack-support`
  |
  = note: it is still passed through to the codegen backend, but use of this feature might be unsound and the behavior of this feature can change in the future
  = help: consider filing a feature request

warning: `amdgpu-device-libs` (lib) generated 1 warning
error: linking with `rust-lld` failed: exit status: 1
  |
  = note:  "rust-lld" "-flavor" "gnu" "--version-script=/tmp/nix-shell.YJcp1H/rustcRBUJWw/list" "--no-undefined-version" "<2 object files omitted>" "--as-needed" "-Bstatic" "/rusttest/amdgpu-rs/examples/println/target/amdgcn-amd-amdhsa/release/deps/{libamdgpu_device_libs-b3e559c59352d88b,liballoc-cc2fa346781d4ed5,libcore-595a027f1d29f784,libcompiler_builtins-4c3afce4ad3c6e2b}.rlib" "-L" "/tmp/nix-shell.YJcp1H/rustcRBUJWw/raw-dylibs" "-Bdynamic" "--eh-frame-hdr" "-z" "noexecstack" "-plugin-opt=O3" "-plugin-opt=mcpu=gfx1010" "-o" "/rusttest/amdgpu-rs/examples/println/target/amdgcn-amd-amdhsa/release/deps/println.elf" "--gc-sections" "-shared" "-O1" "--strip-debug" "/nix/store/4srmqmw5y2zff1h41cwz35yq64fmbb63-rocm-device-libs-19.0.0-rocm/amdgcn/bitcode/ockl.bc" "/nix/store/4srmqmw5y2zff1h41cwz35yq64fmbb63-rocm-device-libs-19.0.0-rocm/amdgcn/bitcode/oclc_isa_version_1010.bc" "/nix/store/4srmqmw5y2zff1h41cwz35yq64fmbb63-rocm-device-libs-19.0.0-rocm/amdgcn/bitcode/oclc_abi_version_600.bc" "/nix/store/4srmqmw5y2zff1h41cwz35yq64fmbb63-rocm-device-libs-19.0.0-rocm/amdgcn/bitcode/oclc_wavefrontsize64_off.bc" "/rusttest/amdgpu-rs/amdgpu-device-libs-build/util32.bc" "--undefined-version" "--no-gc-sections"
  = note: some arguments are omitted. use `--verbose` to show all linker arguments
  = note: rust-lld: warning: /rusttest/amdgpu-rs/examples/println/target/amdgcn-amd-amdhsa/release/deps/libamdgpu_device_libs-b3e559c59352d88b.rlib: archive member 'lib.rmeta' is neither ET_REL nor LLVM bitcode
          rust-lld: warning: /rusttest/amdgpu-rs/examples/println/target/amdgcn-amd-amdhsa/release/deps/liballoc-cc2fa346781d4ed5.rlib: archive member 'lib.rmeta' is neither ET_REL nor LLVM bitcode
          rust-lld: warning: /rusttest/amdgpu-rs/examples/println/target/amdgcn-amd-amdhsa/release/deps/libcore-595a027f1d29f784.rlib: archive member 'lib.rmeta' is neither ET_REL nor LLVM bitcode
          rust-lld: warning: /rusttest/amdgpu-rs/examples/println/target/amdgcn-amd-amdhsa/release/deps/libcompiler_builtins-4c3afce4ad3c6e2b.rlib: archive member 'lib.rmeta' is neither ET_REL nor LLVM bitcode
          rust-lld: error: incompatible xnack: /rusttest/amdgpu-rs/examples/println/target/amdgcn-amd-amdhsa/release/deps/println.elf.lto.liballoc-cc2fa346781d4ed5.rlib(alloc-cc2fa346781d4ed5.alloc.11bdb97dbc2d79b2-cgu.5.rcgu.o at 181598).o


warning: `println` (lib) generated 1 warning (1 duplicate)
error: could not compile `println` (lib) due to 1 previous error; 1 warning emitted

Change 2

Emitting raw bitcode files with || sess.target.requires_lto (and leaving obj_is_bitcode=false for amdgpu):

The vector_copy example compiles successfully but trying to run fails because it compiled for the wrong target (requested gfx1010 but got gfx700).

$ cd examples/vector_copy
$ CARGO_BUILD_RUSTFLAGS='-Ctarget-cpu=gfx1010 -Ctarget-feature=-xnack-support -Clink-arg=--undefined-version -Clink-arg=--no-gc-sections' cargo +stage1 build --release
$ llvm-readelf -a target/amdgcn-amd-amdhsa/release/vector_copy.elf | rg target
amdhsa.target:   amdgcn-amd-amdhsa--gfx700

examples/println works with this change.

Click to see detailed output

$ cd examples/println
$ CARGO_BUILD_RUSTFLAGS='-Ctarget-cpu=gfx1010 -Ctarget-feature=-xnack-support' cargo +stage1 build --release
$ llvm-readelf -a target/amdgcn-amd-amdhsa/release/vector_copy.elf | rg target
amdhsa.target:   amdgcn-amd-amdhsa--gfx1010
$ ../default-cpu/target/debug/default-cpu target/amdgcn-amd-amdhsa/release/println.elf
# Default output of running the example
PASSED!
Free
Finished

[bjorn3]

I see.

I’m fixing this by removing the .kd symbol workaround (wrapping the symbol_export::extend_exported_symbols call in if false) and adding link-args to not remove the .kd symbol.

This kinda makes sense. I'm guessing currently the linker sees the .kd symbol in the object file and links it, while with bitcode only files, the linker doesn't know about the .kd symbol until after LTO is done, which is after determining which symbols to export.

but trying to run fails because it compiled for the wrong target (requested gfx1010 but got gfx700).

The LTO linker plugin should know the correct target cpu thanks to -plugin-opt=mcpu=gfx1010. Can you try -Zbuild-std? Maybe the linker plugin gets confused when it sees object files with different target cpus? Without -Zbuild-std the standard library is probably compiled for gfx700.

The println example fails to compile somewhat similar, apparently xnack flags are not consistent between compiled objects.

The LLVM docs seem to suggest that xnack must be consistent between all object files, which would include the standard library.

[Flakebi]

the linker doesn't know about the .kd symbol until after LTO is done, which is after determining which symbols to export.

Yeah, there’s an open LLVM issue for that, it’s linked somewhere in the amdgpu tracking issue.

Can you try -Zbuild-std? Maybe the linker plugin gets confused when it sees object files with different target cpus? Without -Zbuild-std the standard library is probably compiled for gfx700.

The amdgpu target forces -Zbuild-std (rustc errors out if -Ctarget-cpu is not set explicitly), so this is all with build-std. (Also note that it works fine without this patch and lto=true, or with the current version of this patch.)

[Flakebi]

Friendly ping for review

[Flakebi]

@bjorn3, is this fix something you could get merged?

Would it be worth to have another rust-timer run before? (I see this is currently labelled with perf-regression but that comes from an earlier version of this PR.)

[bjorn3]

I figured @Mark-Simulacrum would approve given that they are assigned. I strongly doubt the current state will cause a perf regression.

@bors r+

[rust-bors]

📌 Commit 842c087 has been approved by bjorn3

It is now in the queue for this repository.

[Mark-Simulacrum]

For the future, I don't look at waiting-on-author PRs at all today, so I wouldn't have seen this. I'd recommend updating labels (e.g., @rustbot ready).

[Flakebi]

Ups, I didn’t see the label was pointing to me, sorry. Thanks for approving!