This is an archive of the discontinued LLVM Phabricator instance.

Differential D90216

[stack-clash] Fix probing of dynamic alloca
ClosedPublic

Authored by serge-sans-paille on Oct 27 2020, 3:17 AM.

Details

Reviewers
cuviper
Commits
rG0f60bcc36c34: [stack-clash] Fix probing of dynamic alloca
Summary

Perform the probing in the *cough* correct direction.




Related to https://github.com/rust-lang/rust/pull/77885#issuecomment-711062924
Diff Detail
Event Timeline
serge-sans-paille created this revision.Oct 27 2020, 3:17 AM
Herald added a project: Restricted Project.  ·  View Herald TranscriptOct 27 2020, 3:17 AM
Herald added subscribers: llvm-commits, JDevlieghere, hiraditya.  ·  View Herald Transcript
serge-sans-paille requested review of this revision.Oct 27 2020, 3:17 AM
mati865 added a subscriber: mati865.Oct 27 2020, 4:04 AM
cuviper added a comment.Oct 27 2020, 1:26 PM
I tried the C reproducer from that Rust bug, and it turns out that alloca(0) was already fixed by D88548. But if I change that to a larger size, then it was skipping right over the probe loop. I confirmed that your change here does make it go through the loop.



It would be nice to have tests that actually execute this stuff, rather than just relying on manual reviews of the expected assembly, but I don't know if that's feasible in the current infrastructure.
llvm/test/CodeGen/X86/stack-clash-dynamic-alloca.ll


