[rfc] dwarf2 unwinder and MIPS n32

[rfc] dwarf2 unwinder and MIPS n32

[Daniel Jacobowitz]

The DWARF-2 unwinder uses store_typed_address to store the value of
the CFA or RA into a register-sized buffer.  The type of the register
might be a pointer or integer type, so it passes
builtin_type_void_data_ptr and builtin_type_void_func_ptr as
appropriate.  But for MIPS N32, sizeof (void *) == 4 and the stack
pointer is 64-bit.  So it unwinds writes four bytes into the first
four of the eight byte slot; since I'm testing big-endian, the failure
is quickly obvious.

So what do we do about it?  The patch below works for MIPS, but I'm
reasonably sure it's wrong; it avoids the architecture's
ADDRESS_TO_POINTER method entirely.  If we pass the register's type to
store_typed_address we'll get various failures if the architecture
doesn't define the relevant register as a pointer.  And MIPS doesn't,
partly because the register is 64-bit and the pointer would only be
32-bit.

Maybe if the size of the register != the size of a void * we should
store it as an unsigned integer.  But that seems hackish to me.

I'd love comments; I don't want to commit this patch, but I can't turn
on CFI for MIPS without it.

-- 
Daniel Jacobowitz
CodeSourcery

2007-04-28  Daniel Jacobowitz  <dan@codesourcery.com>

	* dwarf2-frame.c (dwarf2_frame_prev_register): Use
	store_unsigned_integer instead of store_typed_address.

---
 dwarf2-frame.c |   15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

Index: gdb/dwarf2-frame.c
===================================================================
--- gdb.orig/dwarf2-frame.c	2007-04-27 17:03:21.000000000 -0400
+++ gdb/dwarf2-frame.c	2007-04-27 17:42:37.000000000 -0400
@@ -1137,10 +1137,8 @@ dwarf2_frame_prev_register (struct frame
       *addrp = 0;
       *realnump = -1;
       if (valuep)
-	{
-	  /* Store the value.  */
-	  store_typed_address (valuep, builtin_type_void_data_ptr, cache->cfa);
-	}
+	store_unsigned_integer (valuep, register_size (gdbarch, regnum),
+				cache->cfa);
       break;
 
     case DWARF2_FRAME_REG_CFA_OFFSET:
@@ -1149,11 +1147,8 @@ dwarf2_frame_prev_register (struct frame
       *addrp = 0;
       *realnump = -1;
       if (valuep)
-	{
-	  /* Store the value.  */
-	  store_typed_address (valuep, builtin_type_void_data_ptr,
-			       cache->cfa + cache->reg[regnum].loc.offset);
-	}
+	store_unsigned_integer (valuep, register_size (gdbarch, regnum),
+				cache->cfa + cache->reg[regnum].loc.offset);
       break;
 
     case DWARF2_FRAME_REG_RA_OFFSET:
@@ -1167,7 +1162,7 @@ dwarf2_frame_prev_register (struct frame
 
           regnum = DWARF2_REG_TO_REGNUM (cache->retaddr_reg.loc.reg);
           pc += frame_unwind_register_unsigned (next_frame, regnum);
-          store_typed_address (valuep, builtin_type_void_func_ptr, pc);
+	  store_unsigned_integer (valuep, register_size (gdbarch, regnum), pc);
         }
       break;
 

Re: [rfc] dwarf2 unwinder and MIPS n32

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> So what do we do about it?  The patch below works for MIPS, but I'm
> reasonably sure it's wrong; it avoids the architecture's
> ADDRESS_TO_POINTER method entirely.  If we pass the register's type to
> store_typed_address we'll get various failures if the architecture
> doesn't define the relevant register as a pointer.  And MIPS doesn't,
> partly because the register is 64-bit and the pointer would only be
> 32-bit.

Well, when we *read* a register in order to get (or compute) the CFA,
read_reg does

  unpack_long (register_type (gdbarch, regnum), buf)

which in turn uses either extract_type_address, extract_signed_integer,
or extract_unsigned_integer, depending on the type.

It may make sense to perform the analogous operation when *writing*
the CFA to (an unwound copy of the contents of) a register.  There
is no "pack_long", but something like

  memcpy (buf, value_contents
		 (value_from_longest (register_type (gdbarch, regnum), cfa)),
	       register_size (gdbarch, regnum));

should have that effect.

Not sure if that is the right thing to do in all cases, but at least
it would be consistent ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] dwarf2 unwinder and MIPS n32

[Maciej W. Rozycki]

On Sat, 28 Apr 2007, Daniel Jacobowitz wrote:

> Maybe if the size of the register != the size of a void * we should
> store it as an unsigned integer.  But that seems hackish to me.

 Of course MIPS addresses are signed, so if you have a stack pointer in 
KSEG0 (e.g. 0x8fff0000), you want to store it as a signed integer, don't 
you?

 Anyway, I have the following patch waiting in the queue for submission -- 
perhaps it is less hackish. ;-)

