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

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

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