[RISCV] Fix missing stack pointer recover

[kito-cheng]

Summary

In order to make sure the stack point is right through the EH region,
we also need to restore stack pointer from the frame pointer if we
don't preserve stack space within prologue/epilogue for outgoing variables,
normally it's just checking the variable sized object is present or not
is enough, but we also don't preserve that at prologue/epilogue when
have vector objects in stack.

Example to show what happened:

try {
  sp adjust for outgoing args. // 1. Sp changed.
  func_call  // 2. Exception raised
  sp restore // Oh, not restored
} catch {
  // 3. And now we are here.
}

// 4. Prepare to return!, restore return address from stack, but...sp is wrong.
// 5. Screw up!

[reames]

I'm missing something here.

If we have a frame pointer, shouldn't restoring SP simply be a register move? Why bother with all of the offset adjustments in this case at all? Shouldn't we be able to structure the code as:
if (hasFP) {

copy SP to FP

} else {

do it the hard way

}

[kito-cheng]

In D126861#3553679, @reames wrote:

I'm missing something here.

If we have a frame pointer, shouldn't restoring SP simply be a register move? Why bother with all of the offset adjustments in this case at all? Shouldn't we be able to structure the code as:
if (hasFP) {

copy SP to FP

} else {

do it the hard way

}

I think that might less intuitive on the sp recover code here, ideally we could restore sp by moving s0 to sp and addi sp, sp, 32 could be removed like this,:

mv sp, s0, -32
ld ra, -8(sp) # 8-byte Folded Reload
ld s0, -16(sp) # 8-byte Folded Reload
ld s1, -24(sp) # 8-byte Folded Reload

but we can't do that in practice since that is not signal-safe/interrupt-safe, when the sp changing means we already released the space of stack, and then anything could happen on those area.

Give a practical example here, we got a signal after execute mv sp, s0, -32, and then kernel start to prepare the arguments and put into stack, ok - you could imagine what happened: the stack is corrupt, ra, s0 and s1 in stack might be corrupt, but technically it's not corrupt since you already move the stack pointer that means we release that, and that's same for the interrupt situation.

mv sp, s0, -32
; Get signal or interrupt here!
ld ra, -8(sp) # 8-byte Folded Reload   ; Signal handler or interrupt handler might overwrite this area,
ld s0, -16(sp) # 8-byte Folded Reload ; because we already released the stack space!
ld s1, -24(sp) # 8-byte Folded Reload

[reames]

In D126861#3560149, @kito-cheng wrote:

In D126861#3553679, @reames wrote:

I'm missing something here.

If we have a frame pointer, shouldn't restoring SP simply be a register move? Why bother with all of the offset adjustments in this case at all? Shouldn't we be able to structure the code as:
if (hasFP) {

copy SP to FP

} else {

do it the hard way

}

I think that might less intuitive on the sp recover code here, ideally we could restore sp by moving s0 to sp and addi sp, sp, 32 could be removed like this,:

mv sp, s0, -32
ld ra, -8(sp) # 8-byte Folded Reload
ld s0, -16(sp) # 8-byte Folded Reload
ld s1, -24(sp) # 8-byte Folded Reload

but we can't do that in practice since that is not signal-safe/interrupt-safe, when the sp changing means we already released the space of stack, and then anything could happen on those area.

Give a practical example here, we got a signal after execute mv sp, s0, -32, and then kernel start to prepare the arguments and put into stack, ok - you could imagine what happened: the stack is corrupt, ra, s0 and s1 in stack might be corrupt, but technically it's not corrupt since you already move the stack pointer that means we release that, and that's same for the interrupt situation.

mv sp, s0, -32
; Get signal or interrupt here!
ld ra, -8(sp) # 8-byte Folded Reload   ; Signal handler or interrupt handler might overwrite this area,
ld s0, -16(sp) # 8-byte Folded Reload ; because we already released the stack space!
ld s1, -24(sp) # 8-byte Folded Reload

Kito, I don't get your comment. I'd expect the CSR restores to be happening before stack adjustment. Are you saying that there's some other code which tries to insert restores in the middle of the existing stack adjustment sequence?

If so, that's an invariant which really should be documented in comments.

[kito-cheng]

Kito, I don't get your comment. I'd expect the CSR restores to be happening before stack adjustment.

It's too late to do stack adjustment after CSR restore by moving fp to sp, because s0(fp) is CSR.

addi sp, s0, -32
ld ra, 24(sp) # 8-byte Folded Reload
ld s0, 16(sp) # 8-byte Folded Reload <----- s0 is restore here 
ld s1, 8(sp) # 8-byte Folded Reload
addi sp, sp, 32
ret

Are you saying that there's some other code which tries to insert restores in the middle of the existing stack adjustment sequence?

No, I mean signal and interrupt might raise during epilogue, and that will keep using sp, and adjust that in the handler, so sp must adjust after CSR restores.

[rogfer01]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

648

I assume that the exceptional path doesn't execute the add sp, sp, 32 after call _Z3fooiiiiiiiiiiPi@plt, instead we directly land to .LBB0_2: # %lpad. If this is th case, it does leaves the stack unaligned.

However, the expansion of !hasReservedCallFrame(MF) results in checking hasFP(MF) && hasRVVFrameObject(MF).

Isn't this an issue that happens irrespective of vectors and it is caused by having a fp? E.g. when using something like -fno-omit-frame-pointer.

[rogfer01]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

648

Answering myself here: when not using v then we account the callee arguments as part of the caller stack frame.

[rogfer01]

LGTM. Thanks @kito-cheng

[kito-cheng]

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

648

Yeah, your understating is right.

BTW, same issue could be happen without v in theory (without this patch), but that situation will fixed by clang, it will emit stack pointer store/restore intrinsic...use this case to demonstrate:

simple.cpp
$ clang simple.cpp -o - -S -O -emit-llvm -march=rv64gc -target riscv64-unknown-linux-gnu

void foo(int, int, int, int, int, int, int, int, int, int, int *);

bool check_frame_variant(int x) {
  int exception_value = 1;
  try {
      int yy[x];
      foo(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, &yy[0]);
  } catch (int i) {
    exception_value = i;
  }
  return exception_value;
}

Generated LLVM IR:

define dso_local noundef zeroext i1 @_Z19check_frame_varianti(i32 noundef signext %x) local_unnamed_addr #0 personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
entry:
  %0 = zext i32 %x to i64
  %1 = call i8* @llvm.stacksave()   <--- clang emit a sp back up here
  %vla = alloca i32, i64 %0, align 8
  invoke void @_Z3fooiiiiiiiiiiPi(i32 noundef signext 0, i32 noundef signext 0, i32 noundef signext 0, i32 noundef signext 0, i32 noundef signext 0, i32 noundef signext 0, i32 noundef signext 0, i32 noundef signext 0, i32 noundef 0, i32 noundef 0, i32* noundef nonnull %vla)
          to label %invoke.cont unwind label %lpad

invoke.cont:
  call void @llvm.stackrestore(i8* %1) <-- and a sp restore here!!!!!
  br label %try.cont

[thakis]

Looks like this breaks check-llvm: http://45.33.8.238/linux/78124/step_12.txt

Please take a look, and revert for now if it takes a while to fix.

[thakis]

Also broken e.g. here https://lab.llvm.org/buildbot/#/builders/105/builds/26529

[kito-cheng]

Fixed via this commit, my bad, I didn't upload those testcase update...

[thakis]

Thanks!

Please run tests locally before committing :)

[efriedma]

I'm a bit concerned about the example given in the commit message. In general, if we catch an exception from a call, we should immediately free the memory allocated for the call arguments. Otherwise, if we throw exceptions in a loop, we can allocate an arbitrary amount of memory, which eventually leads to a stack overflow.

[kito-cheng]

@efriedma Thanks, that's good catch, created another issue to tracking that[1]!

[1] https://github.com/llvm/llvm-project/issues/56008

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

void RISCVFrameLowering::emitEpilogue(MachineFunction &MF,
   uint64_t StackSize = getStackSizeWithRVVPadding(MF);
   uint64_t RealStackSize = StackSize + RVFI->getLibCallStackSize();
   uint64_t FPOffset = RealStackSize - RVFI->getVarArgsSaveSize();
   uint64_t RVVStackSize = RVFI->getRVVStackSize();
 
   // Restore the stack pointer using the value of the frame pointer. Only
   // necessary if the stack pointer was modified, meaning the stack size is
   // unknown.
-  if (RI->hasStackRealignment(MF) || MFI.hasVarSizedObjects()) {
+  //
+  // In order to make sure the stack point is right through the EH region,
+  // we also need to restore stack pointer from the frame pointer if we
+  // don't preserve stack space within prologue/epilogue for outgoing variables,
+  // normally it's just checking the variable sized object is present or not
+  // is enough, but we also don't preserve that at prologue/epilogue when
+  // have vector objects in stack.
+  if (RI->hasStackRealignment(MF) || MFI.hasVarSizedObjects() ||
+      !hasReservedCallFrame(MF)) {
     assert(hasFP(MF) && "frame pointer should not have been eliminated");
     adjustReg(MBB, LastFrameDestroy, DL, SPReg, FPReg, -FPOffset,
               MachineInstr::FrameDestroy);
   } else {
     if (RVVStackSize)
       adjustStackForRVV(MF, MBB, LastFrameDestroy, DL, RVVStackSize,
                         MachineInstr::FrameDestroy);
   }

llvm/test/CodeGen/RISCV/miss-sp-restore-eh.ll

 ; CHECK-NEXT:    sext.w a1, a1
 ; CHECK-NEXT:    li a2, 1
 ; CHECK-NEXT:    bne a1, a2, .LBB0_4
 ; CHECK-NEXT:  # %bb.3: # %catch
 ; CHECK-NEXT:    call __cxa_begin_catch@plt
 ; CHECK-NEXT:    lw s1, 0(a0)
 ; CHECK-NEXT:    call __cxa_end_catch@plt
 ; CHECK-NEXT:    mv a0, s1
+; CHECK-NEXT:    addi sp, s0, -32
 ; CHECK-NEXT:    ld ra, 24(sp) # 8-byte Folded Reload
 ; CHECK-NEXT:    ld s0, 16(sp) # 8-byte Folded Reload
 ; CHECK-NEXT:    ld s1, 8(sp) # 8-byte Folded Reload
 ; CHECK-NEXT:    addi sp, sp, 32
 ; CHECK-NEXT:    ret
 ; CHECK-NEXT:  .LBB0_4: # %ehcleanup
 ; CHECK-NEXT:    call _Unwind_Resume@plt
 entry: