[commit] Fix OpenBSD/i386 and OpenBSD/amd64 kernel trapframe unwinders

[commit] Fix OpenBSD/i386 and OpenBSD/amd64 kernel trapframe unwinders

[Mark Kettenis]

Teach OpenBSD/i386 how to unwind from interrupt frames too.  Plus a
small fix for OpenBSD/amd64 for interrupts from user space.

Mark

Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>

	* amd64obsd-tdep.c (amd64obsd_trapframe_cache): Fix detection of
	interrupts from user space.
	* i386obsd-tdep.c (i386obsd_trapframe_cache): Handle interrupt
	frames too.
	(i386obsd_trapframe_sniffer): Turn into a proper unwinder sniffer.
	(i386obsd_trapframe_unwind): Add sniffer.
	(i386obsd_init_abi): Prepend i386obsd_trapframe_unwind instead of
	appending i386obsd_trapframe_sniffer.

Index: amd64obsd-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/amd64obsd-tdep.c,v
retrieving revision 1.19
diff -u -p -r1.19 amd64obsd-tdep.c
--- amd64obsd-tdep.c 22 Dec 2005 13:17:49 -0000 1.19
+++ amd64obsd-tdep.c 22 Dec 2005 14:06:28 -0000
@@ -370,7 +370,7 @@ amd64obsd_trapframe_cache(struct frame_i
       trad_frame_set_reg_addr (cache, i, addr + amd64obsd_tf_reg_offset[i]);
 
   /* Read %cs from trap frame.  */
-  addr = sp + amd64obsd_tf_reg_offset[AMD64_CS_REGNUM];
+  addr += amd64obsd_tf_reg_offset[AMD64_CS_REGNUM];
   cs = read_memory_unsigned_integer (addr, 8); 
   if ((cs & I386_SEL_RPL) == I386_SEL_UPL)
     {
Index: i386obsd-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386obsd-tdep.c,v
retrieving revision 1.26
diff -u -p -r1.26 i386obsd-tdep.c
--- i386obsd-tdep.c 19 Dec 2005 22:19:49 -0000 1.26
+++ i386obsd-tdep.c 22 Dec 2005 14:06:28 -0000
@@ -345,6 +345,7 @@ i386obsd_trapframe_cache(struct frame_in
   struct trad_frame_cache *cache;
   CORE_ADDR func, sp, addr;
   ULONGEST cs;
+  char *name;
   int i;
 
   if (*this_cache)
@@ -355,12 +356,19 @@ i386obsd_trapframe_cache(struct frame_in
 
   func = frame_func_unwind (next_frame);
   sp = frame_unwind_register_unsigned (next_frame, I386_ESP_REGNUM);
+
+  find_pc_partial_function (func, &name, NULL, NULL);
+  if (name && strncmp(name, "Xintr", 5) == 0)
+    addr = sp + 8;		/* It's an interrupt frame.  */
+  else
+    addr = sp;
+
   for (i = 0; i < ARRAY_SIZE (i386obsd_tf_reg_offset); i++)
     if (i386obsd_tf_reg_offset[i] != -1)
-      trad_frame_set_reg_addr (cache, i, sp + i386obsd_tf_reg_offset[i]);
+      trad_frame_set_reg_addr (cache, i, addr + i386obsd_tf_reg_offset[i]);
 
   /* Read %cs from trap frame.  */
-  addr = sp + i386obsd_tf_reg_offset[I386_CS_REGNUM];
+  addr += i386obsd_tf_reg_offset[I386_CS_REGNUM];
   cs = read_memory_unsigned_integer (addr, 4); 
   if ((cs & I386_SEL_RPL) == I386_SEL_UPL)
     {
@@ -400,17 +408,10 @@ i386obsd_trapframe_prev_register (struct
 			   optimizedp, lvalp, addrp, realnump, valuep);
 }
 
-static const struct frame_unwind i386obsd_trapframe_unwind = {
-  /* FIXME: kettenis/20051219: This really is more like an interrupt
-     frame, but SIGTRAMP_FRAME would print <signal handler called>,
-     which really is not what we want here.  */
-  NORMAL_FRAME,
-  i386obsd_trapframe_this_id,
-  i386obsd_trapframe_prev_register
-};
-
-static const struct frame_unwind *
-i386obsd_trapframe_sniffer (struct frame_info *next_frame)
+static int
+i386obsd_trapframe_sniffer (const struct frame_unwind *self,
+			    struct frame_info *next_frame,
+			    void **this_prologue_cache)
 {
   ULONGEST cs;
   char *name;
@@ -420,12 +421,21 @@ i386obsd_trapframe_sniffer (struct frame
     return NULL;
 
   find_pc_partial_function (frame_pc_unwind (next_frame), &name, NULL, NULL);
-  if (name && ((strcmp ("calltrap", name) == 0)
-	       || (strcmp ("syscall1", name) == 0)))
-    return &i386obsd_trapframe_unwind;
-
-  return NULL;
+  return (name && ((strcmp (name, "calltrap") == 0)
+		   || (strcmp (name, "syscall1") == 0)
+		   || (strncmp (name, "Xintr", 5) == 0)));
 }
+
+static const struct frame_unwind i386obsd_trapframe_unwind = {
+  /* FIXME: kettenis/20051219: This really is more like an interrupt
+     frame, but SIGTRAMP_FRAME would print <signal handler called>,
+     which really is not what we want here.  */
+  NORMAL_FRAME,
+  i386obsd_trapframe_this_id,
+  i386obsd_trapframe_prev_register,
+  NULL,
+  i386obsd_trapframe_sniffer
+};
 \f
 
 static void 
@@ -459,7 +469,7 @@ i386obsd_init_abi (struct gdbarch_info i
   bsd_uthread_set_collect_uthread (gdbarch, i386obsd_collect_uthread);
 
   /* Unwind kernel trap frames correctly.  */
-  frame_unwind_append_sniffer (gdbarch, i386obsd_trapframe_sniffer);
+  frame_unwind_prepend_unwinder (gdbarch, &i386obsd_trapframe_unwind);
 }
 
 /* OpenBSD a.out.  */

Re: [commit] Fix OpenBSD/i386 and OpenBSD/amd64 kernel trapframe unwinders

[Daniel Jacobowitz]

On Thu, Dec 22, 2005 at 03:09:21PM +0100, Mark Kettenis wrote:
> +
> +  find_pc_partial_function (func, &name, NULL, NULL);
> +  if (name && strncmp(name, "Xintr", 5) == 0)
> +    addr = sp + 8;		/* It's an interrupt frame.  */
> +  else
> +    addr = sp;
> +

Is this series of patches really a good idea?

I realize there's no way to configure GDB at runtime to do this sort of
thing, and I accept that that's a serious shortcoming that we
eventually need to fix.  But you're adding bits to the OpenBSD target
that only make sense in terms of the naming conventions of a single
OpenBSD program (which happens to be the kernel); I've resisted doing
this in the past both for The Server Then Known As XFree86 and for the
Linux kernel.  Why should GDB know the names of functions in a single
one of the infinitely many things it might be debugging?

On a less metaphysical note, I'd worry about any other program with a
function named Xintr.

On a nitpicking note, you missed a space before a parenthesis above :-)

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [commit] Fix OpenBSD/i386 and OpenBSD/amd64 kernel trapframe unwinders

[Mark Kettenis]

> Date: Thu, 22 Dec 2005 09:20:13 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Thu, Dec 22, 2005 at 03:09:21PM +0100, Mark Kettenis wrote:
> > +
> > +  find_pc_partial_function (func, &name, NULL, NULL);
> > +  if (name && strncmp(name, "Xintr", 5) == 0)
> > +    addr = sp + 8;		/* It's an interrupt frame.  */
> > +  else
> > +    addr = sp;
> > +
> 
> Is this series of patches really a good idea?
> 
> I realize there's no way to configure GDB at runtime to do this sort of
> thing, and I accept that that's a serious shortcoming that we
> eventually need to fix.  But you're adding bits to the OpenBSD target
> that only make sense in terms of the naming conventions of a single
> OpenBSD program (which happens to be the kernel); I've resisted doing
> this in the past both for The Server Then Known As XFree86 and for the
> Linux kernel.  Why should GDB know the names of functions in a single
> one of the infinitely many things it might be debugging?
>
> On a less metaphysical note, I'd worry about any other program with a
> function named Xintr.

I've struggled with this for a while.  Apart from adding DWARF2 CFI to
the kernel, there's no real alternative to matching function names.
And DWARF2 doesn't allow me to terminate the backtrace at the user to
kernel transition.  Simply matching the function names is of course
unacceptable, so I had to do something a bit more clever.  If you look
at the sniffer for the unwinder, you see the following check:

  cs = frame_unwind_register_unsigned (next_frame, AMD64_CS_REGNUM);
  if ((cs & I386_SEL_RPL) == I386_SEL_UPL)
    return 0;

This checks the Requested Protection Level, which is stored in the
lower two bits of %cs.  When executing in the kernel, the RPL is 0,
for userland its 3.  So we can reliably check whether we're in the
kernel or in userland.  And I only check the magic function names when
the current frame is a kernel frame.  I guess I should add a comment
in the code about that.

The nice thing about this approach is that you don't need any magic
knobs to turn things like this on.  I guess you can do something
similar for Linux; that's why I added the I386_SEL_XXX defines to
i386-tdep.h.

> On a nitpicking note, you missed a space before a parenthesis above :-)

Duh!  I'll fix that later today.

P.S. The new Xorg X11R6.9/X11R7.0 doesn't need any special patches
anymore since they now use dlopen(3) to load modules.

Re: [commit] Fix OpenBSD/i386 and OpenBSD/amd64 kernel trapframe unwinders

[Daniel Jacobowitz]

On Thu, Dec 22, 2005 at 04:20:07PM +0100, Mark Kettenis wrote:
> I've struggled with this for a while.  Apart from adding DWARF2 CFI to
> the kernel, there's no real alternative to matching function names.
> And DWARF2 doesn't allow me to terminate the backtrace at the user to
> kernel transition.

Yes it does - what do you think inspired my patches for an undefined
return address column? :-)  They were for Linux's KGDB.

Unless the OpenBSD kernel has a distressingly large number of entry
points, the same thing should work there.  It's only if you want to do
something besides stop at the boundary that things get really
complicated.

> Simply matching the function names is of course
> unacceptable, so I had to do something a bit more clever.  If you look
> at the sniffer for the unwinder, you see the following check:
> 
>   cs = frame_unwind_register_unsigned (next_frame, AMD64_CS_REGNUM);
>   if ((cs & I386_SEL_RPL) == I386_SEL_UPL)
>     return 0;
> 
> This checks the Requested Protection Level, which is stored in the
> lower two bits of %cs.  When executing in the kernel, the RPL is 0,
> for userland its 3.  So we can reliably check whether we're in the
> kernel or in userland.  And I only check the magic function names when
> the current frame is a kernel frame.  I guess I should add a comment
> in the code about that.

Aha, I missed that.  And we're OpenBSD targeted here, so if we're in
kernel mode, it's a reasonably safe assumption that we're in the
OpenBSD kernel.  I'd appreciate a clarifying comment.

> P.S. The new Xorg X11R6.9/X11R7.0 doesn't need any special patches
> anymore since they now use dlopen(3) to load modules.

Ooh, that's good news.  The Linux kernel module loader still does,
though - they've been talking about loading objects as shared libraries
for years, but I don't think it's going to happen soon.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [commit] Fix OpenBSD/i386 and OpenBSD/amd64 kernel trapframe unwinders

[Mark Kettenis]

> Date: Thu, 22 Dec 2005 10:26:15 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Thu, Dec 22, 2005 at 04:20:07PM +0100, Mark Kettenis wrote:
> > I've struggled with this for a while.  Apart from adding DWARF2 CFI to
> > the kernel, there's no real alternative to matching function names.
> > And DWARF2 doesn't allow me to terminate the backtrace at the user to
> > kernel transition.
> 
> Yes it does - what do you think inspired my patches for an undefined
> return address column? :-)  They were for Linux's KGDB.
> 
> Unless the OpenBSD kernel has a distressingly large number of entry
> points, the same thing should work there.  It's only if you want to do
> something besides stop at the boundary that things get really
> complicated.

Yup.  There's a seperate entry point for every interrupt vector (hence
the strncmp() for "Xintr") and on top of that, interrupts can come
from both userland and from within the kernel.  I only want to
terminate the backtrace when the interrupt came from userland.

