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

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

[Sangamesh Mallayya]

[-- Attachment #1: Type: text/plain, Size: 3617 bytes --]

Hi All,

Attaching the patch for adding signal handler support in AIX.

If gdb is debugging an application which has a signal handler and reaches 
the signal handler frame, then we need to read the back chain address from 
sigconext saved on the 
stack, similarly the LR.

As backchain at an offset 0 will be 0, because we will have sigconext 
saved after the minimum stack size. 
So, correct backchain will be at an offset after minimum stack and the LR 
at an offset 8 will be of the
signal millicode address. If the back chain pointer is NULL and the LR 
field is in the kernel 
segment(ex. 0x00004a14) then we can probably assume we are in a signal 
handler.

This can be demonstrated using the below sample program.

# cat aix-sighandle.c
#include <stdio.h>
#include <signal.h>
#include <string.h>

void sig_handle_aix(int signo)
{
  printf("signal is: %d\n", signo);
}

void foo()
{
  char *ptr; 
  signal(SIGSEGV, sig_handle_aix);
  strcpy(ptr, "signal");
}

int main()
{
  foo();
}


without patch
-----------------

Reading symbols from ./aix-sighandle...done.
(gdb) br sig_handle_aix
Breakpoint 1 at 0x100006bc: file aix-sighandle.c, line 7.
(gdb) r
Starting program: aix-sighandle 

Program received signal SIGSEGV, Segmentation fault.
0x0000000100000748 in foo () at /home/binutils-gdb/gdb/aix-sighandle.c:14
14        strcpy(ptr, "signal");
(gdb) c
Continuing.

Breakpoint 1, sig_handle_aix (signo=11) at 
/home/binutils-gdb/gdb/aix-sighandle.c:7
7         printf("signal is: %d\n", signo);
(gdb) bt
#0  sig_handle_aix (signo=11) at /home/binutils-gdb/gdb/aix-sighandle.c:7
#1  0x0000000000004a94 in ?? ()
(gdb) 

with patch
-------------

Reading symbols from ./aix-sighandle...done.
(gdb) br sig_handle_aix
Breakpoint 1 at 0x100006bc: file /home/binutils-gdb/gdb/aix-sighandle.c, 
line 7.
(gdb) r
Starting program: /home/binutils-gdb/gdb/aix-sighandle 

Program received signal SIGSEGV, Segmentation fault.
0x0000000100000748 in foo () at /home/binutils-gdb/gdb/aix-sighandle.c:14
14        strcpy(ptr, "signal");
(gdb) c
Continuing.

Breakpoint 1, sig_handle_aix (signo=11) at 
/home/binutils-gdb/gdb/aix-sighandle.c:7
7         printf("signal is: %d\n", signo);
(gdb) bt
#0  sig_handle_aix (signo=11) at/home/binutils-gdb/gdb/aix-sighandle.c:7
#1  <signal handler called>
#2  0x0000000100000748 in foo () at 
/home/binutils-gdb/gdb/aix-sighandle.c:14
#3  0x000000010000079c in main () at 
/home/binutils-gdb/gdb/aix-sighandle.c:19
warning: (Internal error: pc 0x1000004f3 in read in psymtab, but not in 
symtab.)

warning: (Internal error: pc 0x1000004f4 in read in psymtab, but not in 
symtab.)

(gdb) 


Summary of the gdb.base testsuites.
I saw assertion failure not related to the fix while running the testcases 
which i will be fixing it soon.

Wrote a small testcase to run it on only AIX to test the signal handler.

Without patch
-----------------
# of expected passes            16372
# of unexpected failures        2279
# of expected failures          14
# of unresolved testcases       30
# of untested testcases         68
# of unsupported tests          37

with patch
-------------
# of expected passes            16395
# of unexpected failures        2256
# of expected failures          14
# of unresolved testcases       30
# of untested testcases         68
# of unsupported tests          37


We already had some discussion on this before and here is the link.
https://sourceware.org/ml/gdb-patches/2018-01/msg00255.html


 

Thanks,
Sangamesh

[-- Attachment #2: aix-sighandle.patch --]
[-- Type: application/octet-stream, Size: 5567 bytes --]

--- ./gdb/rs6000-aix-tdep.c_orig	2018-03-13 08:59:25 +0000
+++ ./gdb/rs6000-aix-tdep.c	2018-08-27 03:24:23 +0000
@@ -38,6 +38,8 @@
 #include "solib-aix.h"
 #include "target-float.h"
 #include "xml-utils.h"
+#include "tramp-frame.h"
+#include "trad-frame.h"
 
 /* If the kernel has to deliver a signal, it pushes a sigcontext
    structure on the stack and then calls the signal handler, passing
@@ -45,11 +47,24 @@
    the signal handler doesn't save this register, so we have to
    access the sigcontext structure via an offset from the signal handler
    frame.
-   The following constants were determined by experimentation on AIX 3.2.  */
+   The following constants were determined by experimentation on AIX 3.2.
+
+   sigconext structure have the mstsave saved under the
+   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 */
+
 #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;
+  }
+
+  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
   if (func == 0)
     return 0;
   tramp_cache = FRAME_OBSTACK_ZALLOC (struct tramp_frame_cache);

[-- Attachment #3: aix-sighandle_test.c --]
[-- Type: application/octet-stream, Size: 246 bytes --]

#include <stdio.h>
#include <signal.h>
#include <string.h>

void sig_handle_aix(int signo)
{
  printf("signal is: %d\n", signo);
}

void foo()
{
  char *ptr; 
  signal(SIGSEGV, sig_handle_aix);
  strcpy(ptr, "signal");
}

int main()
{
  foo();
}

[-- Attachment #4: aix-sighandle_test.exp --]
[-- Type: application/octet-stream, Size: 850 bytes --]

if {![istarget "powerpc*-*-aix*"]} {
    return
}

if { [prepare_for_testing "failed to prepare" aix-sighandle aix-sighandle.c] } {
    return -1
}

set srcfile aix-sighandle.c
set binfile aix-sighandle

gdb_test "break sig_handle_aix" \
     "Breakpoint.1.at.*:.file.*$srcfile,.line.7." \
     "breakpoint sig_handle_aix"
gdb_test "run" \
  "Starting.program:.*$binfile.*\r\nProgram.received.signal.SIGSEGV,.*\r\n.*.in.foo.*.at.*$srcfile:14.*" \
  "run to breakpoint sig_handle_aix"

gdb_test "continue" \
  "Continuing.*Breakpoint.1,.sig_handle_aix..signo=11..at.*$srcfile:7.*" \
  "continue to breakpoint sig_handle_aix"

gdb_test_sequence "backtrace" "backtrace for sighandle" {
  "\[\r\n\]+#0 .* sig_handle_aix \\(signo=11\\) at "
  "\[\r\n\]+#1 .* .signal.handler.called."
  "\[\r\n\]+#2 .* foo \\(\\) at "
  "\[\r\n\]+#3 .* main \\(\\) at "
}

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

[Kevin Buettner]

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

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

[Sangamesh Mallayya]

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
};

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

[Kevin Buettner]

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

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

[Ulrich Weigand]

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

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

[Sangamesh Mallayya]

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

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

[Sangamesh Mallayya]

[-- Attachment #1: Type: text/plain, Size: 3729 bytes --]

Hi Ulrich/Kevin,

I have modified the patch and implemented it using the trad frame cache.
Please review and let me know the comments.


Here is the dwarf test results summary.
Right now gdb isn't properly able to debug stabs binary which i will be 
debugging.

Without patch
=============
 
# of expected passes            18912
# of unexpected failures        2289
# of expected failures          14
# of known failures             17
# of unresolved testcases       29
# of untested testcases         64
# of unsupported tests          37


With patch
========== 

# of expected passes            18936
# of unexpected failures        2265
# of expected failures          14
# of known failures             17
# of unresolved testcases       27
# of untested testcases         65
# of unsupported tests          37








Thanks,
Sangamesh



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


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



[-- Attachment #2: aix-sighandle.patch --]
[-- Type: application/octet-stream, Size: 4800 bytes --]

--- ./gdb/rs6000-aix-tdep.c_orig	2018-10-27 06:23:41 +0000
+++ ./gdb/rs6000-aix-tdep.c	2018-10-29 12:29:26 +0000
@@ -38,6 +38,8 @@
 #include "solib-aix.h"
 #include "target-float.h"
 #include "xml-utils.h"
+#include "trad-frame.h"
+#include "frame-unwind.h"
 
 /* If the kernel has to deliver a signal, it pushes a sigcontext
    structure on the stack and then calls the signal handler, passing
@@ -45,11 +47,121 @@
    the signal handler doesn't save this register, so we have to
    access the sigcontext structure via an offset from the signal handler
    frame.
-   The following constants were determined by experimentation on AIX 3.2.  */
+   The following constants were determined by experimentation on AIX 3.2.
+
+   sigconext structure have the mstsave saved under the
+   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 */
+
 #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
+
+static struct trad_frame_cache *
+aix_sighandle_frame_cache (struct frame_info *this_frame,
+			   void **this_cache)
+{
+  LONGEST backchain;
+  CORE_ADDR base, base_orig, func;
+  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);
+  struct trad_frame_cache *this_trad_cache;
+
+  if ((*this_cache) != NULL)
+    return (struct trad_frame_cache *) (*this_cache);
+
+  this_trad_cache = trad_frame_cache_zalloc (this_frame);
+  (*this_cache) = this_trad_cache;
+
+  base = get_frame_register_unsigned (this_frame,
+                                      gdbarch_sp_regnum (gdbarch));
+  base_orig = base;
+
+  if (wordsize == 4) {
+    func = read_memory_unsigned_integer (base_orig +
+					 SIG_FRAME_PC_OFFSET + 8, 
+					 tdep->wordsize, byte_order);
+    safe_read_memory_integer (base_orig + SIG_FRAME_FP_OFFSET + 8,
+			      wordsize, byte_order, &backchain);
+    base = (CORE_ADDR)backchain;
+  } else {
+    func = read_memory_unsigned_integer (base_orig +
+					 SIG_FRAME_LR_OFFSET64,
+					 tdep->wordsize, byte_order); 
+    safe_read_memory_integer (base_orig + SIG_FRAME_FP_OFFSET64,
+			      wordsize, byte_order, &backchain);
+    base = (CORE_ADDR)backchain;
+  }
+
+  trad_frame_set_reg_value (this_trad_cache, gdbarch_pc_regnum (gdbarch), func);
+  trad_frame_set_reg_value (this_trad_cache, gdbarch_sp_regnum (gdbarch), base);
+
+  if (wordsize == 4) {
+    trad_frame_set_reg_addr (this_trad_cache, tdep->ppc_lr_regnum,
+                             base_orig + 0x38 + 52 + 8);
+  } else {
+    trad_frame_set_reg_addr (this_trad_cache, tdep->ppc_lr_regnum,
+                             base_orig + 0x70 + 320);
+  }
+
+  trad_frame_set_id (this_trad_cache, frame_id_build (base, func));
+  trad_frame_set_this_base (this_trad_cache, base);
+
+  return this_trad_cache;
+}
+
+static void
+aix_sighandle_frame_this_id (struct frame_info *this_frame,
+			     void **this_prologue_cache,
+			     struct frame_id *this_id)
+{
+  struct trad_frame_cache *this_trad_cache
+    = aix_sighandle_frame_cache (this_frame, this_prologue_cache);
+  trad_frame_get_id (this_trad_cache, this_id);
+}
+
+static struct value *
+aix_sighandle_frame_prev_register (struct frame_info *this_frame,
+				   void **this_prologue_cache, int regnum)
+{
+  struct trad_frame_cache *this_trad_cache
+    = aix_sighandle_frame_cache (this_frame, this_prologue_cache); 
+  return trad_frame_get_register (this_trad_cache, this_frame, regnum);
+} 
+
+int
+aix_sighandle_frame_sniffer (const struct frame_unwind *self,
+			     struct frame_info *this_frame,
+			     void **this_prologue_cache)
+{
+  CORE_ADDR pc = get_frame_pc (this_frame);
+  if (pc && pc < AIX_TEXT_SEGMENT_BASE)
+    return 1;
+
+  return 0;
+}
+
+/* AIX signal handler frame unwinder */
+
+static const struct frame_unwind aix_sighandle_frame_unwind = {
+  SIGTRAMP_FRAME,
+  default_frame_unwind_stop_reason,
+  aix_sighandle_frame_this_id,
+  aix_sighandle_frame_prev_register,
+  NULL,
+  aix_sighandle_frame_sniffer
+};
 
 /* Core file support.  */
 
@@ -1061,6 +1173,7 @@
   set_gdbarch_auto_wide_charset (gdbarch, rs6000_aix_auto_wide_charset);
 
   set_solib_ops (gdbarch, &solib_aix_so_ops);
+  frame_unwind_append_unwinder (gdbarch, &aix_sighandle_frame_unwind);
 }
 
 void

[-- Attachment #3: ChangeLog_gdb --]
[-- Type: application/octet-stream, Size: 317 bytes --]

2018-10-30  Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>

	* rs6000-aix-tdep.c: New signal handler frame_unwinder structure.
	(aix_sighandle_frame_cache): New Function.
	(aix_sighandle_frame_this_id): New Function.
	(aix_sighandle_frame_prev_register): New Function.
	(aix_sighandle_frame_sniffer): New Function. 

[-- Attachment #4: ChangeLog_testsuites --]
[-- Type: application/octet-stream, Size: 151 bytes --]

2018-10-30  Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>

	* gdb.base/aix-sighandle_test.c: New file.
	* gdb.base/aix-sighandle_test.exp: New file.

[-- Attachment #5: aix-sighandle_test.c --]
[-- Type: application/octet-stream, Size: 952 bytes --]

/* Copyright 2018 Free Software Foundation, Inc.

This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation; either version 3 of the License, or
(at your option) any later version.

This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
GNU General Public License for more details.

You should have received a copy of the GNU General Public License
along with this program.  If not, see <http://www.gnu.org/licenses/>. */

#include <stdio.h>
#include <signal.h>
#include <string.h>

void sig_handle_aix (int signo)
{
  printf ("signal is: %d\n", signo);
}

void foo()
{
  char *ptr; 
  signal (SIGSEGV, sig_handle_aix);
  strcpy (ptr, "signal");
}

int main()
{
  foo ();
}

[-- Attachment #6: aix-sighandle_test.exp --]
[-- Type: application/octet-stream, Size: 1600 bytes --]

# Copyright 2018 Free Software Foundation, Inc.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program.  If not, see <http://www.gnu.org/licenses/>.

if {![istarget "powerpc*-*-aix*"]} {
    return
}

if { [prepare_for_testing "failed to prepare" aix-sighandle_test aix-sighandle_test.c] } {
    return -1
}

set srcfile aix-sighandle_test.c
set binfile aix-sighandle_test

gdb_test "break sig_handle_aix" \
     "Breakpoint.1.at.*:.file.*$srcfile,.line.22." \
     "breakpoint sig_handle_aix"
gdb_test "run" \
  "Starting.program:.*$binfile.*\r\nProgram.received.signal.SIGSEGV,.*\r\n.*.in.foo.*.at.*$srcfile:29.*" \
  "run to breakpoint sig_handle_aix"

gdb_test "continue" \
  "Continuing.*Breakpoint.1,.sig_handle_aix..signo=11..at.*$srcfile:22.*" \
  "continue to breakpoint sig_handle_aix"

gdb_test_sequence "backtrace" "backtrace for sighandle" {
  "\[\r\n\]+#0 .* sig_handle_aix \\(signo=11\\) at "
  "\[\r\n\]+#1 .* .signal.handler.called."
  "\[\r\n\]+#2 .* foo \\(\\) at "
  "\[\r\n\]+#3 .* main \\(\\) at "
}

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

[Ulrich Weigand]

Sangamesh Mallayya wrote:

> I have modified the patch and implemented it using the trad frame cache.
> Please review and let me know the comments.

This version now looks pretty good to me, I just have a few minor comments
related to coding style.  Once those are addresses, it should be ready to
commit.

> +   sigconext structure have the mstsave saved under the

Typo "sigcontext" ?

> +  if (wordsize == 4) {
> +    func = read_memory_unsigned_integer (base_orig +
> +					 SIG_FRAME_PC_OFFSET + 8, 
> +					 tdep->wordsize, byte_order);
> +    safe_read_memory_integer (base_orig + SIG_FRAME_FP_OFFSET + 8,
> +			      wordsize, byte_order, &backchain);
> +    base = (CORE_ADDR)backchain;
> +  } else {
> +    func = read_memory_unsigned_integer (base_orig +
> +					 SIG_FRAME_LR_OFFSET64,
> +					 tdep->wordsize, byte_order); 
> +    safe_read_memory_integer (base_orig + SIG_FRAME_FP_OFFSET64,
> +			      wordsize, byte_order, &backchain);
> +    base = (CORE_ADDR)backchain;
> +  }

GNU coding style is to put braces on separate lines, and indent like so

     if (wordsize == 4)
       {
         func = ...
       }
     else
       {
         func = ...
       }

Also, why do you use tdep->wordsize in some places and wordsize in others?

> +  if (wordsize == 4) {
> +    trad_frame_set_reg_addr (this_trad_cache, tdep->ppc_lr_regnum,
> +                             base_orig + 0x38 + 52 + 8);
> +  } else {
> +    trad_frame_set_reg_addr (this_trad_cache, tdep->ppc_lr_regnum,
> +                             base_orig + 0x70 + 320);
> +  }

Where there is just a single line inside the if / else block, you
should just omit the braces completely.

>	* gdb.base/aix-sighandle_test.c: New file.
>	* gdb.base/aix-sighandle_test.exp: New file.

Those should best go in gdb.arch, not in gdb.base (since they are
specific to a single platform).   Also, it may be better to omit
the "_test" part of the file names (all files in these directories
are tests!).

Thanks,
Ulrich

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

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

[Sangamesh Mallayya]

[-- Attachment #1: Type: text/plain, Size: 2489 bytes --]

Hi Ulrich,

Thanks for the review.

> 
> > I have modified the patch and implemented it using the trad frame 
cache.
> > Please review and let me know the comments.
> 
> This version now looks pretty good to me, I just have a few minor 
comments
> related to coding style.  Once those are addresses, it should be ready 
to
> commit.
> 
> > +   sigconext structure have the mstsave saved under the
> 
> Typo "sigcontext" ?
> 

Yes. Corrected this.

> > +  if (wordsize == 4) {
> > +    func = read_memory_unsigned_integer (base_orig +
> > +                SIG_FRAME_PC_OFFSET + 8, 
> > +                tdep->wordsize, byte_order);
> > +    safe_read_memory_integer (base_orig + SIG_FRAME_FP_OFFSET + 8,
> > +               wordsize, byte_order, &backchain);
> > +    base = (CORE_ADDR)backchain;
> > +  } else {
> > +    func = read_memory_unsigned_integer (base_orig +
> > +                SIG_FRAME_LR_OFFSET64,
> > +                tdep->wordsize, byte_order); 
> > +    safe_read_memory_integer (base_orig + SIG_FRAME_FP_OFFSET64,
> > +               wordsize, byte_order, &backchain);
> > +    base = (CORE_ADDR)backchain;
> > +  }
> 
> GNU coding style is to put braces on separate lines, and indent like so
> 
>      if (wordsize == 4)
>        {
>          func = ...
>        }
>      else
>        {
>          func = ...
>        }
> 

Changed to have the proper coding style.

> Also, why do you use tdep->wordsize in some places and wordsize in 
others?
> 

Now using only tdep->wordsize.

> > +  if (wordsize == 4) {
> > +    trad_frame_set_reg_addr (this_trad_cache, tdep->ppc_lr_regnum,
> > +                             base_orig + 0x38 + 52 + 8);
> > +  } else {
> > +    trad_frame_set_reg_addr (this_trad_cache, tdep->ppc_lr_regnum,
> > +                             base_orig + 0x70 + 320);
> > +  }
> 
> Where there is just a single line inside the if / else block, you
> should just omit the braces completely.
> 

Removed the extra braces.

> >   * gdb.base/aix-sighandle_test.c: New file.
> >   * gdb.base/aix-sighandle_test.exp: New file.
> 
> Those should best go in gdb.arch, not in gdb.base (since they are
> specific to a single platform).   Also, it may be better to omit
> the "_test" part of the file names (all files in these directories
> are tests!).
> 

Moved these to gdb.arch.


Attaching the modified files.
Please review and let me know if fine to commit.




[-- Attachment #2: aix-sighandle.patch --]
[-- Type: application/octet-stream, Size: 4807 bytes --]

--- ./gdb/rs6000-aix-tdep.c_orig	2018-10-27 06:23:41 +0000
+++ ./gdb/rs6000-aix-tdep.c	2018-10-31 04:36:58 +0000
@@ -38,6 +38,8 @@
 #include "solib-aix.h"
 #include "target-float.h"
 #include "xml-utils.h"
+#include "trad-frame.h"
+#include "frame-unwind.h"
 
 /* If the kernel has to deliver a signal, it pushes a sigcontext
    structure on the stack and then calls the signal handler, passing
@@ -45,11 +47,122 @@
    the signal handler doesn't save this register, so we have to
    access the sigcontext structure via an offset from the signal handler
    frame.
-   The following constants were determined by experimentation on AIX 3.2.  */
+   The following constants were determined by experimentation on AIX 3.2.
+
+   sigcontext structure have the mstsave saved under the
+   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 */
+
 #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
+
+static struct trad_frame_cache *
+aix_sighandle_frame_cache (struct frame_info *this_frame,
+			   void **this_cache)
+{
+  LONGEST backchain;
+  CORE_ADDR base, base_orig, func;
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+  struct trad_frame_cache *this_trad_cache;
+
+  if ((*this_cache) != NULL)
+    return (struct trad_frame_cache *) (*this_cache);
+
+  this_trad_cache = trad_frame_cache_zalloc (this_frame);
+  (*this_cache) = this_trad_cache;
+
+  base = get_frame_register_unsigned (this_frame,
+                                      gdbarch_sp_regnum (gdbarch));
+  base_orig = base;
+
+  if (tdep->wordsize == 4)
+    {
+      func = read_memory_unsigned_integer (base_orig +
+					   SIG_FRAME_PC_OFFSET + 8, 
+					   tdep->wordsize, byte_order);
+      safe_read_memory_integer (base_orig + SIG_FRAME_FP_OFFSET + 8,
+				tdep->wordsize, byte_order, &backchain);
+      base = (CORE_ADDR)backchain;
+    }
+  else
+    {
+      func = read_memory_unsigned_integer (base_orig +
+					   SIG_FRAME_LR_OFFSET64,
+					   tdep->wordsize, byte_order); 
+      safe_read_memory_integer (base_orig + SIG_FRAME_FP_OFFSET64,
+				tdep->wordsize, byte_order, &backchain);
+      base = (CORE_ADDR)backchain;
+    }
+
+  trad_frame_set_reg_value (this_trad_cache, gdbarch_pc_regnum (gdbarch), func);
+  trad_frame_set_reg_value (this_trad_cache, gdbarch_sp_regnum (gdbarch), base);
+
+  if (tdep->wordsize == 4)
+    trad_frame_set_reg_addr (this_trad_cache, tdep->ppc_lr_regnum,
+                             base_orig + 0x38 + 52 + 8);
+  else
+    trad_frame_set_reg_addr (this_trad_cache, tdep->ppc_lr_regnum,
+                             base_orig + 0x70 + 320);
+
+  trad_frame_set_id (this_trad_cache, frame_id_build (base, func));
+  trad_frame_set_this_base (this_trad_cache, base);
+
+  return this_trad_cache;
+}
+
+static void
+aix_sighandle_frame_this_id (struct frame_info *this_frame,
+			     void **this_prologue_cache,
+			     struct frame_id *this_id)
+{
+  struct trad_frame_cache *this_trad_cache
+    = aix_sighandle_frame_cache (this_frame, this_prologue_cache);
+  trad_frame_get_id (this_trad_cache, this_id);
+}
+
+static struct value *
+aix_sighandle_frame_prev_register (struct frame_info *this_frame,
+				   void **this_prologue_cache, int regnum)
+{
+  struct trad_frame_cache *this_trad_cache
+    = aix_sighandle_frame_cache (this_frame, this_prologue_cache); 
+  return trad_frame_get_register (this_trad_cache, this_frame, regnum);
+} 
+
+int
+aix_sighandle_frame_sniffer (const struct frame_unwind *self,
+			     struct frame_info *this_frame,
+			     void **this_prologue_cache)
+{
+  CORE_ADDR pc = get_frame_pc (this_frame);
+  if (pc && pc < AIX_TEXT_SEGMENT_BASE)
+    return 1;
+
+  return 0;
+}
+
+/* AIX signal handler frame unwinder */
+
+static const struct frame_unwind aix_sighandle_frame_unwind = {
+  SIGTRAMP_FRAME,
+  default_frame_unwind_stop_reason,
+  aix_sighandle_frame_this_id,
+  aix_sighandle_frame_prev_register,
+  NULL,
+  aix_sighandle_frame_sniffer
+};
 
 /* Core file support.  */
 
@@ -1061,6 +1174,7 @@
   set_gdbarch_auto_wide_charset (gdbarch, rs6000_aix_auto_wide_charset);
 
   set_solib_ops (gdbarch, &solib_aix_so_ops);
+  frame_unwind_append_unwinder (gdbarch, &aix_sighandle_frame_unwind);
 }
 
 void

[-- Attachment #3: ChangeLog_gdb --]
[-- Type: application/octet-stream, Size: 317 bytes --]

2018-10-31  Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>

	* rs6000-aix-tdep.c: New signal handler frame_unwinder structure.
	(aix_sighandle_frame_cache): New Function.
	(aix_sighandle_frame_this_id): New Function.
	(aix_sighandle_frame_prev_register): New Function.
	(aix_sighandle_frame_sniffer): New Function. 

[-- Attachment #4: aix-sighandle.c --]
[-- Type: application/octet-stream, Size: 952 bytes --]

/* Copyright 2018 Free Software Foundation, Inc.

This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation; either version 3 of the License, or
(at your option) any later version.

This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
GNU General Public License for more details.

You should have received a copy of the GNU General Public License
along with this program.  If not, see <http://www.gnu.org/licenses/>. */

#include <stdio.h>
#include <signal.h>
#include <string.h>

void sig_handle_aix (int signo)
{
  printf ("signal is: %d\n", signo);
}

void foo()
{
  char *ptr; 
  signal (SIGSEGV, sig_handle_aix);
  strcpy (ptr, "signal");
}

int main()
{
  foo ();
}

[-- Attachment #5: aix-sighandle.exp --]
[-- Type: application/octet-stream, Size: 1580 bytes --]

# Copyright 2018 Free Software Foundation, Inc.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program.  If not, see <http://www.gnu.org/licenses/>.

if {![istarget "powerpc*-*-aix*"]} {
    return
}

if { [prepare_for_testing "failed to prepare" aix-sighandle aix-sighandle.c] } {
    return -1
}

set srcfile aix-sighandle.c
set binfile aix-sighandle

gdb_test "break sig_handle_aix" \
     "Breakpoint.1.at.*:.file.*$srcfile,.line.22." \
     "breakpoint sig_handle_aix"
gdb_test "run" \
  "Starting.program:.*$binfile.*\r\nProgram.received.signal.SIGSEGV,.*\r\n.*.in.foo.*.at.*$srcfile:29.*" \
  "run to breakpoint sig_handle_aix"

gdb_test "continue" \
  "Continuing.*Breakpoint.1,.sig_handle_aix..signo=11..at.*$srcfile:22.*" \
  "continue to breakpoint sig_handle_aix"

gdb_test_sequence "backtrace" "backtrace for sighandle" {
  "\[\r\n\]+#0 .* sig_handle_aix \\(signo=11\\) at "
  "\[\r\n\]+#1 .* .signal.handler.called."
  "\[\r\n\]+#2 .* foo \\(\\) at "
  "\[\r\n\]+#3 .* main \\(\\) at "
}

[-- Attachment #6: ChangeLog_testsuites --]
[-- Type: application/octet-stream, Size: 141 bytes --]

2018-10-31  Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>

	* gdb.arch/aix-sighandle.c: New file.
	* gdb.arch/aix-sighandle.exp: New file.

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

[Ulrich Weigand]

Sangamesh Mallayya wrote:

> Attaching the modified files.
> Please review and let me know if fine to commit.

This is OK to commit.

> 2018-10-31  Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
> 
> 	* rs6000-aix-tdep.c: New signal handler frame_unwinder structure.
> 	(aix_sighandle_frame_cache): New Function.
> 	(aix_sighandle_frame_this_id): New Function.
> 	(aix_sighandle_frame_prev_register): New Function.
> 	(aix_sighandle_frame_sniffer): New Function. 
> 
> 2018-10-31  Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
> 
> 	* gdb.arch/aix-sighandle.c: New file.
> 	* gdb.arch/aix-sighandle.exp: New file.


When committing, please add a few more details to the ChangeLog,
in particular:
- the added #include statements
- the new #defines
- the new variable aix_sighandle_frame_unwind 
- the added call to frame_unwind_append_unwinder

The ChangeLog for rs6000-aix-tdep should read something like this:

	* rs6000-aix-tdep.c: Include "trad-frame.h" and "frame-unwind.h".
	(SIG_FRAME_LR_OFFSET64): New define.
	(SIG_FRAME_FP_OFFSET64): New define.
 	(aix_sighandle_frame_cache): New function.
 	(aix_sighandle_frame_this_id): New function.
 	(aix_sighandle_frame_prev_register): New function.
 	(aix_sighandle_frame_sniffer): New function. 
	(aix_sighandle_frame_unwind): New global variable.
	(rs6000_aix_init_osabi): Install new frame unwinder.

Thanks,
Ulrich

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