[ARM64][SEH] PR54879: Packed Unwind Info when Homing Int Param Regs

[zzheng]

Summary

Try to fix: https://github.com/llvm/llvm-project/issues/54879

When checking if the epilogue is symmetric to or a subset of the prologue,
unwinding NOP should be skipped.

[apazos]

llvm/lib/MC/MCWin64EH.cpp

652

Can you add a comment for checking the number of nops is 4, to be clear the pattern you are looking for

[efriedma]

llvm/lib/MC/MCWin64EH.cpp

640

We also use checkPackedEpilog() for to compute the epilogue offset in xdata records; we can't mess with the result for the sake of analyzing packed unwind. If you need a different analysis, please split it into a separate function, or use a flag. (Probably easier to understand if you split it into a separate function?)

855

Could use a comment explaining where "5" here comes from.

I think if H == 1, we need to reject PackedEpilogOffset == 0 and PackedEpilogOffset == 1. If the unwinder sees packed unwind, it will assume the epilogue doesn't contain any nops. So if the epilogue does in fact contain nops for some reason, the stack offsets will get messed up.

[mstorsjo]

llvm/lib/MC/MCWin64EH.cpp

640

+1, indeed. If we should tolerate a differing epilogue for the case of packed unwind info, we need a separate function to determine if an epilogue matches a prologue, for a specific packed form. Otherwise, we'd end up trying to share epilogues with prologues (for the non-packed format) where they actually don't match.

However, I'd like to take this whole issue back to the drawing board first; let me follow up on the bug report, to first exactly nail the expected behaviour.

llvm/test/MC/AArch64/seh-packed-unwind.s

509

If we would proceed with this patch, then we must also change the behaviour for the existing testcase for func5. For a specific packed form of unwind data, there can't be any ambiguity whether an epilog contains nops or not - we can't allow both cases. Let me follow up on the original bug report on that topic.

[zzheng]

As discussed in https://github.com/llvm/llvm-project/issues/54879, we decide not to pack unwind info when homing int param regs.
See: https://reviews.llvm.org/D125876

