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

bootstrap cannot check library/std for windows from macOS (and possibly others) #101172

Closed
thomcc opened this issue Aug 29, 2022 · 15 comments · Fixed by #101833 or #119556
Closed

bootstrap cannot check library/std for windows from macOS (and possibly others) #101172

thomcc opened this issue Aug 29, 2022 · 15 comments · Fixed by #101833 or #119556
Labels
A-cross Area: Cross compilation C-bug Category: This is a bug. E-help-wanted Call for participation: Help is requested to fix this issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@thomcc
Copy link
Member

thomcc commented Aug 29, 2022

I often write patches for std::sys::windows while not using windows. It would be nice if ./x.py check --target x86_64-pc-windows-msvc library/std worked, but instead it does not -- I get an error like https://gist.github.com/thomcc/7c889b3a1306922f7eda28f45f1d304e (this is from a build where aarch64-apple-darwin is the host).

As a result, to check changes to std::sys::windows while not on windows, I'm forced to a full stage1 build, and then in some other workspace run cargo +stage1 check -Zbuild-std --target x86_64-pc-windows-msvc (where +stage1 is a linked toolchain). This works, but is tedious/time-consuming.

CC @jyn514 who suggested I file this issue, and suggested that it "looks like we're handling the linker arguments wrong when cross compiling".

@thomcc thomcc added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) O-windows Operating system: Windows labels Aug 29, 2022
@jyn514 jyn514 added A-cross Area: Cross compilation C-bug Category: This is a bug. E-help-wanted Call for participation: Help is requested to fix this issue. and removed O-windows Operating system: Windows labels Aug 29, 2022
@jyn514
Copy link
Member

jyn514 commented Sep 14, 2022

@thomcc how did you get that far in the build? When I try it I get

; ./x.py check --target x86_64-pc-windows-msvc library/std
thread 'main' panicked at '
cmake does not support Visual Studio generators.

This is likely due to it being an msys/cygwin build of cmake,
rather than the required windows version, built using MinGW
or Visual Studio.

If you are building under msys2 try installing the mingw-w64-x86_64-cmake
package instead of cmake:

$ pacman -R cmake && pacman -S mingw-w64-x86_64-cmake
', sanity.rs:221:17

@jyn514
Copy link
Member

jyn514 commented Sep 15, 2022

download-ci-llvm worked`

@jyn514
Copy link
Member

jyn514 commented Sep 15, 2022

To reproduce this I also needed to have the llvm submodule checked out so

cargo.env("RUST_COMPILER_RT_ROOT", &compiler_builtins_root);
would trigger.

fee1-dead added a commit to fee1-dead-contrib/rust that referenced this issue Sep 26, 2022
…tins, r=Mark-Simulacrum

Make the `c` feature for `compiler-builtins` an explicit opt-in

Its build script doesn't support cross-compilation. I tried fixing it, but the cc crate itself doesn't appear to support cross-compiling to windows either unless you use the -gnu toolchain:
```
  error occurred: Failed to find tool. Is `lib.exe` installed?
```

Fixes rust-lang#101172.
@bors bors closed this as completed in bf40408 Sep 29, 2022
@jyn514 jyn514 reopened this Oct 29, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 31, 2022
…iltins, r=jyn514

Revert "Make the `c` feature for `compiler-builtins` opt-in instead of inferred"

This reverts commit 3acb505 (PR rust-lang#101833).

The changes in this commit caused several bugs/incompatibilities (rust-lang#101833 (comment), rust-lang#102560). For now we're reverting this commit and will re-land it alongside fixes for those bugs.

Re-opens rust-lang#101172
cc rust-lang#102560
cc rust-lang#102579
@mati865
Copy link
Contributor

mati865 commented Jan 21, 2023

You can simply run ./x.py check --target x86_64-pc-windows-gnu library/std once you install mingw-w64 cross toolchain that is usually available for your OS.

Details
Checking stage0 library artifacts (x86_64-unknown-linux-gnu -> x86_64-pc-windows-gnu)
    Updating crates.io index
  Downloaded compiler_builtins v0.1.85
  Downloaded libc v0.2.138
  Downloaded 2 crates (776.6 KB) in 1.23s
   Compiling cc v1.0.76
    Checking core v0.0.0 (/home/mateusz/Projects/rust/library/core)
   Compiling libc v0.2.138
   Compiling memchr v2.5.0
   Compiling std v0.0.0 (/home/mateusz/Projects/rust/library/std)
   Compiling compiler_builtins v0.1.85
   Compiling unwind v0.0.0 (/home/mateusz/Projects/rust/library/unwind)
    Checking rustc-std-workspace-core v1.99.0 (/home/mateusz/Projects/rust/library/rustc-std-workspace-core)
    Checking alloc v0.0.0 (/home/mateusz/Projects/rust/library/alloc)
    Checking cfg-if v1.0.0
    Checking adler v1.0.2
    Checking rustc-demangle v0.1.21
    Checking rustc-std-workspace-alloc v1.99.0 (/home/mateusz/Projects/rust/library/rustc-std-workspace-alloc)
    Checking panic_abort v0.0.0 (/home/mateusz/Projects/rust/library/panic_abort)
    Checking panic_unwind v0.0.0 (/home/mateusz/Projects/rust/library/panic_unwind)
    Checking gimli v0.26.2
    Checking object v0.29.0
    Checking std_detect v0.1.5 (/home/mateusz/Projects/rust/library/stdarch/crates/std_detect)
    Checking miniz_oxide v0.5.3
    Checking hashbrown v0.12.3
    Checking addr2line v0.17.0
    Checking rustc-std-workspace-std v1.99.0 (/home/mateusz/Projects/rust/library/rustc-std-workspace-std)
    Checking proc_macro v0.0.0 (/home/mateusz/Projects/rust/library/proc_macro)
    Checking unicode-width v0.1.10
    Checking getopts v0.2.21
    Checking test v0.0.0 (/home/mateusz/Projects/rust/library/test)
    Finished release [optimized] target(s) in 1m 04s
Checking stage0 library test/bench/example targets (x86_64-unknown-linux-gnu -> x86_64-pc-windows-gnu)
    Checking rand_core v0.6.4
    Checking proc_macro v0.0.0 (/home/mateusz/Projects/rust/library/proc_macro)
    Checking test v0.0.0 (/home/mateusz/Projects/rust/library/test)
    Checking rand v0.8.5
    Checking rand_xorshift v0.3.0
    Checking std v0.0.0 (/home/mateusz/Projects/rust/library/std)
    Checking core v0.0.0 (/home/mateusz/Projects/rust/library/core)
    Checking alloc v0.0.0 (/home/mateusz/Projects/rust/library/alloc)
    Finished release [optimized] target(s) in 8.29s
Build completed successfully in 0:01:45

@thomcc
Copy link
Member Author

thomcc commented Jan 21, 2023

That shouldn't be needed to check (it's not needed for check on other targets), and I had trouble getting that to work on aarch64-apple-darwin last time I tried (admittedly it was a while ago, and the situation may have improved).

@jyn514
Copy link
Member

jyn514 commented Jan 21, 2023

Note also that the original issue was for -msvc, not -gnu.

@mati865
Copy link
Contributor

mati865 commented Jan 21, 2023

Note also that the original issue was for -msvc, not -gnu.

To me it sounds like this issue is for an ability to generally check sys::windows code without having to use Windows.
I don't deny the fact check should work without the linker (because it should) but this is workaround that allows to check majority of that code on other platforms.

@jyn514
Copy link
Member

jyn514 commented Mar 10, 2023

don't know when I'll have time to follow up on this. here's a summary of the situation:

the original problem was "bootstrap decides to use the optimized-builtins cargo feature depending on whether src/llvm-project is checked out or not". that's silly and confusing so I turned it into a config flag that's off by default locally and on in CI: #101833. that broke a bunch of distros for reasons I don't quite understand - apparently our atomics are broken on some platforms unless we use llvm intrinsics (??? #101833 (comment)) so @pietroalbini reverted that PR. also @tmandry is doing things I don't understand where llvm is out of tree but still rust-lang/llvm-project, and wants bootstrap to behave differently in those cases: #102560

I opened #102579 to try and fix all those three things, but it's currently stuck on whether the default should be optimized-builtins = false or true. the errors for both suck: here's false https://gist.github.com/pietroalbini/33e1a25fecaddc8ad936eb2ba26ecf05
and true: https://gist.github.com/thomcc/7c889b3a1306922f7eda28f45f1d304e

@thomcc says he's seen the error message for true before and fixed it with -Ctarget-feature=-outline-atomics; maybe that would help here?

@jyn514
Copy link
Member

jyn514 commented Mar 10, 2023

@thomcc says we can't turn off outline-atomics because it's part of the target spec and affects ABI, but we might be able to move the outlined atomics from assembly into global_asm so they work properly with cargo check without needing a linker for the target platform installed.

@thomcc
Copy link
Member Author

thomcc commented Mar 12, 2023

It doesn't effect ABI (these aren't __sync builtins), the issues are that:

  1. These are turned on by either rustc_target and LLVM by somewhat convoluted1 logic that seems hard for bootstrap to replicate

  2. Even if you disable the compiler-builtins C shims, and build the stdlib with RUSTFLAGS=-Ctarget-feature=-outline-atomics, building user code will fail, as the target definition will have re-enabled this as soon as that flag is not provided (but now we won't have definitions for the outline-atomic functions)

So, maybe we should port them to compiler-builtins with global_asm!?

Well, it's unclear. We could port part of it (the function definitions), but part of it depends on being able to perform runtime CPU feature detection, which requires libc on aarch64 Linux targets (specifically the C stub compiled here is responsible for that), and I'm not sure how we want to handle that.

In theory we could compile in a global_asm!-using impl that doesn't ever use runtime feature detection, but this defeats the entire point of outline-atomics, and it's probably better to just find a way to disable the feature globally if built against a compiler-builtins that doesn't have it (somehow?).

IDK, IMO it's kinda a bad situation that we let a mandatory libc dependency slip in on these targets, but I suppose that's just how it is.

Footnotes

  1. Grep for IsAArch64OutlineAtomicsDefault in LLVM. Specifically, +outline-atomics is on by default on aarch64 targets for: fuschia (always) and linux (if the libgcc runtime lib is >= 9.3.1).

    In addition to that rustc_target turns it on by default for all(?) of the targets that would match cfg(all(target_arch = "aarch64", target_env = "gnu", target_os = "llvm")).

    As far as I can tell though, the stubs for outline-atomics are always compiled in on aarch64 targets, just in case a user wants to enable them via -Ctarget-feature=+outline-atomics. I suppose this is useful but causes quite a few problems.

@thomcc
Copy link
Member Author

thomcc commented Mar 12, 2023

That said: that none of this really impacts windows or macOS which don't have any particular need for these.

I don't know if compiler-rt's impls support them on Windows at all (if they do, we don't use them), and aarch64 apple targets don't seem to have them on either (the macOS ones statically the LSE feature available so there'd be no point, and while some of the other apple targets probably could use them, we don't turn +outline-atomics on for them, and I'm not about to suggest starting)

@thomcc
Copy link
Member Author

thomcc commented Mar 12, 2023

CC @Amanieu who may have more nuanced thoughts here than I do.

@workingjubilee
Copy link
Member

workingjubilee commented Mar 12, 2023

Hmm. @sebpop did not mention this disadvantage regarding the feature in the PR to add it. I thought this was handled purely through some other resolution mechanism. I would not have approved that PR if I had been made aware, as Rust's core libraries acquiring a dependency on libc is strictly unacceptable. It is an actual violation of our policies.

@jyn514
Copy link
Member

jyn514 commented Mar 13, 2023

marking this as blocked on #109064; once that's done we can change the default to optimized-builtins = false without causing inscrutable linker errors.

@jyn514 jyn514 added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Mar 13, 2023
@ChrisDenton ChrisDenton removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Dec 13, 2023
@ChrisDenton
Copy link
Member

#109064 is completed so unblocking this.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 4, 2024
…tins, r=onur-ozkan

Reland optimized-compiler-builtins config

Copy of rust-lang#102579 PR.

From rust-lang#102579:

> No concerns on my side. Currently, Jyn isn't actively working on the project. I will close this PR; open another one to cherry-pick the commits, resolve conflicts, and then r+ it.

> Fixes rust-lang#102560. Fixes rust-lang#101172. Helps with rust-lang#105065 (although there's some weirdness there - it's still broken when optimized-compiler-builtins is set to true).

Fixes rust-lang#102560. Fixes rust-lang#101172. Helps with rust-lang#105065

r? ghost
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 7, 2024
…ns, r=onur-ozkan

Reland optimized-compiler-builtins config

Copy of rust-lang#102579 PR.

From rust-lang#102579:

> No concerns on my side. Currently, Jyn isn't actively working on the project. I will close this PR; open another one to cherry-pick the commits, resolve conflicts, and then r+ it.

> Fixes rust-lang#102560. Fixes rust-lang#101172. Helps with rust-lang#105065 (although there's some weirdness there - it's still broken when optimized-compiler-builtins is set to true).

Fixes rust-lang#102560. Fixes rust-lang#101172. Helps with rust-lang#105065

r? ghost
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Jan 10, 2024
…r-ozkan

Reland optimized-compiler-builtins config

Copy of #102579 PR.

From #102579:

> No concerns on my side. Currently, Jyn isn't actively working on the project. I will close this PR; open another one to cherry-pick the commits, resolve conflicts, and then r+ it.

> Fixes rust-lang/rust#102560. Fixes rust-lang/rust#101172. Helps with rust-lang/rust#105065 (although there's some weirdness there - it's still broken when optimized-compiler-builtins is set to true).

Fixes rust-lang/rust#102560. Fixes rust-lang/rust#101172. Helps with rust-lang/rust#105065

r? ghost
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
…r-ozkan

Reland optimized-compiler-builtins config

Copy of #102579 PR.

From #102579:

> No concerns on my side. Currently, Jyn isn't actively working on the project. I will close this PR; open another one to cherry-pick the commits, resolve conflicts, and then r+ it.

> Fixes rust-lang/rust#102560. Fixes rust-lang/rust#101172. Helps with rust-lang/rust#105065 (although there's some weirdness there - it's still broken when optimized-compiler-builtins is set to true).

Fixes rust-lang/rust#102560. Fixes rust-lang/rust#101172. Helps with rust-lang/rust#105065

r? ghost
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 20, 2024
…k-Simulacrum

Make the `c` feature for `compiler-builtins` an explicit opt-in

Its build script doesn't support cross-compilation. I tried fixing it, but the cc crate itself doesn't appear to support cross-compiling to windows either unless you use the -gnu toolchain:
```
  error occurred: Failed to find tool. Is `lib.exe` installed?
```

Fixes rust-lang/rust#101172.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
…k-Simulacrum

Make the `c` feature for `compiler-builtins` an explicit opt-in

Its build script doesn't support cross-compilation. I tried fixing it, but the cc crate itself doesn't appear to support cross-compiling to windows either unless you use the -gnu toolchain:
```
  error occurred: Failed to find tool. Is `lib.exe` installed?
```

Fixes rust-lang/rust#101172.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
…r-ozkan

Reland optimized-compiler-builtins config

Copy of #102579 PR.

From #102579:

> No concerns on my side. Currently, Jyn isn't actively working on the project. I will close this PR; open another one to cherry-pick the commits, resolve conflicts, and then r+ it.

> Fixes rust-lang/rust#102560. Fixes rust-lang/rust#101172. Helps with rust-lang/rust#105065 (although there's some weirdness there - it's still broken when optimized-compiler-builtins is set to true).

Fixes rust-lang/rust#102560. Fixes rust-lang/rust#101172. Helps with rust-lang/rust#105065

r? ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cross Area: Cross compilation C-bug Category: This is a bug. E-help-wanted Call for participation: Help is requested to fix this issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
5 participants