29–30
Shouldn't this sub before mov 0? I think right now, the first iteration is going to clobber the most recent thing on the stack, in this case the saved value from pushq %rbp.
serge-sans-paille updated this revision to Diff 301257.Oct 28 2020, 6:28 AM
Fix issue with clobbered stack memory
cuviper accepted this revision.Oct 28 2020, 1:49 PM
Works for me!
This revision is now accepted and ready to land.Oct 28 2020, 1:49 PM
Closed by commit rG0f60bcc36c34: [stack-clash] Fix probing of dynamic alloca (authored by serge-sans-paille).  ·  Explain WhyOct 30 2020, 7:34 AM
This revision was automatically updated to reflect the committed changes.
serge-sans-paille added a commit: rG0f60bcc36c34: [stack-clash] Fix probing of dynamic alloca.
Revision Contents
PathSize
llvm/
lib/
Target/
X86/
2 lines
test/
CodeGen/
X86/
8 lines
4 lines
Diff 300933
llvm/lib/Target/X86/X86ISelLowering.cpp
  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 32,307 Lines • ▼ Show 20 LinesX86TargetLowering::EmitLoweredProbedAlloca(MachineInstr &MI,




  BuildMI(testMBB, DL,
  BuildMI(testMBB, DL,

          TII->get(TFI.Uses64BitFramePtr ? X86::CMP64rr : X86::CMP32rr))
          TII->get(TFI.Uses64BitFramePtr ? X86::CMP64rr : X86::CMP32rr))

      .addReg(FinalStackPtr)
      .addReg(FinalStackPtr)

      .addReg(physSPReg);
      .addReg(physSPReg);




  BuildMI(testMBB, DL, TII->get(X86::JCC_1))
  BuildMI(testMBB, DL, TII->get(X86::JCC_1))

      .addMBB(tailMBB)
      .addMBB(tailMBB)

      .addImm(X86::COND_LE);
      .addImm(X86::COND_GE);

  testMBB->addSuccessor(blockMBB);
  testMBB->addSuccessor(blockMBB);

  testMBB->addSuccessor(tailMBB);
  testMBB->addSuccessor(tailMBB);




  // Touch the block then extend it. This is done on the opposite side of
  // Touch the block then extend it. This is done on the opposite side of

  // static probe where we allocate then touch, to avoid the need of probing the
  // static probe where we allocate then touch, to avoid the need of probing the

  // tail of the static alloca. Possible scenarios are:
  // tail of the static alloca. Possible scenarios are:

  //
  //

  //       + ---- <- ------------ <- ------------- <- ------------ +
  //       + ---- <- ------------ <- ------------- <- ------------ +

▲ Show 20 Lines • Show All 18,898 Lines • Show Last 20 Lines
llvm/test/CodeGen/X86/stack-clash-dynamic-alloca.ll
Show All 18 Lines
; CHECK-X86-64-NEXT:    movq  %rsp, %rbp
; CHECK-X86-64-NEXT:    movq  %rsp, %rbp

; CHECK-X86-64-NEXT:    .cfi_def_cfa_register %rbp
; CHECK-X86-64-NEXT:    .cfi_def_cfa_register %rbp

; CHECK-X86-64-NEXT:    movq    %rsp, %rax
; CHECK-X86-64-NEXT:    movq    %rsp, %rax

; CHECK-X86-64-NEXT:    movl    %edi, %ecx
; CHECK-X86-64-NEXT:    movl    %edi, %ecx

; CHECK-X86-64-NEXT:    leaq 15(,%rcx,4), %rcx
; CHECK-X86-64-NEXT:    leaq 15(,%rcx,4), %rcx

; CHECK-X86-64-NEXT:    andq  $-16, %rcx
; CHECK-X86-64-NEXT:    andq  $-16, %rcx

; CHECK-X86-64-NEXT:    subq  %rcx, %rax
; CHECK-X86-64-NEXT:    subq  %rcx, %rax

; CHECK-X86-64-NEXT:    cmpq  %rsp, %rax
; CHECK-X86-64-NEXT:    cmpq  %rsp, %rax

; CHECK-X86-64-NEXT:    jle .LBB0_3
; CHECK-X86-64-NEXT:    jge .LBB0_3

; CHECK-X86-64-NEXT:  .LBB0_2: # =>This Inner Loop Header: Depth=1
; CHECK-X86-64-NEXT:  .LBB0_2: # =>This Inner Loop Header: Depth=1

; CHECK-X86-64-NEXT:    movq  $0, (%rsp)
; CHECK-X86-64-NEXT:    movq  $0, (%rsp)

; CHECK-X86-64-NEXT:    subq  $4096, %rsp # imm = 0x1000
; CHECK-X86-64-NEXT:    subq  $4096, %rsp # imm = 0x1000

cuviperUnsubmitted
Not Done
ReplyInline Actions
Shouldn't this sub before mov 0? I think right now, the first iteration is going to clobber the most recent thing on the stack, in this case the saved value from pushq %rbp.
cuviper: Shouldn't this `sub` before `mov 0`? I think right now, the first iteration is going to clobber…
; CHECK-X86-64-NEXT:    cmpq  %rsp, %rax
; CHECK-X86-64-NEXT:    cmpq  %rsp, %rax

; CHECK-X86-64-NEXT:    jg  .LBB0_2
; CHECK-X86-64-NEXT:    jl  .LBB0_2

; CHECK-X86-64-NEXT:  .LBB0_3:
; CHECK-X86-64-NEXT:  .LBB0_3:

; CHECK-X86-64-NEXT:    movq  %rax, %rsp
; CHECK-X86-64-NEXT:    movq  %rax, %rsp

; CHECK-X86-64-NEXT:    movl  $1, 4792(%rax)
; CHECK-X86-64-NEXT:    movl  $1, 4792(%rax)

; CHECK-X86-64-NEXT:    movl  (%rax), %eax
; CHECK-X86-64-NEXT:    movl  (%rax), %eax

; CHECK-X86-64-NEXT:    movq  %rbp, %rsp
; CHECK-X86-64-NEXT:    movq  %rbp, %rsp

; CHECK-X86-64-NEXT:    popq  %rbp
; CHECK-X86-64-NEXT:    popq  %rbp

; CHECK-X86-64-NEXT:  .cfi_def_cfa %rsp, 8
; CHECK-X86-64-NEXT:  .cfi_def_cfa %rsp, 8

; CHECK-X86-64-NEXT:   retq
; CHECK-X86-64-NEXT:   retq







; CHECK-X86-32-LABEL: foo:
; CHECK-X86-32-LABEL: foo:

; CHECK-X86-32:       # %bb.0:
; CHECK-X86-32:       # %bb.0:

; CHECK-X86-32-NEXT:    pushl   %ebp
; CHECK-X86-32-NEXT:    pushl   %ebp

; CHECK-X86-32-NEXT:    .cfi_def_cfa_offset 8
; CHECK-X86-32-NEXT:    .cfi_def_cfa_offset 8

; CHECK-X86-32-NEXT:    .cfi_offset %ebp, -8
; CHECK-X86-32-NEXT:    .cfi_offset %ebp, -8

; CHECK-X86-32-NEXT:    movl    %esp, %ebp
; CHECK-X86-32-NEXT:    movl    %esp, %ebp

; CHECK-X86-32-NEXT:    .cfi_def_cfa_register %ebp
; CHECK-X86-32-NEXT:    .cfi_def_cfa_register %ebp

; CHECK-X86-32-NEXT:    subl    $8, %esp
; CHECK-X86-32-NEXT:    subl    $8, %esp

; CHECK-X86-32-NEXT:    movl    8(%ebp), %ecx
; CHECK-X86-32-NEXT:    movl    8(%ebp), %ecx

; CHECK-X86-32-NEXT:    movl    %esp, %eax
; CHECK-X86-32-NEXT:    movl    %esp, %eax

; CHECK-X86-32-NEXT:    leal    15(,%ecx,4), %ecx
; CHECK-X86-32-NEXT:    leal    15(,%ecx,4), %ecx

; CHECK-X86-32-NEXT:    andl    $-16, %ecx
; CHECK-X86-32-NEXT:    andl    $-16, %ecx

; CHECK-X86-32-NEXT:    subl    %ecx, %eax
; CHECK-X86-32-NEXT:    subl    %ecx, %eax

; CHECK-X86-32-NEXT:    cmpl    %esp, %eax
; CHECK-X86-32-NEXT:    cmpl    %esp, %eax

; CHECK-X86-32-NEXT:    jle  .LBB0_3
; CHECK-X86-32-NEXT:    jge  .LBB0_3

; CHECK-X86-32-NEXT:  .LBB0_2: # =>This Inner Loop Header: Depth=1
; CHECK-X86-32-NEXT:  .LBB0_2: # =>This Inner Loop Header: Depth=1

; CHECK-X86-32-NEXT:    movl    $0, (%esp)
; CHECK-X86-32-NEXT:    movl    $0, (%esp)

; CHECK-X86-32-NEXT:    subl    $4096, %esp # imm = 0x1000
; CHECK-X86-32-NEXT:    subl    $4096, %esp # imm = 0x1000

; CHECK-X86-32-NEXT:    cmpl    %esp, %eax
; CHECK-X86-32-NEXT:    cmpl    %esp, %eax

; CHECK-X86-32-NEXT:    jg .LBB0_2
; CHECK-X86-32-NEXT:    jl .LBB0_2

; CHECK-X86-32-NEXT:  .LBB0_3:
; CHECK-X86-32-NEXT:  .LBB0_3:

; CHECK-X86-32-NEXT:    movl    %eax, %esp
; CHECK-X86-32-NEXT:    movl    %eax, %esp

; CHECK-X86-32-NEXT:    movl    $1, 4792(%eax)
; CHECK-X86-32-NEXT:    movl    $1, 4792(%eax)

; CHECK-X86-32-NEXT:    movl    (%eax), %eax
; CHECK-X86-32-NEXT:    movl    (%eax), %eax

; CHECK-X86-32-NEXT:    movl    %ebp, %esp
; CHECK-X86-32-NEXT:    movl    %ebp, %esp

; CHECK-X86-32-NEXT:    popl    %ebp
; CHECK-X86-32-NEXT:    popl    %ebp

; CHECK-X86-32-NEXT:    .cfi_def_cfa %esp, 4
; CHECK-X86-32-NEXT:    .cfi_def_cfa %esp, 4

; CHECK-X86-32-NEXT:    retl
; CHECK-X86-32-NEXT:    retl




llvm/test/CodeGen/X86/stack-clash-small-alloc-medium-align.ll
Show First 20 Lines • Show All 100 Lines • ▼ Show 20 Lines
; CHECK-NEXT: .cfi_offset %rbx, -24
; CHECK-NEXT: .cfi_offset %rbx, -24

; CHECK-NEXT: movl  $1, (%rbx,%rdi,4)
; CHECK-NEXT: movl  $1, (%rbx,%rdi,4)

; CHECK-NEXT: movl  (%rbx), %ecx
; CHECK-NEXT: movl  (%rbx), %ecx

; CHECK-NEXT: movq  %rsp, %rax
; CHECK-NEXT: movq  %rsp, %rax

; CHECK-NEXT: leaq  15(,%rcx,4), %rcx
; CHECK-NEXT: leaq  15(,%rcx,4), %rcx

; CHECK-NEXT: andq  $-16, %rcx
; CHECK-NEXT: andq  $-16, %rcx

; CHECK-NEXT: subq  %rcx, %rax
; CHECK-NEXT: subq  %rcx, %rax

; CHECK-NEXT: cmpq  %rsp, %rax
; CHECK-NEXT: cmpq  %rsp, %rax

; CHECK-NEXT: jle .LBB3_3
; CHECK-NEXT: jge .LBB3_3

; CHECK-NEXT:.LBB3_2:                                # =>This Inner Loop Header: Depth=1
; CHECK-NEXT:.LBB3_2:                                # =>This Inner Loop Header: Depth=1

; CHECK-NEXT: movq  $0, (%rsp)
; CHECK-NEXT: movq  $0, (%rsp)

; CHECK-NEXT: subq  $4096, %rsp                     # imm = 0x1000
; CHECK-NEXT: subq  $4096, %rsp                     # imm = 0x1000

; CHECK-NEXT: cmpq  %rsp, %rax
; CHECK-NEXT: cmpq  %rsp, %rax

; CHECK-NEXT: jg  .LBB3_2
; CHECK-NEXT: jl  .LBB3_2

; CHECK-NEXT:.LBB3_3:
; CHECK-NEXT:.LBB3_3:

; CHECK-NEXT: andq  $-64, %rax
; CHECK-NEXT: andq  $-64, %rax

; CHECK-NEXT: movq  %rax, %rsp
; CHECK-NEXT: movq  %rax, %rsp

; CHECK-NEXT: movl  (%rax), %eax
; CHECK-NEXT: movl  (%rax), %eax

; CHECK-NEXT: leaq  -8(%rbp), %rsp
; CHECK-NEXT: leaq  -8(%rbp), %rsp

; CHECK-NEXT: popq  %rbx
; CHECK-NEXT: popq  %rbx

; CHECK-NEXT: popq  %rbp
; CHECK-NEXT: popq  %rbp

; CHECK-NEXT: .cfi_def_cfa %rsp, 8
; CHECK-NEXT: .cfi_def_cfa %rsp, 8

Show All 13 Lines