2007-04-30  Maciej W. Rozycki  <macro@mips.com>

	* dwarf2-frame.c (read_reg): Extract an address using the width
	of the register to be used to hold it as the size.
	(dwarf2_frame_prev_register): Store an address using the width
	of the holding register as the size.

 It can certainly jump the queue if you find it useful. ;-)

  Maciej

13118.diff
Index: gdb/src/gdb/dwarf2-frame.c
===================================================================
--- gdb.orig/src/gdb/dwarf2-frame.c	2007-02-13 13:51:55.000000000 +0000
+++ gdb/src/gdb/dwarf2-frame.c	2007-02-13 13:51:56.000000000 +0000
@@ -229,6 +229,7 @@
 static CORE_ADDR
 read_reg (void *baton, int reg)
 {
+  struct type reg_void_data_ptr = *builtin_type_void_data_ptr;
   struct frame_info *next_frame = (struct frame_info *) baton;
   struct gdbarch *gdbarch = get_frame_arch (next_frame);
   int regnum;
@@ -236,6 +237,8 @@
 
   regnum = DWARF2_REG_TO_REGNUM (reg);
 
+  TYPE_LENGTH(&reg_void_data_ptr) = register_size (gdbarch, regnum);
+
   buf = alloca (register_size (gdbarch, regnum));
   frame_unwind_register (next_frame, regnum, buf);
 
@@ -244,7 +247,7 @@
      under the covers, and this makes more sense for non-pointer
      registers.  Maybe read_reg and the associated interfaces should
      deal with "struct value" instead of CORE_ADDR.  */
-  return unpack_long (register_type (gdbarch, regnum), buf);
+  return extract_typed_address (buf, &reg_void_data_ptr);
 }
 
 static void
@@ -1020,10 +1023,13 @@
 			    enum lval_type *lvalp, CORE_ADDR *addrp,
 			    int *realnump, gdb_byte *valuep)
 {
+  struct type reg_void_data_ptr = *builtin_type_void_data_ptr;
   struct gdbarch *gdbarch = get_frame_arch (next_frame);
   struct dwarf2_frame_cache *cache =
     dwarf2_frame_cache (next_frame, this_cache);
 
+  TYPE_LENGTH(&reg_void_data_ptr) = register_size (gdbarch, regnum);
+
   switch (cache->reg[regnum].how)
     {
     case DWARF2_FRAME_REG_UNDEFINED:
@@ -1132,7 +1138,7 @@
       if (valuep)
 	{
 	  /* Store the value.  */
-	  store_typed_address (valuep, builtin_type_void_data_ptr, cache->cfa);
+	  store_typed_address (valuep, &reg_void_data_ptr, cache->cfa);
 	}
       break;
 

Re: [rfc] dwarf2 unwinder and MIPS n32

[Daniel Jacobowitz]

On Mon, Apr 30, 2007 at 01:40:54PM +0100, Maciej W. Rozycki wrote:
> On Sat, 28 Apr 2007, Daniel Jacobowitz wrote:
> 
> > Maybe if the size of the register != the size of a void * we should
> > store it as an unsigned integer.  But that seems hackish to me.
> 
>  Of course MIPS addresses are signed, so if you have a stack pointer in 
> KSEG0 (e.g. 0x8fff0000), you want to store it as a signed integer, don't 
> you?

It should already be sign extended at this point; CORE_ADDR will be a
64-bit type and it should be set to 0xffffffff_8fff0000 by read_reg.
That's the only reason we can get away with it.

>  Anyway, I have the following patch waiting in the queue for submission -- 
> perhaps it is less hackish. ;-)

Ewww.  Sorry, this one's even worse than mine :-) Let's not abuse our
types that way.  I have a nicer patch now based on Ulrich's
suggestion.  I'll post it in a few minutes when testing finishes (or a
little later if I can figure out where to get that board file you
mentioned).

