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

x86-interrupt calling convention with error code is broken on x86_64-unknown-none #109918

Closed
SamZhang3 opened this issue Apr 4, 2023 · 3 comments · Fixed by #111672
Closed

x86-interrupt calling convention with error code is broken on x86_64-unknown-none #109918

SamZhang3 opened this issue Apr 4, 2023 · 3 comments · Fixed by #111672
Labels
A-ABI Area: Concerning the application binary interface (ABI) C-bug Category: This is a bug. O-x86_32 Target: x86 processors, 32 bit (like i686-*) O-x86_64 Target: x86-64 processors (like x86_64-*) requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@SamZhang3
Copy link
Contributor

SamZhang3 commented Apr 4, 2023

See this godbolt link.

At line 177 of the generated assembly, it loads the address rsp + 176 and passes it to the core::fmt::ArgumentV1::new_debug function. The offset 176 is incorrect since the data pushed to the stack in lines 166-175, the 40-byte stack frame pushed during the interrupt, and the 8-byte error code are only 128 bytes total, so the address will point to something random below the interrupt stack frame on the stack.

This does not occur if the error code argument is removed: see godbolt. Here, an additional sub rsp, 80 instruction appears at line 175 after saving registers, and the offset used at line 177 is correct (skipping the 80 bytes plus the 72 from the 9 saved registers, for 152 bytes total).

Experimenting with custom target JSON specifications instead of x86_64-unknown-none, it appears that the bug disappears when the "stack-probes": {"kind": "inline-or-call", ...} part of the specification is removed.

@SamZhang3 SamZhang3 added the C-bug Category: This is a bug. label Apr 4, 2023
SamZhang3 added a commit to SamZhang3/doors-rs-old that referenced this issue Apr 4, 2023
Currently the x86-interrupt calling convention seems to be broken; see
rust-lang/rust#109918. This results in junk
values when printing the InterruptStackFrame.
@bjorn3 bjorn3 added requires-nightly This issue requires a nightly compiler in some way. A-ABI Area: Concerning the application binary interface (ABI) O-x86 O-x86_64 Target: x86-64 processors (like x86_64-*) labels Apr 4, 2023
@cuviper
Copy link
Member

cuviper commented Apr 5, 2023

cc #40180

@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
SamZhang3 added a commit to SamZhang3/doors-rs-old that referenced this issue Apr 5, 2023
We avoid using the "x86-interrupt" abi and instead implement interrupt
service routines directly in assembly. This avoids the bug
rust-lang/rust#109918, allowing a correct stack
frame to be obtained by the handler.
@Freax13
Copy link
Contributor

Freax13 commented May 3, 2023

This regression seems to have been introduced by the LLVM 16 upgrade.


searched nightlies: from nightly-2023-03-24 to nightly-2023-05-03
regressed nightly: nightly-2023-03-26
searched commit range: 8be3c2b...0c61c7a
regressed commit: 0c61c7a

bisected with cargo-bisect-rustc v0.6.5

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --script build.sh --start 2023-03-24 -vvvvv --with-src --preserve 

@Freax13
Copy link
Contributor

Freax13 commented May 6, 2023

This bug seems to have been present in LLVM versions earlier than 16, I'm not sure why this only surfaced now. I'm working on a patch for LLVM.

