[PATCH] gdb/s390: Add packed-stack support to the backchain unwinder

[PATCH] gdb/s390: Add packed-stack support to the backchain unwinder

[Ilya Leoshkevich]

Currently it's not possible to unwind s390 kernel stacks past eBPF
progs.  There is no DWARF debug info or symbols for eBPF progs,
therefore doing so requires using the backchain unwinder.  The kernel
uses the packed-stack ABI, which the backchain unwinder does not
support.

As far as unwinding is concerned, the difference between the
"regular" ABI and the packed-stack ABI are frame offsets for backchain,
stack pointer and program counter.

Parameterize the existing code that does backchain unwinding by these
three values; try the "regular" ones first, and the packed-stack ones
as a fallback.
---
Regtested on s390x-redhat-linux.  Ok for master?

 gdb/s390-tdep.c | 72 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 48 insertions(+), 24 deletions(-)

diff --git a/gdb/s390-tdep.c b/gdb/s390-tdep.c
index 54b5c89e5e3..2d5c3f99dd8 100644
--- a/gdb/s390-tdep.c
+++ b/gdb/s390-tdep.c
@@ -2520,32 +2520,24 @@ s390_prologue_frame_unwind_cache (frame_info_ptr this_frame,
   return 1;
 }
 
-/* Unwind THIS_FRAME and write the information into unwind cache INFO using
-   back chain unwinding.  Helper for s390_frame_unwind_cache.  */
+/* Unwind THIS_FRAME using back chain unwinding for the ABI described by
+   WORD_SIZE, BYTE_ORDER, BACKCHAIN_IDX, SP_IDX and PC_IDX.  If successful,
+   write the information into unwind cache INFO and return true, otherwise
+   return false.  Helper for s390_backchain_frame_unwind_cache.  */
 
