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

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

Thanks Ulrich and kevin for the review and comments.
Sorry for the late reply as i was off due to personal emergency.

I will have look at the suggestions and  implement the new changes.

Thanks,
Sangamesh



From:   "Ulrich Weigand" <uweigand@de.ibm.com>
To:     Sangamesh Mallayya/India/IBM@IBMIN
Cc:     kevinb@redhat.com (Kevin Buettner), gdb-patches@sourceware.org
Date:   09/12/2018 07:23 PM
Subject:        Re: [PATCH] Adding support for reding signal handler frame 
in AIX



Sangamesh Mallayya wrote:

> 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=20
> 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 =3D get_frame_arch (this_frame);
>    enum bfd_endian byte_order =3D gdbarch_byte_order (gdbarch);
>    int ti;
> +  CORE_ADDR old_pc =3D pc;
> =20
>    /* Check if we can use this trampoline.  */
>    if (tramp->validate && !tramp->validate (tramp, this_frame, &pc))
>      return 0;
> -
> +  if ((tramp->insn[0].bytes =3D=3D TRAMP_SENTINEL_INSN) &&
> +     (tramp->insn[1].bytes =3D=3D AIX_TRAMP_SENTINEL_INSN) &&
> +     (old_pc < 0x1000000))
> +    return pc;

I agree with Kevin that code like this shouldn't be in common code.

It looks like the underlying problem is that tramp-frame isn't a
good match for what you're trying to do: tramp-frame tries to
detect trampolines by matching well-known *code sequences*.
However, you don't actually have any code sequence to match,
but want to identify trampolines solely by their PC.  Since you
pass no code sequence to the tramp-frame matcher, it will actually
never match.

I'd suggest the best way forward is to not actually use tramp-frame
at all then, but just write your own matcher based directly on a
trad-frame cache.

An example to look at might be s390_stub_frame_unwind.  Along those
lines, you can implement a sniffer that checks for special PC value
(and possibly a backchain zero check in addition), and then implement
this_id and prev_register routines based on a trad-frame register
cache (you should be able to use the aix_sigtramp_cache routine in
your patch as-is for that part).

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com