llvm/lib/MC/MCWin64EH.cpp

 }
 
 static int checkPackedEpilog(MCStreamer &streamer, WinEH::FrameInfo *info,
                              int PrologCodeBytes) {
   // Can only pack if there's one single epilog
   if (info->EpilogMap.size() != 1)
     return -1;
 
+  const std::vector<WinEH::Instruction> &Prolog = info->Instructions;
   const std::vector<WinEH::Instruction> &Epilog =
       info->EpilogMap.begin()->second;
 
   // Can pack if the epilog is a subset of the prolog but not vice versa
-  if (Epilog.size() > info->Instructions.size())
+  if (Epilog.size() > Prolog.size())
     return -1;
 
-  // Check that the epilog actually is a perfect match for the end (backwrds)
-  // of the prolog.
-  for (int I = Epilog.size() - 1; I >= 0; I--) {
-    if (info->Instructions[I] != Epilog[Epilog.size() - 1 - I])
-      return -1;
+  // Check that the epilog actually matches the prolog with nops skipped
+  // in both.
+  unsigned PNops = 0, ENops = 0;
+  for (int EI = Epilog.size() - 1, PI = 0; EI >= 0; ) {
+    assert(PI < Prolog.size() && "Prolog overflow!");
+    // Unwinding ops in prolog and epilog match.
+    if (Prolog[PI] == Epilog[EI]) {
+      PI++; EI--;
+      continue;
+    }
+    // Skip UOP_Nops in either prolog or epilog.
+    if (Prolog[PI].Operation == Win64EH::UOP_Nop) {
+      PNops++; PI++; continue;
+    }
+    if (Epilog[EI].Operation == Win64EH::UOP_Nop) {
+      ENops++; EI--; continue;
+    }
+    // If we reach here, we find unmatching ops that are NOT UOP_Nop.
   }
 
   // Check that the epilog actually is at the very end of the function,
   // otherwise it can't be packed.
   uint32_t DistanceFromEnd = (uint32_t)GetAbsDifference(
       streamer, info->FuncletOrFuncEnd, info->EpilogMap.begin()->first);
   if (DistanceFromEnd / 4 != Epilog.size())
     return -1;
 
-  int Offset = Epilog.size() == info->Instructions.size()
+  int PH = PNops == 4, EH = ENops == 4;
+  int Offset = (Epilog.size() - (EH * 4)) == (Prolog.size() - (PH * 4))
                    ? 0
                    : ARM64CountOfUnwindCodes(ArrayRef<WinEH::Instruction>(
-                         &info->Instructions[Epilog.size()],
-                         info->Instructions.size() - Epilog.size()));
-
+                         &Prolog[0], Prolog.size())) -
+                     ARM64CountOfUnwindCodes(ArrayRef<WinEH::Instruction>(
+                         &Epilog[0], Epilog.size()));
   // Check that the offset and prolog size fits in the first word; it's
   // unclear whether the epilog count in the extension word can be taken
   // as packed epilog offset.
   if (Offset > 31 || PrologCodeBytes > 124)
     return -1;
 
   info->EpilogMap.clear();
   return Offset;
 }
 
 static bool tryPackedUnwind(WinEH::FrameInfo *info, uint32_t FuncLength,
                             int PackedEpilogOffset) {
-  if (PackedEpilogOffset == 0) {
-    // Fully symmetric prolog and epilog, should be ok for packed format.
-    // For CR=3, the corresponding synthesized epilog actually lacks the
-    // SetFP opcode, but unwinding should work just fine despite that
-    // (if at the SetFP opcode, the unwinder considers it as part of the
-    // function body and just unwinds the full prolog instead).
-  } else if (PackedEpilogOffset == 1) {
-    // One single case of differences between prolog and epilog is allowed:
-    // The epilog can lack a single SetFP that is the last opcode in the
-    // prolog, for the CR=3 case.
-    if (info->Instructions.back().Operation != Win64EH::UOP_SetFP)
-      return false;
-  } else {
-    // Too much difference between prolog and epilog.
-    return false;
-  }
   unsigned RegI = 0, RegF = 0;
   int Predecrement = 0;
   enum {
     Start,
     Start2,
     IntRegs,
     FloatRegs,
     InputArgs,
if (RegI > 10 || RegF > 8)
     return false;
   if (StandaloneLR && FPLRPair)
     return false;
   if (FPLRPair && Location != End)
     return false;
   if (Nops != 0 && Nops != 4)
     return false;
   int H = Nops == 4;
+
+  if (PackedEpilogOffset == 0) {
+    // Fully symmetric prolog and epilog, should be ok for packed format.
+    // For CR=3, the corresponding synthesized epilog actually lacks the
+    // SetFP opcode, but unwinding should work just fine despite that
+    // (if at the SetFP opcode, the unwinder considers it as part of the
+    // function body and just unwinds the full prolog instead).
+  } else if ((PackedEpilogOffset == 1) ||
+             // Homing int param regs
+             (PackedEpilogOffset == 5 && H == 1)) {
+    // One single case of differences between prolog and epilog is allowed:
+    // The epilog can lack a single SetFP that is the last opcode in the
+    // prolog, for the CR=3 case.
+    if (info->Instructions.back().Operation != Win64EH::UOP_SetFP)
+      return false;
+  } else {
+    // Too much difference between prolog and epilog.
+    return false;
+  }
+
   int IntSZ = 8 * RegI;
   if (StandaloneLR)
     IntSZ += 8;
   int FpSZ = 8 * RegF; // RegF not yet decremented
   int SavSZ = (IntSZ + FpSZ + 8 * 8 * H + 0xF) & ~0xF;
   if (Predecrement != SavSZ)
     return false;
   if (FPLRPair && StackOffset < 16)

llvm/test/MC/AArch64/seh-packed-unwind.s

 // CHECK-NEXT:       stp x4, x5, [sp, #40]
 // CHECK-NEXT:       stp x2, x3, [sp, #24]
 // CHECK-NEXT:       stp x0, x1, [sp, #8]
 // CHECK-NEXT:       str x19, [sp, #-80]!
 // CHECK-NEXT:       end
 // CHECK-NEXT:     ]
 // CHECK-NEXT:   }
 // CHECK-NEXT:   RuntimeFunction {
+// CHECK-NEXT:     Function: func5b
+// CHECK-NEXT:     Fragment: No
+// CHECK-NEXT:     FunctionLength: 40
+// CHECK-NEXT:     RegF: 0
+// CHECK-NEXT:     RegI: 1
+// CHECK-NEXT:     HomedParameters: Yes
+// CHECK-NEXT:     CR: 0
+// CHECK-NEXT:     FrameSize: 112
+// CHECK-NEXT:     Prologue [
+// CHECK-NEXT:       sub sp, sp, #32
+// CHECK-NEXT:       stp x6, x7, [sp, #56]
+// CHECK-NEXT:       stp x4, x5, [sp, #40]
+// CHECK-NEXT:       stp x2, x3, [sp, #24]
+// CHECK-NEXT:       stp x0, x1, [sp, #8]
+// CHECK-NEXT:       str x19, [sp, #-80]!
+// CHECK-NEXT:       end
+// CHECK-NEXT:     ]
+// CHECK-NEXT:   }
+// CHECK-NEXT:   RuntimeFunction {
+// CHECK-NEXT:     Function: func5c
+// CHECK-NEXT:     Fragment: No
+// CHECK-NEXT:     FunctionLength: 60
+// CHECK-NEXT:     RegF: 0
+// CHECK-NEXT:     RegI: 2
+// CHECK-NEXT:     HomedParameters: Yes
+// CHECK-NEXT:     CR: 3
+// CHECK-NEXT:     FrameSize: 96
+// CHECK-NEXT:     Prologue [
+// CHECK-NEXT:       mov x29, sp
+// CHECK-NEXT:       stp x29, lr, [sp, #-16]!
+// CHECK-NEXT:       stp x6, x7, [sp, #64]
+// CHECK-NEXT:       stp x4, x5, [sp, #48]
+// CHECK-NEXT:       stp x2, x3, [sp, #32]
+// CHECK-NEXT:       stp x0, x1, [sp, #16]
+// CHECK-NEXT:       stp x19, x20, [sp, #-80]!
+// CHECK-NEXT:       end
+// CHECK-NEXT:     ]
+// CHECK-NEXT:   }
+// CHECK-NEXT:   RuntimeFunction {
 // CHECK-NEXT:     Function: func6
 // CHECK-NEXT:     Fragment: No
 // CHECK-NEXT:     FunctionLength: 24
 // CHECK-NEXT:     RegF: 0
 // CHECK-NEXT:     RegI: 0
 // CHECK-NEXT:     HomedParameters: No
 // CHECK-NEXT:     CR: 1
 // CHECK-NEXT:     FrameSize: 32
func5:
     .seh_stackalloc 32
     nop
     .seh_nop
     nop
     .seh_nop
     nop
     .seh_nop
     nop
     .seh_nop
     ldr x19, [sp], #80
     .seh_save_reg_x x19, 80
     .seh_endepilogue
     ret
     .seh_endproc
 
+// [PRi54879]: homing int param regs, but no nops in epilog
+func5b:
+    .seh_proc func5b
+    str x19, [sp, #-80]!
+    .seh_save_reg_x x19, 80
+    stp x0,  x1,  [sp, #8]
+    .seh_nop
+    stp x2,  x3,  [sp, #24]
+    .seh_nop
+    stp x4,  x5,  [sp, #40]
+    .seh_nop
+    stp x6,  x7,  [sp, #56]
+    .seh_nop
+    sub sp,  sp,  #32
+    .seh_stackalloc 32
+    .seh_endprologue
+    nop
+    .seh_startepilogue
+    add sp,  sp,  #32
+    .seh_stackalloc 32
+    ldr x19, [sp], #80
+    .seh_save_reg_x x19, 80
+    .seh_endepilogue
+    ret
+    .seh_endproc
+
+// [PRi54879]: homing int param regs, nops in epilog, only difference between
+// prolog and epilog is set_fp
+func5c:
+    .seh_proc func5c
+    stp x19, x20,[sp,#-0x50]!
+    .seh_save_regp_x x19, 0x50
+    stp x0, x1, [sp, #0x10]
+    .seh_nop
+    stp x2, x3, [sp, #0x20]
+    .seh_nop
+    stp x4, x5, [sp, #0x30]
+    .seh_nop
+    stp x6, x7, [sp, #0x40]
+    .seh_nop
+    stp fp, lr, [sp, #-0x10]!
+    .seh_save_fplr_x 0x10
+    mov fp, sp
+    .seh_set_fp
+    .seh_endprologue
+    //return success
+    mov w0, wzr
+    .seh_startepilogue
+    ldp fp, lr, [sp], #0x10
+    .seh_save_fplr_x 0x10
+    nop
+    .seh_nop
+    nop
+    .seh_nop
+    nop
+    .seh_nop
+    nop
+    .seh_nop
+    ldp x19, x20, [sp], #0x50
+    .seh_save_regp_x x19, 0x50
+    .seh_endepilogue
+    ret
+    .seh_endfunclet
+    .seh_endproc
+
 func6:
     .seh_proc func6
     str lr,       [sp, #-16]!
     .seh_save_reg_x lr, 16
     sub sp,  sp,  #16
     .seh_stackalloc 16
     .seh_endprologue
     nop