Re: [PATCH] Adding support for reding signal handler frame in AIX

[Kevin Buettner <kevinb@redhat.com>]

On Wed, 29 Aug 2018 05:54:40 +0000
"Sangamesh Mallayya" <sangamesh.swamy@in.ibm.com> wrote:

> +   sigconext structure have the mstsave saved under the

typo, I think - s/sigconext/sigcontext/

> +   sc_jmpbuf.jmp_context. STKMIN(minimum stack size) is 56 for 32-bit
> +   processes, and iar offset under sc_jmpbuf.jmp_context is 40.
> +   ie offsetof(struct sigcontext, sc_jmpbuf.jmp_context.iar).
> +   so PC offset in this case is STKMIN+iar offset, which is 96 */

Please add a period at the end of this sentence.

> +
>  #define SIG_FRAME_PC_OFFSET 96
>  #define SIG_FRAME_LR_OFFSET 108
> +/* STKMIN+grp1 offset, which is 56+228=284 */
>  #define SIG_FRAME_FP_OFFSET 284
>  
> +/* 64 bit process
> +   STKMIN64  is 112 and iar offset is 312. So 112+312=424 */
> +#define SIG_FRAME_LR_OFFSET64 424 
> +/* STKMIN64+grp1 offset. 112+56=168 */
> +#define SIG_FRAME_FP_OFFSET64 168
>  
>  /* Core file support.  */
>  
> @@ -103,6 +118,104 @@
>    -1 /* vrsave_offset */
>  };
>  
> +static void 
> +aix_sigtramp_cache (struct frame_info *this_frame,
> +                    struct trad_frame_cache *this_cache,
> +                    CORE_ADDR func, LONGEST offset,
> +                    int bias)
> +{
> +  LONGEST backchain;
> +  CORE_ADDR base, frame_base, base_orig;
> +  CORE_ADDR regs;
> +  CORE_ADDR gpregs;
> +  CORE_ADDR fpregs;
> +  int i;
> +  struct gdbarch *gdbarch = get_frame_arch (this_frame);
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +  int wordsize = tdep->wordsize;
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +
> +  base = get_frame_register_unsigned (this_frame,
> +                                      gdbarch_sp_regnum (gdbarch));
> +  base_orig = base;
> +  if (wordsize == 4) {
> +    safe_read_memory_integer (base_orig + SIG_FRAME_FP_OFFSET + 8,
> +                              wordsize, byte_order, &backchain);
> +    base = (CORE_ADDR)backchain;
> +  } else {
> +      safe_read_memory_integer (base_orig + SIG_FRAME_FP_OFFSET64,
> +				wordsize, byte_order, &backchain);
> +      base = (CORE_ADDR)backchain;
> +  }

I'm wondering if the cast might be avoided by using
read_memory_typed_address in place of safe_read_memory_integer?
That way you'd be able to assign directly to base and also be
able to eliminate the "backchain" variable.

> +
> +  trad_frame_set_reg_value (this_cache, gdbarch_pc_regnum (gdbarch), func);
> +  trad_frame_set_reg_value (this_cache, gdbarch_sp_regnum (gdbarch), base);
> +
> +  if (wordsize == 4) {
> +    trad_frame_set_reg_addr (this_cache, tdep->ppc_lr_regnum,
> +                             base_orig + offset + 52 + 8);
> +  } else {
> +    trad_frame_set_reg_addr (this_cache, tdep->ppc_lr_regnum,
> +                             base_orig + offset + 320);
> +  } 
> +  trad_frame_set_id (this_cache, frame_id_build (base, func));
> +}
> +
> +static void
> +aix_sigtramp_cache_init (const struct tramp_frame *self,
> +			 struct frame_info *this_frame,
> +			 struct trad_frame_cache *this_cache,
> +			 CORE_ADDR func)
> +{
> +    struct gdbarch *gdbarch = get_frame_arch (this_frame);
> +    struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> +    if (tdep->wordsize == 4)
> +      aix_sigtramp_cache (this_frame, this_cache, func,
> +       			  0x38 /* Minimum stack size  */,
> +			  0);
> +    else
> +      aix_sigtramp_cache (this_frame, this_cache, func,
> +     			  0x70 /* Minimum stack size.  */,
> +			  0);
> +}
> +
> +static int
> +aix_validate_pc (const struct tramp_frame *self,
> +		 struct frame_info *this_frame,
> +		 CORE_ADDR *pc)
> +{
> +    struct gdbarch *gdbarch = get_frame_arch (this_frame);
> +    struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +    CORE_ADDR dest, frame_base;
> +    frame_base = get_frame_register_unsigned (this_frame,
> +				gdbarch_sp_regnum (gdbarch));
> +    if (tdep->wordsize == 4)  {
> +      if (*pc && *pc < AIX_TEXT_SEGMENT_BASE) {
> +        *pc = read_memory_unsigned_integer
> +              (frame_base + SIG_FRAME_PC_OFFSET + 8,
> +              tdep->wordsize, byte_order);
> +      }
> +    } else {
> +      if (*pc && *pc < AIX_TEXT_SEGMENT_BASE) {
> +        *pc = read_memory_unsigned_integer
> +	      (frame_base  + SIG_FRAME_LR_OFFSET64,
> +	      tdep->wordsize, byte_order);
> +      }
> +    }
> +    return 1;
> +} 
> +
> +static struct tramp_frame aix_sighandler_tramp_frame = {
> +  SIGTRAMP_FRAME,
> +  4,
> +  {
> +    { TRAMP_SENTINEL_INSN },
> +  },
> +  aix_sigtramp_cache_init,
> +  aix_validate_pc
> +};
>  
>  /* Supply register REGNUM in the general-purpose register set REGSET
>     from the buffer specified by GREGS and LEN to register cache
> @@ -1083,6 +1196,7 @@
>    set_gdbarch_auto_wide_charset (gdbarch, rs6000_aix_auto_wide_charset);
>  
>    set_solib_ops (gdbarch, &solib_aix_so_ops);
> +  tramp_frame_prepend_unwinder (gdbarch, &aix_sighandler_tramp_frame);
>  }
>  
>  void
> --- ./gdb/tramp-frame.c_orig	2018-08-27 03:25:49 +0000
> +++ ./gdb/tramp-frame.c	2018-08-27 03:26:24 +0000
> @@ -132,6 +132,12 @@
>       false on HPUX which has a signal trampoline that has a name; it can
>       also be false when using an alternative signal stack.  */
>    func = tramp_frame_start (tramp, this_frame, pc);
> +  #if defined (_AIX)
> +  if (pc <= 0x10000000) {
> +      tramp->validate (tramp, this_frame, &pc);
> +      func = pc;
> +  }
> +  #endif

We don't want to be putting OS specific ifdefs here.  It seems to me
that the pc <= 0x10000000 test could be put in the validate code if in
fact it's needed at all.  The return value of that call to validate is
not being checked, so that means that you're calling it to obtain
func.  But func should be correctly set by the call to
tramp_frame_start, earlier on.  Note, too, that tramp_frame_start
calls the validate method, so it seems to me that it ought to be
possible to get it set as needed by some suitable definition of
validate.

Kevin