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

rustup: addressing a series of issues encountered while using Rust's self-contained ld.lld #314268

Merged
merged 1 commit into from
Jun 8, 2024

Conversation

DianQK
Copy link
Contributor

@DianQK DianQK commented May 24, 2024

Description of changes

Fixes #312661.

This PR primarily makes the following changes:

Determining whether to patch based on the pathname is inaccurate, which results in the loss of lld under bin/gcc-ld and attempts to patch *.rlib files.

I determine the patching method by looking up .interp, which should be simpler than the code from readelf.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label May 24, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels May 24, 2024
@DianQK
Copy link
Contributor Author

DianQK commented May 29, 2024

ping? @Mic92

@DianQK DianQK changed the title rustup: determine whether to use patchelf based on the file type rustup: addressing a series of issues encountered while using Rust's self-contained ld.lld May 31, 2024
@ofborg ofborg bot requested a review from Mic92 May 31, 2024 11:16
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels May 31, 2024
@samueltardieu
Copy link
Contributor

Result of nixpkgs-review pr 314268 run on x86_64-linux 1

3 packages built:
  • cargo-dist
  • cargo-msrv
  • rustup

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jun 7, 2024
@Mic92
Copy link
Member

Mic92 commented Jun 7, 2024

What command can I use for testing, which did not work before? i.e. in which rust version does this first break?

@Mic92
Copy link
Member

Mic92 commented Jun 7, 2024

Looks like nightly is affected according to #312661

@DianQK
Copy link
Contributor Author

DianQK commented Jun 8, 2024

What command can I use for testing, which did not work before? i.e. in which rust version does this first break?

The latest nightly rustc has switched the linker to the self-contained lld. This is an example for testing:

Cargo.toml:

[package]
name = "testlld"
version = "0.1.0"
edition = "2021"

[build-dependencies]
cc = "1.0"

src/main.rs:

extern "C" {
    fn add(a: i32, b: i32) -> i32;
}

fn main() {
    unsafe { add(1, 2) };
}

build.rs:

fn main() {
    cc::Build::new()
        .cpp(true)
        .file("foo.cpp")
        .cpp_link_stdlib("stdc++")
        .compile("foo");
}

foo.cpp:

#include <iostream>
#include <vector>

extern "C" {
    int add(int a, int b) {
          std::vector<int> numbers = {1, 2, 3, 4, 5};
          numbers.push_back(6);
          for (int number : numbers) {
            std::cout << number << " ";
          }
          std::cout << std::endl;
          return a + b;
    }
}

Then run the script:

rustup uninstall nightly-2024-05-20 
rustup install nightly-2024-05-20
cargo clean
unset LD_LIBRARY_PATH
cargo +nightly-2024-05-20 run

We will get the following error:

= note: Could not start dynamically linked executable: /home/dianqk/.rustup/toolchains/nightly-2024-05-20-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/bin/gcc-ld/ld.lld
          NixOS cannot run dynamically linked executables intended for generic
          linux environments out of the box. For more information, see:
          https://nix.dev/permalink/stub-ld
          clang: error: unable to execute command: No such file or directory
          clang: error: linker command failed due to signal (use -v to see invocation)

After using patchelf to fix it, cargo run results in the following error:

$ patchelf --set-interpreter $(patchelf --print-interpreter $(which patchelf)) ~/.rustup/toolchains/nightly-2024-05-20-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/bin/gcc-ld/ld.lld
$ unset LD_LIBRARY_PATH
$ cargo +nightly-2024-05-20 run
target/debug/testlld: error while loading shared libraries: libstdc++.so.6: cannot open shared object file: No such file or directory

@ofborg ofborg bot requested a review from Mic92 June 8, 2024 03:28
@DianQK
Copy link
Contributor Author

DianQK commented Jun 8, 2024