-static void
-s390_backchain_frame_unwind_cache (frame_info_ptr this_frame,
-				   struct s390_unwind_cache *info)
+static bool
+try_backchain (frame_info_ptr this_frame, struct s390_unwind_cache *info,
+	       int word_size, enum bfd_endian byte_order, int backchain_idx,
+	       int sp_idx, int pc_idx)
 {
-  struct gdbarch *gdbarch = get_frame_arch (this_frame);
-  int word_size = gdbarch_ptr_bit (gdbarch) / 8;
-  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   CORE_ADDR backchain;
-  ULONGEST reg;
   LONGEST sp, tmp;
-  int i;
-
-  /* Set up ABI call-saved/call-clobbered registers.  */
-  for (i = 0; i < S390_NUM_REGS; i++)
-    if (!s390_register_call_saved (gdbarch, i))
-      info->saved_regs[i].set_unknown ();
-
-  /* CC is always call-clobbered.  */
-  info->saved_regs[S390_PSWM_REGNUM].set_unknown ();
+  ULONGEST reg;
 
   /* Get the backchain.  */
   reg = get_frame_register_unsigned (this_frame, S390_SP_REGNUM);
-  if (!safe_read_memory_integer (reg, word_size, byte_order, &tmp))
+  if (!safe_read_memory_integer (reg + backchain_idx * word_size, word_size,
+				 byte_order, &tmp))
     tmp = 0;
   backchain = (CORE_ADDR) tmp;
 
@@ -2554,15 +2546,17 @@ s390_backchain_frame_unwind_cache (frame_info_ptr this_frame,
      save area pointed to by the backchain in fact links back to
      the save area.  */
   if (backchain != 0
-      && safe_read_memory_integer (backchain + 15*word_size,
-				   word_size, byte_order, &sp)
+      && safe_read_memory_integer (backchain + sp_idx * word_size, word_size,
+				   byte_order, &sp)
       && (CORE_ADDR)sp == backchain)
     {
       /* We don't know which registers were saved, but it will have
 	 to be at least %r14 and %r15.  This will allow us to continue
 	 unwinding, but other prev-frame registers may be incorrect ...  */
-      info->saved_regs[S390_SP_REGNUM].set_addr (backchain + 15*word_size);
-      info->saved_regs[S390_RETADDR_REGNUM].set_addr (backchain + 14*word_size);
+      info->saved_regs[S390_SP_REGNUM].set_addr (backchain
+						 + sp_idx * word_size);
+      info->saved_regs[S390_RETADDR_REGNUM].set_addr (backchain
+						      + pc_idx * word_size);
 
       /* Function return will set PC to %r14.  */
       info->saved_regs[S390_PSWA_REGNUM]
@@ -2570,10 +2564,40 @@ s390_backchain_frame_unwind_cache (frame_info_ptr this_frame,
 
       /* We use the current value of the frame register as local_base,
 	 and the top of the register save area as frame_base.  */
-      info->frame_base = backchain + 16*word_size + 32;
+      info->frame_base = backchain + 16 * word_size + 32;
       info->local_base = reg;
+
+      return true;
     }
 
+  return false;
+}
+
+/* Unwind THIS_FRAME and write the information into unwind cache INFO using
+   back chain unwinding.  Helper for s390_frame_unwind_cache.  */
+
+static void
+s390_backchain_frame_unwind_cache (frame_info_ptr this_frame,
+				   struct s390_unwind_cache *info)
+{
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
+  int word_size = gdbarch_ptr_bit (gdbarch) / 8;
+  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+  int i;
+
+  /* Set up ABI call-saved/call-clobbered registers.  */
+  for (i = 0; i < S390_NUM_REGS; i++)
+    if (!s390_register_call_saved (gdbarch, i))
+      info->saved_regs[i].set_unknown ();
+
+  /* CC is always call-clobbered.  */
+  info->saved_regs[S390_PSWM_REGNUM].set_unknown ();
+
+  /* Try the regular ABI.  */
+  if (!try_backchain (this_frame, info, word_size, byte_order, 0, 15, 14))
+    /* Try the packed stack ABI.  */
+    try_backchain (this_frame, info, word_size, byte_order, 19, 18, 17);
+
   info->func = get_frame_pc (this_frame);
 }
 
-- 
2.41.0

Re: [PATCH] gdb/s390: Add packed-stack support to the backchain unwinder

[Keith Seitz]

On 10/26/23 17:51, Ilya Leoshkevich wrote:
> 
> As far as unwinding is concerned, the difference between the
> "regular" ABI and the packed-stack ABI are frame offsets for backchain,
> stack pointer and program counter.
> 
> Parameterize the existing code that does backchain unwinding by these
> three values; try the "regular" ones first, and the packed-stack ones
> as a fallback.

Thanks for the patch. I'm only very vaguely familiar with s390, but your
patch has gone without comment for several weeks, and I wanted to jump
in and give it at least a preliminary review.

> Regtested on s390x-redhat-linux.  Ok for master?

I've also regtested it on s390x without any issue. I only have one
really minor nit (below).

> diff --git a/gdb/s390-tdep.c b/gdb/s390-tdep.c
> index 54b5c89e5e3..2d5c3f99dd8 100644
> --- a/gdb/s390-tdep.c
> +++ b/gdb/s390-tdep.c
> @@ -2520,32 +2520,24 @@ s390_prologue_frame_unwind_cache (frame_info_ptr this_frame,
>     return 1;
>   }
>   
> -/* Unwind THIS_FRAME and write the information into unwind cache INFO using
> -   back chain unwinding.  Helper for s390_frame_unwind_cache.  */
> +/* Unwind THIS_FRAME using back chain unwinding for the ABI described by
> +   WORD_SIZE, BYTE_ORDER, BACKCHAIN_IDX, SP_IDX and PC_IDX.  If successful,
> +   write the information into unwind cache INFO and return true, otherwise
> +   return false.  Helper for s390_backchain_frame_unwind_cache.  */
>   
> -static void
> -s390_backchain_frame_unwind_cache (frame_info_ptr this_frame,
> -				   struct s390_unwind_cache *info)
> +static bool
> +try_backchain (frame_info_ptr this_frame, struct s390_unwind_cache *info,
> +	       int word_size, enum bfd_endian byte_order, int backchain_idx,
> +	       int sp_idx, int pc_idx)
>   {
> -  struct gdbarch *gdbarch = get_frame_arch (this_frame);
> -  int word_size = gdbarch_ptr_bit (gdbarch) / 8;
> -  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>     CORE_ADDR backchain;
> -  ULONGEST reg;
>     LONGEST sp, tmp;
> -  int i;
> -
> -  /* Set up ABI call-saved/call-clobbered registers.  */
> -  for (i = 0; i < S390_NUM_REGS; i++)
> -    if (!s390_register_call_saved (gdbarch, i))
> -      info->saved_regs[i].set_unknown ();
> -
> -  /* CC is always call-clobbered.  */
> -  info->saved_regs[S390_PSWM_REGNUM].set_unknown ();
> +  ULONGEST reg;
>   
>     /* Get the backchain.  */
>     reg = get_frame_register_unsigned (this_frame, S390_SP_REGNUM);

Nowadays, we typically tend to favor declaring variables where they're
used, if possible. Merging these two statements (ULONGEST reg = ...)
would follow that.

> -  if (!safe_read_memory_integer (reg, word_size, byte_order, &tmp))
> +  if (!safe_read_memory_integer (reg + backchain_idx * word_size, word_size,
> +				 byte_order, &tmp))
>       tmp = 0;
>     backchain = (CORE_ADDR) tmp;
>   
> @@ -2554,15 +2546,17 @@ s390_backchain_frame_unwind_cache (frame_info_ptr this_frame,
>        save area pointed to by the backchain in fact links back to
>        the save area.  */
>     if (backchain != 0
> -      && safe_read_memory_integer (backchain + 15*word_size,
> -				   word_size, byte_order, &sp)
> +      && safe_read_memory_integer (backchain + sp_idx * word_size, word_size,
> +				   byte_order, &sp)
>         && (CORE_ADDR)sp == backchain)
>       {
>         /* We don't know which registers were saved, but it will have
>   	 to be at least %r14 and %r15.  This will allow us to continue
>   	 unwinding, but other prev-frame registers may be incorrect ...  */
> -      info->saved_regs[S390_SP_REGNUM].set_addr (backchain + 15*word_size);
> -      info->saved_regs[S390_RETADDR_REGNUM].set_addr (backchain + 14*word_size);
> +      info->saved_regs[S390_SP_REGNUM].set_addr (backchain
> +						 + sp_idx * word_size);
> +      info->saved_regs[S390_RETADDR_REGNUM].set_addr (backchain
> +						      + pc_idx * word_size);
>   

Thank you for fixing the above (and below) formatting errors while
you were at it. :-)


>         /* Function return will set PC to %r14.  */
>         info->saved_regs[S390_PSWA_REGNUM]
> @@ -2570,10 +2564,40 @@ s390_backchain_frame_unwind_cache (frame_info_ptr this_frame,
>   
>         /* We use the current value of the frame register as local_base,
>   	 and the top of the register save area as frame_base.  */
> -      info->frame_base = backchain + 16*word_size + 32;
> +      info->frame_base = backchain + 16 * word_size + 32;
>         info->local_base = reg;
> +
> +      return true;
>       }
>   
> +  return false;
> +}
> +
> +/* Unwind THIS_FRAME and write the information into unwind cache INFO using
> +   back chain unwinding.  Helper for s390_frame_unwind_cache.  */
> +
> +static void
> +s390_backchain_frame_unwind_cache (frame_info_ptr this_frame,
> +				   struct s390_unwind_cache *info)
> +{
> +  struct gdbarch *gdbarch = get_frame_arch (this_frame);
> +  int word_size = gdbarch_ptr_bit (gdbarch) / 8;
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  int i;
> +
> +  /* Set up ABI call-saved/call-clobbered registers.  */
> +  for (i = 0; i < S390_NUM_REGS; i++)
> +    if (!s390_register_call_saved (gdbarch, i))
> +      info->saved_regs[i].set_unknown ();
> +
> +  /* CC is always call-clobbered.  */
> +  info->saved_regs[S390_PSWM_REGNUM].set_unknown ();
> +
> +  /* Try the regular ABI.  */
> +  if (!try_backchain (this_frame, info, word_size, byte_order, 0, 15, 14))
> +    /* Try the packed stack ABI.  */
> +    try_backchain (this_frame, info, word_size, byte_order, 19, 18, 17);
> +
>     info->func = get_frame_pc (this_frame);
>   }

Do we need to worry about -Wunused-result or similar in this second call 
to try_backchain()?

If you'll forgive me for asking, but is there anyway to exercise this 
code (in the testsuite)?

I've (naively) played a bit with some simple executables built with
-g vs  -mpacked-stack, and I don't see any differences before/after
this patch in testsuite runs. Is it possible to write something that
isn't too burdensome?

Otherwise, this looks okay to me.

Reviewed-by: Keith Seitz <keiths@redhat.com>

Keith