[PATCH] D65653: [AArch64] Change location of frame-record within callee-save area.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen marked 6 inline comments as done.
sdesmalen added inline comments.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:88
 //
 // FIXME: also explain the redzone concept.
 // FIXME: also explain the concept of reserved call frames.
----------------
kristof.beyls wrote:
> I wonder if it would be a good idea to record the rationale for (some of the) order in which specific blocks are put in the frame record?
> I think it would be useful to extend this comment, stating that the frame record is placed specifically to be able to make use of VL-scaled addressing modes to directly access SVE objects from the frame-pointer.
> I feel the frame layout code is pretty complex, and therefore it helps to spell out the higher-level design rationales at the beginning of this file in a comment.
Good point. I have added some words to describe the rationale.


================
Comment at: test/CodeGen/AArch64/wineh-try-catch-realign.ll:13
+; CHECK:      stp     x29, x30, [sp, #-32]!
+; CHECK-NEXT: stp     x28, x19, [sp, #16]
 ; CHECK-NEXT: add     x0, x19, #64
----------------
efriedma wrote:
> sdesmalen wrote:
> > efriedma wrote:
> > > I don't think this is legal. On Windows, paired stores in the prologue must be in the form "stp xN, xN+1, [...]"; otherwise, there is no way to represent the store in the unwind format. (See https://docs.microsoft.com/en-us/cpp/build/arm64-exception-handling?view=vs-2019#unwind-codes )
> > The merging of two separate STR instructions into one STP seems to done by the LoadStoreOptimizer, not PEI. The unwind information generated seems correct.
> > 
> > Running with `stop-after=prologepilog`:
> > ```    frame-setup STRXui killed $x28, $sp, 2 :: (store 8 into %stack.2)
> >     frame-setup SEH_SaveReg 28, 16
> >     frame-setup STRXui killed $x19, $sp, 3 :: (store 8 into %stack.1)
> >     frame-setup SEH_SaveReg 19, 24```
> > 
> > Running `readobj --unwind` on the binary produced by this test gives:
> > ```      Prologue [
> >         0xd003              ; str x19, [sp, #24]
> >         0xd242              ; str x28, [sp, #16]
> >         0x83                ; stp x29, x30, [sp, #-32]!
> >         0xe4                ; end
> >       ]```
> Windows unwinding determines whether an instruction is part of the prologue by computing the distance in bytes between the current pc and the entry point.  So if the real prologue is shorter than the encoded prologue, the exception unwinder could be tricked into skipping important steps in unwinding.
> 
> Even if your patch isn't causing this issue, it's more likely to come up with your patch, I think.
Thanks for pointing this out! I've created D65817 to address that issue.


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

https://reviews.llvm.org/D65653