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

[Kevin Buettner <kevinb@redhat.com>]

On Mon, 10 Sep 2018 12:02:38 +0530
"Sangamesh Mallayya" <sangamesh.swamy@in.ibm.com> wrote:

> > > --- ./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;

Even though you've gotten rid of the ifdefs, my earlier comment, which
I've left intact above, still applies.  We should not be putting
OS/target specific code into tramp-frame.c.  I still think it should
be possible to do what you want via a suitable definition of the
validate method for AIX.  I.e.  the place to make these changes is in
rs6000-aix-tdep.c.

If we don't have the infrastructure to do what you need, then the
infrastructure needs to be extended so that you have the requisite
hooks to be able to place the OS dependent code into the correct tdep
file.  However, before that happens, I'd like to understand why it's not
possible to do what you require via some change to AIX's validate
method.

Kevin