> 2007-04-30  Maciej W. Rozycki  <macro@mips.com>
> 
> 	* dwarf2-frame.c (read_reg): Extract an address using the width
> 	of the register to be used to hold it as the size.

I see the read_reg part of this patch has been updated to apply to
HEAD, but do you know if it is older than this change?  If so, it's
probably not necessary anymore.

2006-05-17  Daniel Jacobowitz  <dan@codesourcery.com>

        * dwarf2-frame.c: Include "value.h".
        (read_reg): Use unpack_long and register_type.
        * Makefile.in (dwarf2-frame.o): Update.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] dwarf2 unwinder and MIPS n32

[Daniel Jacobowitz]

On Sun, Apr 29, 2007 at 12:40:41AM +0200, Ulrich Weigand wrote:
> Well, when we *read* a register in order to get (or compute) the CFA,
> read_reg does
> 
>   unpack_long (register_type (gdbarch, regnum), buf)
> 
> which in turn uses either extract_type_address, extract_signed_integer,
> or extract_unsigned_integer, depending on the type.
> 
> It may make sense to perform the analogous operation when *writing*
> the CFA to (an unwound copy of the contents of) a register.  There
> is no "pack_long", [...]

Well then, how about we add one?  Your mention of symmetry is I think
very important; we have a lot of different ways to manipulate longs /
addresses, and I'd rather reduce than increase the number, but the
first step is to make the interfaces we have consistent.

Tested on mips64-linux N32, where it still fixes the problem; unlike
the previous patch, this one should not break segmented architectures
that need to adjust pointers and addresses.

-- 
Daniel Jacobowitz
CodeSourcery

2007-04-30  Daniel Jacobowitz  <dan@codesourcery.com>

	* dwarf2-frame.c (dwarf2_frame_prev_register): Use pack_long
	instead of store_typed_address.
	* value.c (pack_long): New.
	(value_from_longest): Use it.
	* value.h (pack_long): New prototype.

---
 dwarf2-frame.c |   14 ++++----------
 value.c        |   36 ++++++++++++++++++++++--------------
 value.h        |    2 ++
 3 files changed, 28 insertions(+), 24 deletions(-)

