Skip to content

[X86] Fix overflow with large stack probes on x86-64 #113219

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 22, 2025
Merged

[X86] Fix overflow with large stack probes on x86-64 #113219

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f2b79ab
[X86] Fix overflow with large stack probes on x86-64
mconst Oct 19, 2024
e3d04a5
don't error on too-large stack frames, just trap at runtime
mconst Oct 21, 2024
797a0ae
add test case that triggers the trap
mconst Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 35 additions & 10 deletions llvm/lib/Target/X86/X86FrameLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -798,18 +798,43 @@ void X86FrameLowering::emitStackProbeInlineGenericLoop(
: Is64Bit ? X86::R11D
: X86::EAX;

BuildMI(MBB, MBBI, DL, TII.get(TargetOpcode::COPY), FinalStackProbed)
.addReg(StackPtr)
.setMIFlag(MachineInstr::FrameSetup);

// save loop bound
{
const unsigned BoundOffset = alignDown(Offset, StackProbeSize);
const unsigned SUBOpc = getSUBriOpcode(Uses64BitFramePtr);
BuildMI(MBB, MBBI, DL, TII.get(SUBOpc), FinalStackProbed)
.addReg(FinalStackProbed)
.addImm(BoundOffset)
.setMIFlag(MachineInstr::FrameSetup);
const uint64_t BoundOffset = alignDown(Offset, StackProbeSize);

// Can we calculate the loop bound using SUB with a 32-bit immediate?
// Note that the immediate gets sign-extended when used with a 64-bit
// register, so in that case we only have 31 bits to work with.
bool canUseSub =
Uses64BitFramePtr ? isUInt<31>(BoundOffset) : isUInt<32>(BoundOffset);

if (canUseSub) {
const unsigned SUBOpc = getSUBriOpcode(Uses64BitFramePtr);

BuildMI(MBB, MBBI, DL, TII.get(TargetOpcode::COPY), FinalStackProbed)
.addReg(StackPtr)
.setMIFlag(MachineInstr::FrameSetup);
BuildMI(MBB, MBBI, DL, TII.get(SUBOpc), FinalStackProbed)
.addReg(FinalStackProbed)
.addImm(BoundOffset)
.setMIFlag(MachineInstr::FrameSetup);
} else if (Uses64BitFramePtr) {
BuildMI(MBB, MBBI, DL, TII.get(X86::MOV64ri), FinalStackProbed)
.addImm(-BoundOffset)
.setMIFlag(MachineInstr::FrameSetup);
BuildMI(MBB, MBBI, DL, TII.get(X86::ADD64rr), FinalStackProbed)
.addReg(FinalStackProbed)
.addReg(StackPtr)
.setMIFlag(MachineInstr::FrameSetup);
} else {
// We're being asked to probe a stack frame that's 4 GiB or larger,
// but our stack pointer is only 32 bits.
Copy link
Collaborator

@efriedma-quic efriedma-quic Oct 21, 2024

Choose a reason for hiding this comment

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

I'm a little concerned about printing an error for valid code... maybe less of an issue here than in other contexts, but generally, stack overflows should be a runtime error.

Maybe we can just set the size of the allocation to 2^32-1, and just let the OS generate a stack overflow trap?

Copy link
Contributor Author

@mconst mconst Oct 21, 2024

Choose a reason for hiding this comment

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

Thanks for looking at this!

Sure, trapping at runtime sounds fine, although I'd kind of like to make it an unconditional trap rather than allocating less than the user requested and relying on it to fail. (It's not likely, but you could imagine a situation where the user's code is elsewhere in the address space and the entire lower 4 GiB is available for the stack. The stack pointer would also have to be unaligned.)

Would you replace the error with a warning, or just skip it entirely and rely on the existing -Wframe-larger-than warning?

Copy link
Collaborator

@efriedma-quic efriedma-quic Oct 21, 2024

Choose a reason for hiding this comment

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

I don't think it's realistic to worry about an 32-bit x86 application with an address-space larger than 4GB; I don't think it's actually possible to setup a configuration like that. But an explicit trap is probably okay too.

A warning has basically the same issues as an error: optimizations can cause it to trigger in dynamically unreachable code, and the user is left with a nonsense diagnostic that can only be fixed by blindly refactoring their code until the compiler stops complaining. So general policy is that we don't emit warnings from the backend for runtime issues.

Copy link
Contributor Author

@mconst mconst Oct 21, 2024

Choose a reason for hiding this comment

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

Sounds good! I've updated the code.

DiagnosticInfoResourceLimit Diag(MF.getFunction(),
"probed stack frame size", BoundOffset,
0xffffffff, DS_Error, DK_ResourceLimit);
MF.getFunction().getContext().diagnose(Diag);
return;
}

// while in the loop, use loop-invariant reg for CFI,
// instead of the stack pointer, which changes during the loop
Expand Down
84 changes: 84 additions & 0 deletions llvm/test/CodeGen/X86/stack-clash-huge.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --no_x86_scrub_sp
; RUN: llc -mtriple=x86_64-linux-android < %s | FileCheck -check-prefix=CHECK-X64 %s
; RUN: llc -mtriple=i686-linux-android < %s | FileCheck -check-prefix=CHECK-X86 %s
; RUN: llc -mtriple=x86_64-linux-gnux32 < %s | FileCheck -check-prefix=CHECK-X32 %s

define i32 @foo() local_unnamed_addr #0 {
; CHECK-X64-LABEL: foo:
; CHECK-X64: # %bb.0:
; CHECK-X64-NEXT: movabsq $-2399997952, %r11 # imm = 0xFFFFFFFF70F2F000
; CHECK-X64-NEXT: addq %rsp, %r11
; CHECK-X64-NEXT: .cfi_def_cfa_register %r11
; CHECK-X64-NEXT: .cfi_adjust_cfa_offset 2399997952
; CHECK-X64-NEXT: .LBB0_1: # =>This Inner Loop Header: Depth=1
; CHECK-X64-NEXT: subq $4096, %rsp # imm = 0x1000
; CHECK-X64-NEXT: movq $0, (%rsp)
; CHECK-X64-NEXT: cmpq %r11, %rsp
; CHECK-X64-NEXT: jne .LBB0_1
; CHECK-X64-NEXT: # %bb.2:
; CHECK-X64-NEXT: subq $1928, %rsp # imm = 0x788
; CHECK-X64-NEXT: .cfi_def_cfa_register %rsp
; CHECK-X64-NEXT: .cfi_def_cfa_offset 2399999888
; CHECK-X64-NEXT: movl $1, 264(%rsp)
; CHECK-X64-NEXT: movl $1, 28664(%rsp)
; CHECK-X64-NEXT: movl -128(%rsp), %eax
; CHECK-X64-NEXT: movl $2399999880, %ecx # imm = 0x8F0D1788
; CHECK-X64-NEXT: addq %rcx, %rsp
; CHECK-X64-NEXT: .cfi_def_cfa_offset 8
; CHECK-X64-NEXT: retq
;
; CHECK-X86-LABEL: foo:
; CHECK-X86: # %bb.0:
; CHECK-X86-NEXT: movl %esp, %eax
; CHECK-X86-NEXT: subl $2399997952, %eax # imm = 0x8F0D1000
; CHECK-X86-NEXT: .cfi_def_cfa_register %eax
; CHECK-X86-NEXT: .cfi_adjust_cfa_offset 2399997952
; CHECK-X86-NEXT: .LBB0_1: # =>This Inner Loop Header: Depth=1
; CHECK-X86-NEXT: subl $4096, %esp # imm = 0x1000
; CHECK-X86-NEXT: movl $0, (%esp)
; CHECK-X86-NEXT: cmpl %eax, %esp
; CHECK-X86-NEXT: jne .LBB0_1
; CHECK-X86-NEXT: # %bb.2:
; CHECK-X86-NEXT: subl $2060, %esp # imm = 0x80C
; CHECK-X86-NEXT: .cfi_def_cfa_register %esp
; CHECK-X86-NEXT: .cfi_def_cfa_offset 2400000016
; CHECK-X86-NEXT: movl $1, 392(%esp)
; CHECK-X86-NEXT: movl $1, 28792(%esp)
; CHECK-X86-NEXT: movl (%esp), %eax
; CHECK-X86-NEXT: movl $2400000012, %ecx # imm = 0x8F0D180C
; CHECK-X86-NEXT: addl %ecx, %esp
; CHECK-X86-NEXT: .cfi_def_cfa_offset 4
; CHECK-X86-NEXT: retl
;
; CHECK-X32-LABEL: foo:
; CHECK-X32: # %bb.0:
; CHECK-X32-NEXT: movl %esp, %r11d
; CHECK-X32-NEXT: subl $2399997952, %r11d # imm = 0x8F0D1000
; CHECK-X32-NEXT: .cfi_def_cfa_register %r11
; CHECK-X32-NEXT: .cfi_adjust_cfa_offset 2399997952
; CHECK-X32-NEXT: .LBB0_1: # =>This Inner Loop Header: Depth=1
; CHECK-X32-NEXT: subl $4096, %esp # imm = 0x1000
; CHECK-X32-NEXT: movq $0, (%esp)
; CHECK-X32-NEXT: cmpl %r11d, %esp
; CHECK-X32-NEXT: jne .LBB0_1
; CHECK-X32-NEXT: # %bb.2:
; CHECK-X32-NEXT: subl $1928, %esp # imm = 0x788
; CHECK-X32-NEXT: .cfi_def_cfa_register %rsp
; CHECK-X32-NEXT: .cfi_def_cfa_offset 2399999888
; CHECK-X32-NEXT: movl $1, 264(%esp)
; CHECK-X32-NEXT: movl $1, 28664(%esp)
; CHECK-X32-NEXT: movl -128(%esp), %eax
; CHECK-X32-NEXT: movl $2399999880, %ecx # imm = 0x8F0D1788
; CHECK-X32-NEXT: addq %rcx, %esp
Copy link
Contributor

@phoebewang phoebewang Dec 17, 2024

Choose a reason for hiding this comment

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

Bad instruction.

Copy link
Contributor Author

@mconst mconst Dec 18, 2024

Choose a reason for hiding this comment

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

Thanks for looking at this!

I think this is an unrelated bug. LLVM generates the same invalid instruction even without my changes -- you can reproduce it by compiling the following code with -mx32 -fomit-frame-pointer:

void foo() {
    char x[0xa0000000];
}

https://godbolt.org/z/E41ecqPna

If you want, I can try to fix this, but maybe it might make more sense to do that as a separate PR?

Copy link
Contributor

@phoebewang phoebewang Dec 18, 2024

Choose a reason for hiding this comment

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

Ok, it's not from the modified code. Feel free to fix it or file an issue otherwise.

; CHECK-X32-NEXT: .cfi_def_cfa_offset 8
; CHECK-X32-NEXT: retq
%a = alloca i32, i64 600000000, align 16
%b0 = getelementptr inbounds i32, ptr %a, i64 98
%b1 = getelementptr inbounds i32, ptr %a, i64 7198
store volatile i32 1, ptr %b0
store volatile i32 1, ptr %b1
%c = load volatile i32, ptr %a
ret i32 %c
}

attributes #0 = {"probe-stack"="inline-asm"}
Loading