> > P.S. The new Xorg X11R6.9/X11R7.0 doesn't need any special patches
> > anymore since they now use dlopen(3) to load modules.
> 
> Ooh, that's good news.  The Linux kernel module loader still does,
> though - they've been talking about loading objects as shared libraries
> for years, but I don't think it's going to happen soon.

A monilithic kernel has its benefits ;-).

Re: [commit] Fix OpenBSD/i386 and OpenBSD/amd64 kernel trapframe unwinders

[Daniel Jacobowitz]

On Thu, Dec 22, 2005 at 04:38:14PM +0100, Mark Kettenis wrote:
> Yup.  There's a seperate entry point for every interrupt vector (hence
> the strncmp() for "Xintr") and on top of that, interrupts can come
> from both userland and from within the kernel.  I only want to
> terminate the backtrace when the interrupt came from userland.

That's no worse than for Linux.  This is actually all solvable with
DWARF-2, and in fact, George Anzinger wrote bits to do it for Linux. 
They're a bit disgusting, though (and I hope he'll agree if he's
reading this :-)

  http://source.mvista.com/~ganzinger/common_kgdb_cfi_annotations.patch

He makes use of dwarf2 branches; I'm not sure if he ended up needing an
extra physical stack frame anywhere, but I'm convinced it's possible to
do without that.

-- 
Daniel Jacobowitz
CodeSourcery, LLC