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

["Sangamesh Mallayya" <sangamesh.swamy@in.ibm.com>]

Hi Kevin,

Thanks a lot for review and comments.

> > +   sigconext structure have the mstsave saved under the
> 
> typo, I think - s/sigconext/sigcontext/
> 

Yes. Will correct this.

> > +   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.
> 

Sure.

> > +
> >  #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.
> 

read_memory_typed_address requires the second parameter of *type.
Need to check and confirm how easy to get type, and does it required more 
function calls
than just calling safe_read_memory_integer would be good. 


> > +
> > +  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.
> 

Yes. Thanks!
Earlier code was calling validate function twice which wasn't required.
We can remove that AIX ifdef and i have made the below changes. Rest all 
are same.
Let me know your view on this.

# diff -u tramp-frame.c_orig tramp-frame.c
--- tramp-frame.c_orig  2018-08-27 03:25:49 +0000
+++ tramp-frame.c       2018-09-07 10:20:09 +0000
@@ -86,11 +86,15 @@
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   int ti;
+  CORE_ADDR old_pc = pc;
 
   /* Check if we can use this trampoline.  */
   if (tramp->validate && !tramp->validate (tramp, this_frame, &pc))
     return 0;
-
+  if ((tramp->insn[0].bytes == TRAMP_SENTINEL_INSN) &&
+     (tramp->insn[1].bytes == AIX_TRAMP_SENTINEL_INSN) &&
+     (old_pc < 0x1000000))
+    return pc;


--- tramp-frame.h_orig  2018-09-07 10:03:34 +0000
+++ tramp-frame.h       2018-09-07 10:18:50 +0000
@@ -42,6 +42,7 @@
 /* Magic instruction that to mark the end of the signal trampoline
    instruction sequence.  */
 #define TRAMP_SENTINEL_INSN ((LONGEST) -1)
+#define AIX_TRAMP_SENTINEL_INSN ((LONGEST) -2) <====== New definition to 
make sure this check is done only when running it on AIX.

 
static struct tramp_frame aix_sighandler_tramp_frame = {
  SIGTRAMP_FRAME,
  4,
  {
    { TRAMP_SENTINEL_INSN, -1}, 
    { AIX_TRAMP_SENTINEL_INSN, -2},     <====== New definition to make 
sure this check is done only when running it on AIX.
  },
  aix_sigtramp_cache_init,
  aix_validate_pc
};