Reading the ELF file format on 32-bit systems is different. I don't have the environment to test it. :'(

@Mic92
Copy link
Member

Mic92 commented Jun 8, 2024

Testing 32-bit is hopefully as easy as building nix-build -A pkgsi686Linux.rustup with your example above. I will have a look.

@Mic92
Copy link
Member

Mic92 commented Jun 8, 2024

@ofborg build pkgsi686Linux.rustup

@Mic92
Copy link
Member

Mic92 commented Jun 8, 2024

Interestingly on x86_64-linux I get the following build error:

failures:

---- suite::cli_self_upd::as_rustup_setup stdout ----
inprocess: true
status: Some(0)
duration: 0.002s
stdout:
====

====

stderr:
====

====

inprocess: true
status: Some(0)
duration: 0.001s
stdout:
====

====

stderr:
====
info: auto-self-update mode set to 'disable'

====

thread 'suite::cli_self_upd::as_rustup_setup' panicked at tests/mock/clitools.rs:750:25:
Unable to run test command: Os { code: 26, kind: ExecutableFileBusy, message: "Text file busy" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    suite::cli_self_upd::as_rustup_setup

test result: FAILED. 440 passed; 1 failed; 8 ignored; 0 measured; 4 filtered out; finished in 39.94s

However this might be a different issue, i.e. maybe too many build cores on the system I am building from.

UPDATE Yes. When building with less cores, it becomes better.

@DianQK
Copy link
Contributor Author

DianQK commented Jun 8, 2024

However this might be a different issue, i.e. maybe too many build cores on the system I am building from.

UPDATE Yes. When building with less cores, it becomes better.

I can't reproduce it on my 16-core, 32-thread system. Could you test the upstream repository? I'm curious whether the issue lies with cargo test or the test case itself.

You can test it with the following script:

git clone https://github.com/rust-lang/rustup.git --branch 1.26.0 --depth 1
cd rustup
rustup install 1.77.2
cargo +1.77.2 test

@Mic92
Copy link
Member

Mic92 commented Jun 8, 2024

However this might be a different issue, i.e. maybe too many build cores on the system I am building from.
UPDATE Yes. When building with less cores, it becomes better.

I can't reproduce it on my 16-core, 32-thread system. Could you test the upstream repository? I'm curious whether the issue lies with cargo test or the test case itself.

You can test it with the following script:

git clone https://github.com/rust-lang/rustup.git --branch 1.26.0 --depth 1
cd rustup
rustup install 1.77.2
cargo +1.77.2 test

I don't think we need to fix this in this PR. It's a AMD EPYC 7713P 64-Core, I often run into the problem that build systems and test suite are not exercised with this core count.

@DianQK
Copy link
Contributor Author

DianQK commented Jun 8, 2024

Testing 32-bit is hopefully as easy as building nix-build -A pkgsi686Linux.rustup with your example above. I will have a look.

I mainly want to test whether rustup correctly fixes the files for i686 during the installation of the toolchain.
Ah, I can use the script to test it:

$ env RUSTUP_DEBUG=true rustup toolchain install nightly-i686-unknown-linux-gnu --force-non-host

debug: patching "/home/dianqk/.rustup/toolchains/nightly-i686-unknown-linux-gnu/lib/rustlib/i686-unknown-linux-gnu/lib/libstd-4ebf06893886ecff.so" using patchelf
...
debug: patching "/home/dianqk/.rustup/toolchains/nightly-i686-unknown-linux-gnu/bin/rustc" using patchelf
debug: patching "/home/dianqk/.rustup/toolchains/nightly-i686-unknown-linux-gnu/bin/rustdoc" using patchelf
debug: patching "/home/dianqk/.rustup/toolchains/nightly-i686-unknown-linux-gnu/lib/librustc_driver-370689965a44b509.so" using patchelf
debug: patching "/home/dianqk/.rustup/toolchains/nightly-i686-unknown-linux-gnu/lib/libstd-4ebf06893886ecff.so" using patchelf
debug: patching "/home/dianqk/.rustup/toolchains/nightly-i686-unknown-linux-gnu/lib/rustlib/i686-unknown-linux-gnu/bin/gcc-ld/ld.lld" using patchelf
debug: patching "/home/dianqk/.rustup/toolchains/nightly-i686-unknown-linux-gnu/lib/rustlib/i686-unknown-linux-gnu/bin/gcc-ld/ld64.lld" using patchelf
debug: patching "/home/dianqk/.rustup/toolchains/nightly-i686-unknown-linux-gnu/lib/rustlib/i686-unknown-linux-gnu/bin/gcc-ld/lld-link" using patchelf
debug: patching "/home/dianqk/.rustup/toolchains/nightly-i686-unknown-linux-gnu/lib/rustlib/i686-unknown-linux-gnu/bin/gcc-ld/wasm-ld" using patchelf
debug: patching "/home/dianqk/.rustup/toolchains/nightly-i686-unknown-linux-gnu/lib/rustlib/i686-unknown-linux-gnu/bin/rust-lld" using patchelf
debug: patching "/home/dianqk/.rustup/toolchains/nightly-i686-unknown-linux-gnu/libexec/rust-analyzer-proc-macro-srv" using patchelf

@DianQK
Copy link
Contributor Author

DianQK commented Jun 8, 2024

I pushed a commit for this patch file: DianQK/rustup@f14f5f2, which should help in reviewing the changes.

@Mic92
Copy link
Member

Mic92 commented Jun 8, 2024

Looks like the 32bit rustup build is broken anyway... So let's focus on 32-bit rust toolchains instead.

@Mic92
Copy link
Member

Mic92 commented Jun 8, 2024

So far the 32-bit headers still seem to be corrupted:

% file ./rustc
./rustc: ELF 32-bit LSB pie executable, Intel 80386, version 1 (SYSV), dynamically linked, interpreter /nix/store/k7zgvzp2r31zkg9xqgjim7mbknryv6bs-glibc-2.39-52/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, with debug_info, not stripped
% ./rustc
zsh: accessing a corrupted shared library: ./rustc

I now actually doubt, this ever worked because we are trying to use the 64-bit link-loader for a 32-bit executable.

@Mic92
Copy link
Member

Mic92 commented Jun 8, 2024

What seems to work is rustup target add i686-unknown-linux-gnu as opposed to a 32-bit rustc.

with import <nixpkgs> {};
pkgsi686Linux.stdenv.mkDerivation {
  name = "hello";
}
% file ./target/i686-unknown-linux-gnu/debug/testlld
./target/i686-unknown-linux-gnu/debug/testlld: ELF 32-bit LSB pie executable, Intel 80386, version 1 (SYSV), dynamically linked, interpreter /nix/store/756q2acnjgi6ym709gw1f3jp0ciqwib1-glibc-2.39-52/lib/ld-linux.so.2, for GNU/Linux 3.10.0, with debug_info, not stripped

%  ./target/i686-unknown-linux-gnu/debug/testlld
1 2 3 4 5 6

env = lib.optionalAttrs (pname == "rustup") {
inherit (stdenv.cc.bintools) expandResponseParams shell suffixSalt wrapperName coreutils_bin;
hardening_unsupported_flags = "";
};
Copy link
Member

@Mic92 Mic92 Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could make a better regression test here, given the growing complexity of machinery that gets added here? If we cannot make it pure, even a self-contained test script would be nice i.e. based on the example project that you already provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we do need a test case. I plan to add an internal test case for the rustup project in a subsequent PR.

@DianQK
Copy link
Contributor Author

DianQK commented Jun 8, 2024

So far the 32-bit headers still seem to be corrupted:

% file ./rustc
./rustc: ELF 32-bit LSB pie executable, Intel 80386, version 1 (SYSV), dynamically linked, interpreter /nix/store/k7zgvzp2r31zkg9xqgjim7mbknryv6bs-glibc-2.39-52/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, with debug_info, not stripped
% ./rustc
zsh: accessing a corrupted shared library: ./rustc

I now actually doubt, this ever worked because we are trying to use the 64-bit link-loader for a 32-bit executable.

Is rustup used to install rustc on the i686 platform? It is not common practice to use rustup on the x86-64 platform to install the i686 toolchain. I think we only need to ensure that rustup on i686 can successfully install and run rustc.

@DianQK
Copy link
Contributor Author

DianQK commented Jun 8, 2024

What seems to work is rustup target add i686-unknown-linux-gnu as opposed to a 32-bit rustc.

with import <nixpkgs> {};
pkgsi686Linux.stdenv.mkDerivation {
  name = "hello";
}
% file ./target/i686-unknown-linux-gnu/debug/testlld
./target/i686-unknown-linux-gnu/debug/testlld: ELF 32-bit LSB pie executable, Intel 80386, version 1 (SYSV), dynamically linked, interpreter /nix/store/756q2acnjgi6ym709gw1f3jp0ciqwib1-glibc-2.39-52/lib/ld-linux.so.2, for GNU/Linux 3.10.0, with debug_info, not stripped

%  ./target/i686-unknown-linux-gnu/debug/testlld
1 2 3 4 5 6

I'm not quite sure what you're expressing here. Are you referring to results entirely on i686?

@DianQK
Copy link
Contributor Author

DianQK commented Jun 8, 2024

So far the 32-bit headers still seem to be corrupted:

% file ./rustc
./rustc: ELF 32-bit LSB pie executable, Intel 80386, version 1 (SYSV), dynamically linked, interpreter /nix/store/k7zgvzp2r31zkg9xqgjim7mbknryv6bs-glibc-2.39-52/lib/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, with debug_info, not stripped
% ./rustc
zsh: accessing a corrupted shared library: ./rustc

I now actually doubt, this ever worked because we are trying to use the 64-bit link-loader for a 32-bit executable.

I can use rustup of the master branch or the PR to build pkgsi686Linux.rustup on x86-64, and then use rustup to install rustc for x86-64 to reproduce a similar issue.

@Mic92
Copy link
Member

Mic92 commented Jun 8, 2024

I now actually doubt, this ever worked because we are trying to use the 64-bit link-loader for a 32-bit executable.

I can use rustup of the master branch or the PR to build pkgsi686Linux.rustup on x86-64, and then use rustup to install rustc for x86-64 to reproduce a similar issue.

If you can fix it, it's nice, but it would be also not a new regression because it's broken just now.

@Mic92 Mic92 merged commit 3d59539 into NixOS:master Jun 8, 2024
26 of 28 checks passed
@Mic92
Copy link
Member

Mic92 commented Jun 8, 2024

What seems to work is rustup target add i686-unknown-linux-gnu as opposed to a 32-bit rustc.

with import <nixpkgs> {};
pkgsi686Linux.stdenv.mkDerivation {
  name = "hello";
}
% file ./target/i686-unknown-linux-gnu/debug/testlld
./target/i686-unknown-linux-gnu/debug/testlld: ELF 32-bit LSB pie executable, Intel 80386, version 1 (SYSV), dynamically linked, interpreter /nix/store/756q2acnjgi6ym709gw1f3jp0ciqwib1-glibc-2.39-52/lib/ld-linux.so.2, for GNU/Linux 3.10.0, with debug_info, not stripped

%  ./target/i686-unknown-linux-gnu/debug/testlld
1 2 3 4 5 6

I'm not quite sure what you're expressing here. Are you referring to results entirely on i686?

I thought, this would also require patchelf, but actually it doesn't because everything is compiled afterwards.

@DianQK
Copy link
Contributor Author

DianQK commented Jun 8, 2024

I found that doCheck fails on CI. Would this affect the i686 build after merging? Perhaps the host for this CI is i686?

@Mic92
Copy link
Member

Mic92 commented Jun 8, 2024

Only pkgsi686Linux.rustup failed but we also have rustup, rustup.passthru.tests on x86_64-linux Success. However I checked and pkgsi686Linux.rustup also doesn't work on master. We are slowly phasing out 32-bit to be honest. We do not even have a NixOS installer for it anymore: https://nixos.org/download/

@DianQK
Copy link
Contributor Author

DianQK commented Jun 8, 2024

Only pkgsi686Linux.rustup failed but we also have rustup, rustup.passthru.tests on x86_64-linux Success. However I checked and pkgsi686Linux.rustup also doesn't work on master. We are slowly phasing out 32-bit to be honest. We do not even have a NixOS installer for it anymore: https://nixos.org/download/

This seems reasonable as well. I'm not currently considering dealing with this particular cross-compilation, but I'll try to add a test case.

BTW, we should backport this to 24.05. The backport note could be: fixed rustc's self-contained lld not functioning.

@DianQK DianQK deleted the rustup-patchelf branch June 8, 2024 13:18
@oxalica
Copy link
Contributor

oxalica commented Jun 8, 2024

Should we use nix-ld instead of patchelf in rustup for this? I've seen some people (from nixos_zhcn) complaining about rustup's toolchains break constantly on each system updates, possibly because it's not added to GC roots? I'm not sure if this PR is a good place to discuss it, but this PR does make patchelf in rustup more and more complicated.

@DianQK
Copy link
Contributor Author

DianQK commented Jun 8, 2024

Should we use nix-ld instead of patchelf in rustup for this?

I think we should remain the behavior of rustc itself. rust-lld might perform some proprietary operations, and silently replacing it could cause issues. Additionally, rustc offers the option to use an external ld.

I've seen some people (from nixos_zhcn) complaining about rustup's toolchains break constantly on each system updates, possibly because it's not added to GC roots?

I think we could add a sub-command like rustup repatch.

I'm not sure if this PR is a good place to discuss it, but this PR does make patchelf in rustup more and more complicated.

I don't have any good ideas here.

@Mic92
Copy link
Member

Mic92 commented Jun 10, 2024

Should we use nix-ld instead of patchelf in rustup for this? I've seen some people (from nixos_zhcn) complaining about rustup's toolchains break constantly on each system updates, possibly because it's not added to GC roots? I'm not sure if this PR is a good place to discuss it, but this PR does make patchelf in rustup more and more complicated.

We could also add a gcroot into the rustup directory.

@DianQK
Copy link
Contributor Author

DianQK commented Jun 10, 2024

Writing such test cases on rustup isn't easy. I read through some test code, and it seems like rustup's internal tests only focus on rustup's own behavior. I can only add this test in nixpkgs, but I'm not familiar with nix. Perhaps we should create an additional package or add a test script directly in the postInstall.

@DianQK
Copy link
Contributor Author

DianQK commented Jun 10, 2024

Should we use nix-ld instead of patchelf in rustup for this? I've seen some people (from nixos_zhcn) complaining about rustup's toolchains break constantly on each system updates, possibly because it's not added to GC roots? I'm not sure if this PR is a good place to discuss it, but this PR does make patchelf in rustup more and more complicated.

We could also add a gcroot into the rustup directory.

I think this might confuse many people as to why certain stores can't be cleaned up.
I suspect it's important to maintain reproducibility for rustup. I think we can create a state directory in the .rustup folder and patch all files whenever there's a change in this state. This seems to make it possible for us to only need to create a wrapper.

@Mic92
Copy link
Member

Mic92 commented Jun 10, 2024

Writing such test cases on rustup isn't easy. I read through some test code, and it seems like rustup's internal tests only focus on rustup's own behavior. I can only add this test in nixpkgs, but I'm not familiar with nix. Perhaps we should create an additional package or add a test script directly in the postInstall.

Maybe write a test that just uses nix for dependencies but has internet access. Than we can at least test manually when we change things.

@DianQK
Copy link
Contributor Author

DianQK commented Jun 11, 2024

@Mic92 @NickCao
Could you help me add a backport label? rustc will switch the default linker to lld in the next stable version. rustc has switched the default linker to lld in the nightly channel.
We can continue to improve the garbage collection issue in rustup.

@NickCao NickCao added the backport release-24.05 Backport PR automatically label Jun 12, 2024
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person 12. first-time contribution This PR is the author's first one; please be gentle! backport release-24.05 Backport PR automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Latest rustup update version breaks compilation
7 participants