Index: gdb/dwarf2-frame.c
===================================================================
--- gdb.orig/dwarf2-frame.c	2007-04-30 08:29:26.000000000 -0400
+++ gdb/dwarf2-frame.c	2007-04-30 08:34:48.000000000 -0400
@@ -1137,10 +1137,7 @@ dwarf2_frame_prev_register (struct frame
       *addrp = 0;
       *realnump = -1;
       if (valuep)
-	{
-	  /* Store the value.  */
-	  store_typed_address (valuep, builtin_type_void_data_ptr, cache->cfa);
-	}
+	pack_long (valuep, register_type (gdbarch, regnum), cache->cfa);
       break;
 
     case DWARF2_FRAME_REG_CFA_OFFSET:
@@ -1149,11 +1146,8 @@ dwarf2_frame_prev_register (struct frame
       *addrp = 0;
       *realnump = -1;
       if (valuep)
-	{
-	  /* Store the value.  */
-	  store_typed_address (valuep, builtin_type_void_data_ptr,
-			       cache->cfa + cache->reg[regnum].loc.offset);
-	}
+	pack_long (valuep, register_type (gdbarch, regnum),
+		   cache->cfa + cache->reg[regnum].loc.offset);
       break;
 
     case DWARF2_FRAME_REG_RA_OFFSET:
@@ -1167,7 +1161,7 @@ dwarf2_frame_prev_register (struct frame
 
           regnum = DWARF2_REG_TO_REGNUM (cache->retaddr_reg.loc.reg);
           pc += frame_unwind_register_unsigned (next_frame, regnum);
-          store_typed_address (valuep, builtin_type_void_func_ptr, pc);
+          pack_long (valuep, register_type (gdbarch, regnum), pc);
         }
       break;
 
Index: gdb/value.c
===================================================================
--- gdb.orig/value.c	2007-04-30 08:29:26.000000000 -0400
+++ gdb/value.c	2007-04-30 08:35:14.000000000 -0400
@@ -1505,23 +1505,18 @@ modify_field (gdb_byte *addr, LONGEST fi
   store_unsigned_integer (addr, sizeof oword, oword);
 }
 \f
-/* Convert C numbers into newly allocated values */
+/* Pack NUM into BUF using a target format of TYPE.  */
 
-struct value *
-value_from_longest (struct type *type, LONGEST num)
+void
+pack_long (gdb_byte *buf, struct type *type, LONGEST num)
 {
-  struct value *val = allocate_value (type);
-  enum type_code code;
   int len;
-retry:
-  code = TYPE_CODE (type);
+
+  type = check_typedef (type);
   len = TYPE_LENGTH (type);
 
-  switch (code)
+  switch (TYPE_CODE (type))
     {
-    case TYPE_CODE_TYPEDEF:
-      type = check_typedef (type);
-      goto retry;
     case TYPE_CODE_INT:
     case TYPE_CODE_CHAR:
     case TYPE_CODE_ENUM:
@@ -1529,17 +1524,30 @@ retry:
     case TYPE_CODE_BOOL:
     case TYPE_CODE_RANGE:
     case TYPE_CODE_MEMBERPTR:
-      store_signed_integer (value_contents_raw (val), len, num);
+      store_signed_integer (buf, len, num);
       break;
 
     case TYPE_CODE_REF:
     case TYPE_CODE_PTR:
-      store_typed_address (value_contents_raw (val), type, (CORE_ADDR) num);
+      store_typed_address (buf, type, (CORE_ADDR) num);
       break;
 
     default:
-      error (_("Unexpected type (%d) encountered for integer constant."), code);
+      error (_("Unexpected type (%d) encountered for integer constant."),
+	     TYPE_CODE (type));
     }
+}
+
+
+/* Convert C numbers into newly allocated values.  */
+
+struct value *
+value_from_longest (struct type *type, LONGEST num)
+{
+  struct value *val = allocate_value (type);
+
+  pack_long (value_contents_raw (val), type, num);
+
   return val;
 }
 
Index: gdb/value.h
===================================================================
--- gdb.orig/value.h	2007-04-30 08:29:26.000000000 -0400
+++ gdb/value.h	2007-04-30 08:29:57.000000000 -0400
@@ -271,6 +271,8 @@ extern LONGEST unpack_field_as_long (str
 				     const gdb_byte *valaddr,
 				     int fieldno);
 
+extern void pack_long (gdb_byte *buf, struct type *type, LONGEST num);
+
 extern struct value *value_from_longest (struct type *type, LONGEST num);
 extern struct value *value_from_pointer (struct type *type, CORE_ADDR addr);
 extern struct value *value_from_double (struct type *type, DOUBLEST num);

Re: [rfc] dwarf2 unwinder and MIPS n32

[Maciej W. Rozycki]

On Mon, 30 Apr 2007, Daniel Jacobowitz wrote:

> I see the read_reg part of this patch has been updated to apply to
> HEAD, but do you know if it is older than this change?  If so, it's
> probably not necessary anymore.

 Well, 2005-01-22 is the original date, so it is somewhat older indeed.  
I have not tried to check whether any of the changes have become redundant 
since that change -- the patch builds and causes no regressions as far as 
I can tell.  I would normally check the usefulness of these changes before 
submission.

  Maciej

Re: [rfc] dwarf2 unwinder and MIPS n32

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> Well then, how about we add one?  Your mention of symmetry is I think
> very important; we have a lot of different ways to manipulate longs /
> addresses, and I'd rather reduce than increase the number, but the
> first step is to make the interfaces we have consistent.
> 
> Tested on mips64-linux N32, where it still fixes the problem; unlike
> the previous patch, this one should not break segmented architectures
> that need to adjust pointers and addresses.

Looks good to me ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] dwarf2 unwinder and MIPS n32

[Daniel Jacobowitz]

On Mon, Apr 30, 2007 at 08:43:52PM +0200, Ulrich Weigand wrote:
> Daniel Jacobowitz wrote:
> 
> > Well then, how about we add one?  Your mention of symmetry is I think
> > very important; we have a lot of different ways to manipulate longs /
> > addresses, and I'd rather reduce than increase the number, but the
> > first step is to make the interfaces we have consistent.
> > 
> > Tested on mips64-linux N32, where it still fixes the problem; unlike
> > the previous patch, this one should not break segmented architectures
> > that need to adjust pointers and addresses.
> 
> Looks good to me ...

Thanks!  I checked it in.

-- 
Daniel Jacobowitz
CodeSourcery