[PATCH] D92803: [SystemZFrameLowering] Make sure R1 holding the backchain is not corrupted by probing

Thu Dec 10 02:13:14 PST 2020

uweigand accepted this revision.
uweigand added a comment.
This revision is now accepted and ready to land.

This suggestion wasn't for performance reasons, but correctness.  In theory, when using the backchain, it should be updated on every change to %r15 so that %r15 at all times points to a valid backchain value.  Otherwise, unwinding using the backchain will randomly fail when starting at some PC where the stack pointer has been updated but the backchain not yet written.

However, thinking about this again, it probably doesn't matter in this case since there's nothing in this loop where we might be triggering unwinding.  In any case, GCC also doesn't seem to update the backchain each time through the loop, so we're probably fine without.

So I think this patch LGTM now.

This suggestion:

> // The back chain is stored topmost with packed-stack.
> int Offset = usePackedStack(MF) ? SystemZMC::CallFrameSize - 8 : 0;
>
> Given that this is now duplicated, maybe it would make sense to have that logic in a separate function "getBackchainOffset(MF)" or the like.

is still valid, but can be done in a separate patch.   Looks like there are other places where this offset calculation is duplicated, they should really all be merged.

B.t.w. it seems lowerDYNAMIC_STACKALLOC and lowerSTACKRESTORE always use 0 as backchain offset, so those would be incorrect with the kernel backchain?  I guess the kernel rarely uses dynamic stack allocation, but it seems those two places still ought to be fixed.

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

https://reviews.llvm.org/D92803