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

i686-pc-windows-gnu fails to compile rsbegin #109996

Closed
kleisauke opened this issue Apr 6, 2023 · 11 comments · Fixed by #110283
Closed

i686-pc-windows-gnu fails to compile rsbegin #109996

kleisauke opened this issue Apr 6, 2023 · 11 comments · Fixed by #110283
Labels
C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@kleisauke
Copy link
Contributor

Reproducer

I tried this script:

$ curl https://sh.rustup.rs -sSf | sh -s -- -y \
    --profile minimal \
    --target i686-pc-windows-gnu \
    --default-toolchain nightly-2023-04-01 \
    --component rust-src
$ source "$HOME/.cargo/env"
$ rustc --target=i686-pc-windows-gnu --emit=obj -o rsbegin.o $(rustc --print sysroot)/lib/rustlib/src/rust/library/rtstartup/rsbegin.rs

I expected to see a successful complication.

Instead, this happened:

error: requires `panic` lang_item
  --> /home/kleisauke/.rustup/toolchains/nightly-2023-04-01-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/rtstartup/rsbegin.rs:82:65
   |
82 |         __register_frame_info(&__EH_FRAME_BEGIN__ as *const u8, &mut OBJ as *mut _ as *mut u8);
   |                                                                 ^^^^^^^^

error: requires `panic` lang_item
  --> /home/kleisauke/.rustup/toolchains/nightly-2023-04-01-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/rtstartup/rsbegin.rs:87:67
   |
87 |         __deregister_frame_info(&__EH_FRAME_BEGIN__ as *const u8, &mut OBJ as *mut _ as *mut u8);
   |                                                                   ^^^^^^^^

error: aborting due to 2 previous errors

Passing -Zmir-enable-passes=-CheckAlignment seems to fix this, so it looks like a possible regression introduced in #98112.

Version it worked on

It most recently worked on: rustc 1.70.0-nightly (ec2f40c 2023-03-30)

Version with regression

rustc --version --verbose:

rustc 1.70.0-nightly (5e1d3299a 2023-03-31)
binary: rustc
commit-hash: 5e1d3299a290026b85787bc9c7e72bcc53ac283f
commit-date: 2023-03-31
host: x86_64-unknown-linux-gnu
release: 1.70.0-nightly
LLVM version: 16.0.0

@rustbot modify labels: +regression-from-stable-to-nightly -regression-untriaged

@kleisauke kleisauke added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Apr 6, 2023
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed regression-untriaged Untriaged performance or correctness regression. labels Apr 6, 2023
@apiraino
Copy link
Contributor

apiraino commented Apr 6, 2023

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Apr 6, 2023
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 6, 2023
@apiraino
Copy link
Contributor

apiraino commented Apr 6, 2023

@rustbot label -I-prioritize +P-medium -P-high

@rustbot rustbot added P-medium Medium priority and removed P-high High priority labels Apr 6, 2023
@saethlin
Copy link
Member

saethlin commented Apr 6, 2023

This is a nice reproducer but the priority was reduced because the use seems very niche and/or unmotivated. Can you explain why compiling just rsbegin is a requirement for you? Or what broader thing this is part of?

@bjorn3
Copy link
Member

bjorn3 commented Apr 6, 2023

I was surprised that building the sysroot for i686-pc-windows-gnu doesn't hit this. rsbegin.rs and rsend.rs should be compiled as part of building the sysroot for mingw targets. Turns out we always compile them with the bootstrap compiler:

let mut cmd = Command::new(&builder.initial_rustc);
As such this issue will block bumping the bootstrap compiler to one containing #98112 and breaks local rebuilds. I'm pretty sure it will also prevent me from updating cg_clif to the latest nightly as cg_clif rebuilds rsbegin.rs and rsend.rs when targeting mingw.

The fix for this issue I think is to use addr_of_mut!() instead. Or equivalently using &raw mut, which addr_of_mut! expands to, as addr_of_mut!() is not available in these #![no_core] crates.

@kleisauke
Copy link
Contributor Author

Indeed, this could be problematic when this regression would be present in the stage0 compiler, or when build.rustc is set to a nightly after #98112.

rust/config.example.toml

Lines 213 to 216 in 0534655

# Instead of downloading the src/stage0.json version of the compiler
# specified, use this rustc binary instead as the stage0 snapshot compiler.
# If you set this, you likely want to set `cargo` as well.
#rustc = "/path/to/rustc"

FWIW, this was originally caught here which builds these startup objects with the stage1 compiler. I've never encountered any problems with this approach until now.

@saethlin
Copy link
Member

saethlin commented Apr 6, 2023

The declaration for the static is here:

static mut OBJ: [isize; 6] = [0; 6];

So (since it is not extern) LLVM should be able to optimize out the alignment check, and indeed it does. -Copt-level=1 suffices. So this problem is specific to unoptimized builds, which we explicitly identify as unsupported:

rust/config.example.toml

Lines 396 to 400 in f5b8f44

# Whether or not to optimize the compiler and standard library.
# WARNING: Building with optimize = false is NOT SUPPORTED. Due to bootstrapping,
# building without optimizations takes much longer than optimizing. Further, some platforms
# fail to build without this optimization (c.f. #65352).
#optimize = true

So I don't think this qualifies as a regression. But there is a separate question: It looks like you have been using an unsupported configuration, and maybe found it useful? Maybe we should try to support this use case on those grounds?

@bjorn3
Copy link
Member

bjorn3 commented Apr 6, 2023

The mere presence of this check in MIR that is codegened is enough to cause rustc to record a use of the panic lang item. It doesn't matter that the check is removed by optimizations, rustc will check that the lang item is defined by this crate or any of it's dependencies.

@saethlin
Copy link
Member

saethlin commented Apr 6, 2023

Ah! Adding -Copt-level=1 turns off debug assertions. Knowing that, this makes more sense.

@kleisauke
Copy link
Contributor Author

Here's another reproducer using the bootstrap script:

$ curl https://sh.rustup.rs -sSf | sh -s -- -y --profile minimal --default-toolchain nightly
$ source "$HOME/.cargo/env"
$ git clone https://github.com/rust-lang/rust.git
$ cd rust
$ ./configure --set build.rustc="$(which rustc)" --set build.cargo="$(which cargo)"
$ ./x.py build --target i686-pc-windows-gnu

(this assumes i686-w64-mingw32-gcc is available, for example on Fedora you need to install mingw32-gcc)

So this is not an unsupported configuration, rsbegin/rsend is always compiled without optimizations and therefore this should still be considered a regression.

It looks like you have been using an unsupported configuration, and maybe found it useful? Maybe we should try to support this use case on those grounds?

FWIW, the only reason I compile Rust from source is to support the 32-bit *-pc-windows-gnullvm targets. I've already reported this here: #72241 (comment).

@mati865 Does this unwinding issue still occurs on the 32-bit LLVM targets when bootstrapping Rust natively?

@saethlin
Copy link
Member

@bjorn3 &raw doesn't help, because it still does a Deref of the static pointer.

@kleisauke Sorry I didn't respond on here for a while, I've been thinking about this and finally came up with something that I think works, if it is a bit ungraceful.

@mati865
Copy link
Contributor

mati865 commented Apr 13, 2023

@mati865 Does this unwinding issue still occurs on the 32-bit LLVM targets when bootstrapping Rust natively?

@kleisauke yes, it was fixed by https://reviews.llvm.org/D136879
Given Rust uses LLVM 16 now and no patching is required I suppose this target could be added.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 13, 2023
…ment, r=bjorn3

Only emit alignment checks if we have a panic_impl

Fixes rust-lang#109996

r? `@bjorn3` because you commented that this situation could impact you as well
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants