[PATCH] D126861: [RISCV] Fix missing stack pointer recover

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D126861#3560149 <https://reviews.llvm.org/D126861#3560149>, @kito-cheng wrote:

> In D126861#3553679 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126861/new/

https://reviews.llvm.org/D126861