Freax13 added a commit to Freax13/llvm-project that referenced this issue May 7, 2023
Previously the x86_intrcc calling convention built two
STACKALLOC_W_PROBING machine instructions if the function took an error
code. This was caused by an additional call to emitSPUpdate in
llvm/lib/Target/X86/X86FrameLowering.cpp:1650. The code for inlining the
stack probes only handles the first STACKALLOC_W_PROBING machine
instruction and ignores any futher `STACKALLOC_W_PROBING` instructions.
This lead to miscompilations where the stack pointer wasn't properly
updated (see rust-lang/rust#109918).
This patch prevents a second STACKALLOC_W_PROBING instruction from being
built by hard-coding the stack update to `push rax`.

To be honest I don't quite understand why this didn't lead to more
noticeable miscompilations previously.
phoebewang pushed a commit to llvm/llvm-project that referenced this issue May 9, 2023
The x86_intrcc calling convention will build two STACKALLOC_W_PROBING machine instructions if the function takes an error code. This is caused by an additional call to emitSPUpdate in llvm/lib/Target/X86/X86FrameLowering.cpp:1650. Previously only the first STACKALLOC_W_PROBING machine instruction was properly handled, the second one was simply ignored. This lead to miscompilations where the stack pointer wasn't properly updated (see rust-lang/rust#109918). This patch fixes this by handling all STACKALLOC_W_PROBING machine instructions.

To be honest I don't quite understand why this didn't lead to more noticeable miscompilations previously.

This is my first time contributing to LLVM.

Reviewed By: pengfei

Differential Revision: https://reviews.llvm.org/D150033
Freax13 added a commit to Freax13/llvm-project that referenced this issue May 12, 2023
The x86_intrcc calling convention will build two STACKALLOC_W_PROBING machine instructions if the function takes an error code. This is caused by an additional call to emitSPUpdate in llvm/lib/Target/X86/X86FrameLowering.cpp:1650. Previously only the first STACKALLOC_W_PROBING machine instruction was properly handled, the second one was simply ignored. This lead to miscompilations where the stack pointer wasn't properly updated (see rust-lang/rust#109918). This patch fixes this by handling all STACKALLOC_W_PROBING machine instructions.

To be honest I don't quite understand why this didn't lead to more noticeable miscompilations previously.

This is my first time contributing to LLVM.

Reviewed By: pengfei

Differential Revision: https://reviews.llvm.org/D150033
llvmbot pushed a commit to llvm/llvm-project-release-prs that referenced this issue May 13, 2023
The x86_intrcc calling convention will build two STACKALLOC_W_PROBING machine instructions if the function takes an error code. This is caused by an additional call to emitSPUpdate in llvm/lib/Target/X86/X86FrameLowering.cpp:1650. Previously only the first STACKALLOC_W_PROBING machine instruction was properly handled, the second one was simply ignored. This lead to miscompilations where the stack pointer wasn't properly updated (see rust-lang/rust#109918). This patch fixes this by handling all STACKALLOC_W_PROBING machine instructions.

To be honest I don't quite understand why this didn't lead to more noticeable miscompilations previously.

This is my first time contributing to LLVM.

Reviewed By: pengfei

Differential Revision: https://reviews.llvm.org/D150033

(cherry picked from commit f615436)
tstellar pushed a commit to llvm/llvm-project-release-prs that referenced this issue May 16, 2023
The x86_intrcc calling convention will build two STACKALLOC_W_PROBING machine instructions if the function takes an error code. This is caused by an additional call to emitSPUpdate in llvm/lib/Target/X86/X86FrameLowering.cpp:1650. Previously only the first STACKALLOC_W_PROBING machine instruction was properly handled, the second one was simply ignored. This lead to miscompilations where the stack pointer wasn't properly updated (see rust-lang/rust#109918). This patch fixes this by handling all STACKALLOC_W_PROBING machine instructions.

To be honest I don't quite understand why this didn't lead to more noticeable miscompilations previously.

This is my first time contributing to LLVM.

Reviewed By: pengfei

Differential Revision: https://reviews.llvm.org/D150033

(cherry picked from commit f615436)
@bors bors closed this as completed in 24c180c May 18, 2023
@Noratrieb Noratrieb added O-x86_32 Target: x86 processors, 32 bit (like i686-*) and removed O-x86-all labels Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ABI Area: Concerning the application binary interface (ABI) C-bug Category: This is a bug. O-x86_32 Target: x86 processors, 32 bit (like i686-*) O-x86_64 Target: x86-64 processors (like x86_64-*) requires-nightly This issue requires a nightly compiler in some way. 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.

5 participants