[RFA/RFC] dwarf2-frame read_reg

[RFA/RFC] dwarf2-frame read_reg

[Michael Snyder]

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

I want you guys to vett this change.  I was getting wrong results
on a target where sizeof (SP) != sizeof (void *).  The local func
read_reg was calling extract_unsigned_integer with the wrong size.




[-- Attachment #2: read_reg --]
[-- Type: text/plain, Size: 933 bytes --]

2006-04-11  Michael Snyder  <msnyder@redhat.com>

	* dwarf2-frame.c (read_reg): Use register type instead of 
	builtin_data_pointer_type to extract register's value.

Index: dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.59
diff -p -r1.59 dwarf2-frame.c
*** dwarf2-frame.c	5 Apr 2006 20:01:19 -0000	1.59
--- dwarf2-frame.c	12 Apr 2006 03:30:08 -0000
*************** read_reg (void *baton, int reg)
*** 214,220 ****
  
    buf = alloca (register_size (gdbarch, regnum));
    frame_unwind_register (next_frame, regnum, buf);
!   return extract_typed_address (buf, builtin_type_void_data_ptr);
  }
  
  static void
--- 214,220 ----
  
    buf = alloca (register_size (gdbarch, regnum));
    frame_unwind_register (next_frame, regnum, buf);
!   return extract_typed_address (buf, register_type (gdbarch, regnum));
  }
  
  static void

Re: [RFA/RFC] dwarf2-frame read_reg

[Jim Blandy]

On 4/11/06, Michael Snyder <msnyder@redhat.com> wrote:
> I want you guys to vett this change.  I was getting wrong results
> on a target where sizeof (SP) != sizeof (void *).  The local func
> read_reg was calling extract_unsigned_integer with the wrong size.

Well, extract_typed_address requires the type of the register to be
some sort of pointer.  read_reg is given as a callback to the Dwarf
expression evaluator in dwarf2expr.c, so it could be handed any
register at all.

How about unpack_long (buf, register_type (gdbarch, regnum))? 
Definitely regression-test this on several platforms...

Re: [RFA/RFC] dwarf2-frame read_reg

[Mark Kettenis]

> Date: Tue, 11 Apr 2006 21:42:01 -0700
> From: "Jim Blandy" <jimb@red-bean.com>
> 
> On 4/11/06, Michael Snyder <msnyder@redhat.com> wrote:
> > I want you guys to vett this change.  I was getting wrong results
> > on a target where sizeof (SP) != sizeof (void *).  The local func
> > read_reg was calling extract_unsigned_integer with the wrong size.
> 
> Well, extract_typed_address requires the type of the register to be
> some sort of pointer.  read_reg is given as a callback to the Dwarf
> expression evaluator in dwarf2expr.c, so it could be handed any
> register at all.
> 
> How about unpack_long (buf, register_type (gdbarch, regnum))? 
> Definitely regression-test this on several platforms...

This is likely to be wrong for platforms where addresses are signed.

Michael, what kind of funny target is this?  It has a stack pointer
register, but its size is different from the size of pointers?

Mark

Re: [RFA/RFC] dwarf2-frame read_reg

[Michael Snyder]

Mark Kettenis wrote:
>>Date: Tue, 11 Apr 2006 21:42:01 -0700
>>From: "Jim Blandy" <jimb@red-bean.com>
>>
>>On 4/11/06, Michael Snyder <msnyder@redhat.com> wrote:
>>
>>>I want you guys to vett this change.  I was getting wrong results
>>>on a target where sizeof (SP) != sizeof (void *).  The local func
>>>read_reg was calling extract_unsigned_integer with the wrong size.
>>
>>Well, extract_typed_address requires the type of the register to be
>>some sort of pointer.  read_reg is given as a callback to the Dwarf
>>expression evaluator in dwarf2expr.c, so it could be handed any
>>register at all.
>>
>>How about unpack_long (buf, register_type (gdbarch, regnum))? 
>>Definitely regression-test this on several platforms...
> 
> 
> This is likely to be wrong for platforms where addresses are signed.
> 
> Michael, what kind of funny target is this?  It has a stack pointer
> register, but its size is different from the size of pointers?

Yep, the reg size is 3 bytes.  When it's pushed onto the stack,
it occupies 3 bytes of memory.  Funky, huh?  The cpu can only
reach 24 bits worth of address space.

Re: [RFA/RFC] dwarf2-frame read_reg

[Michael Snyder]

Jim Blandy wrote:
> On 4/11/06, Michael Snyder <msnyder@redhat.com> wrote:
> 
>>I want you guys to vett this change.  I was getting wrong results
>>on a target where sizeof (SP) != sizeof (void *).  The local func
>>read_reg was calling extract_unsigned_integer with the wrong size.
> 
> 
> Well, extract_typed_address requires the type of the register to be
> some sort of pointer.  read_reg is given as a callback to the Dwarf
> expression evaluator in dwarf2expr.c, so it could be handed any
> register at all.
> 
> How about unpack_long (buf, register_type (gdbarch, regnum))? 
> Definitely regression-test this on several platforms...
> 

Hmmm... extract_typed_address calls POINTER_TO_ADDRESS, which
seems more likely to "do the right thing".

How about this?

   if (TYPE_CODE (register_type (gdbarch, regnum)) == TYPE_CODE_PTR)
     return extract_typed_address (buf, register_type (gdbarch, regnum));
   else
     return extract_typed_address (buf, builtin_type_void_data_ptr);

(I am assuming that a register_type is unlikely to be TYPE_CODE_REF)

Re: [RFA/RFC] dwarf2-frame read_reg

[Daniel Jacobowitz]

On Wed, Apr 12, 2006 at 08:17:07PM +0200, Mark Kettenis wrote:
> > Date: Tue, 11 Apr 2006 21:42:01 -0700
> > From: "Jim Blandy" <jimb@red-bean.com>
> > 
> > On 4/11/06, Michael Snyder <msnyder@redhat.com> wrote:
> > > I want you guys to vett this change.  I was getting wrong results
> > > on a target where sizeof (SP) != sizeof (void *).  The local func
> > > read_reg was calling extract_unsigned_integer with the wrong size.
> > 
> > Well, extract_typed_address requires the type of the register to be
> > some sort of pointer.  read_reg is given as a callback to the Dwarf
> > expression evaluator in dwarf2expr.c, so it could be handed any
> > register at all.
> > 
> > How about unpack_long (buf, register_type (gdbarch, regnum))? 
> > Definitely regression-test this on several platforms...
> 
> This is likely to be wrong for platforms where addresses are signed.

It shouldn't be.

unpack_long (struct type *type, const gdb_byte *valaddr)
{
...
    case TYPE_CODE_PTR:
    case TYPE_CODE_REF:
      /* Assume a CORE_ADDR can fit in a LONGEST (for now).  Not sure
         whether we want this to be true eventually.  */
      return extract_typed_address (valaddr, type);

which calls POINTER_TO_ADDRESS.  And that will be the signed unpack for
MIPS, and the unsigned unpack for other targets.

So I think unpack_long is a good choice.

(I didn't realize that before.  I think I have another pending patch
that this would be useful for - maybe the psaddr_t one?)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA/RFC] dwarf2-frame read_reg

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Wed, Apr 12, 2006 at 08:17:07PM +0200, Mark Kettenis wrote:
> 
>>>Date: Tue, 11 Apr 2006 21:42:01 -0700
>>>From: "Jim Blandy" <jimb@red-bean.com>
>>>
>>>On 4/11/06, Michael Snyder <msnyder@redhat.com> wrote:
>>>
>>>>I want you guys to vett this change.  I was getting wrong results
>>>>on a target where sizeof (SP) != sizeof (void *).  The local func
>>>>read_reg was calling extract_unsigned_integer with the wrong size.
>>>
>>>Well, extract_typed_address requires the type of the register to be
>>>some sort of pointer.  read_reg is given as a callback to the Dwarf
>>>expression evaluator in dwarf2expr.c, so it could be handed any
>>>register at all.
>>>
>>>How about unpack_long (buf, register_type (gdbarch, regnum))? 
>>>Definitely regression-test this on several platforms...
>>
>>This is likely to be wrong for platforms where addresses are signed.
> 
> 
> It shouldn't be.
> 
> unpack_long (struct type *type, const gdb_byte *valaddr)
> {
> ...
>     case TYPE_CODE_PTR:
>     case TYPE_CODE_REF:
>       /* Assume a CORE_ADDR can fit in a LONGEST (for now).  Not sure
>          whether we want this to be true eventually.  */
>       return extract_typed_address (valaddr, type);
> 
> which calls POINTER_TO_ADDRESS.  And that will be the signed unpack for
> MIPS, and the unsigned unpack for other targets.
> 
> So I think unpack_long is a good choice.
> 
> (I didn't realize that before.  I think I have another pending patch
> that this would be useful for - maybe the psaddr_t one?)

May I release the patch into your care?  I don't really think
I understand the problem domain well enough (wouldn't be able
to test it well), and I've made my own need for it go away by
other means.

Re: [RFA/RFC] dwarf2-frame read_reg

[Mark Kettenis]

> X-Spam-Checker-Version: SpamAssassin 3.1.0 (2005-09-13) on 
> 	elgar.sibelius.xs4all.nl
> X-Spam-Level: 
> X-Spam-Status: No, score=0.0 required=5.0 tests=none autolearn=failed 
> 	version=3.1.0
> Date: Thu, 20 Apr 2006 13:21:12 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: jimb@red-bean.com, msnyder@redhat.com, gdb-patches@sourceware.org
> Mail-Followup-To: Mark Kettenis <mark.kettenis@xs4all.nl>, 	jimb@red-bean.com, msnyder@redhat.com, gdb-patches@sourceware.org
> Content-Disposition: inline
> X-IsSubscribed: yes
> Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm
> Sender: gdb-patches-owner@sourceware.org
> X-UTwente-MailScanner-Information: Scanned by MailScanner. Contact helpdesk@ITBE.utwente.nl for more information.
> X-UTwente-MailScanner: Found to be clean
> X-MailScanner-From: gdb-patches-return-43755-m.m.kettenis=alumnus.utwente.nl@sourceware.org
> X-XS4ALL-DNSBL-Checked: mxdrop5.xs4all.nl checked 192.87.17.19 against DNS blacklists
> X-Virus-Scanned: by XS4ALL Virus Scanner
> X-XS4ALL-Spam-Score: 0 () 
> X-XS4ALL-Spam: NO
> Envelope-To: mark.kettenis@xs4all.nl
> X-UIDL: 1145553701._smtp.mxdrop5.43414,S=4323
> 
> On Wed, Apr 12, 2006 at 08:17:07PM +0200, Mark Kettenis wrote:
> > > Date: Tue, 11 Apr 2006 21:42:01 -0700
> > > From: "Jim Blandy" <jimb@red-bean.com>
> > > 
> > > On 4/11/06, Michael Snyder <msnyder@redhat.com> wrote:
> > > > I want you guys to vett this change.  I was getting wrong results
> > > > on a target where sizeof (SP) != sizeof (void *).  The local func
> > > > read_reg was calling extract_unsigned_integer with the wrong size.
> > > 
> > > Well, extract_typed_address requires the type of the register to be
> > > some sort of pointer.  read_reg is given as a callback to the Dwarf
> > > expression evaluator in dwarf2expr.c, so it could be handed any
> > > register at all.
> > > 
> > > How about unpack_long (buf, register_type (gdbarch, regnum))? 
> > > Definitely regression-test this on several platforms...
> > 
> > This is likely to be wrong for platforms where addresses are signed.
> 
> It shouldn't be.

Ok, cool.  Objection withdrawn.

Mark

Re: [RFA/RFC] dwarf2-frame read_reg

[Daniel Jacobowitz]

On Tue, Apr 11, 2006 at 08:34:01PM -0700, Michael Snyder wrote:
> I want you guys to vett this change.  I was getting wrong results
> on a target where sizeof (SP) != sizeof (void *).  The local func
> read_reg was calling extract_unsigned_integer with the wrong size.

> 2006-04-11  Michael Snyder  <msnyder@redhat.com>
> 
> 	* dwarf2-frame.c (read_reg): Use register type instead of 
> 	builtin_data_pointer_type to extract register's value.

I think the thread consensus was to do the attached.  I've tested it on
x86_64, and I'm fairly confident in it.  Any concerns?  (Concerns that
come with test results for some other platform are the best kind!)

-- 
Daniel Jacobowitz
CodeSourcery

2006-05-05  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.

Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.811
diff -u -p -r1.811 Makefile.in
--- Makefile.in	23 Apr 2006 14:15:01 -0000	1.811
+++ Makefile.in	5 May 2006 19:41:34 -0000
@@ -1907,7 +1907,8 @@ dwarf2expr.o: dwarf2expr.c $(defs_h) $(s
 dwarf2-frame.o: dwarf2-frame.c $(defs_h) $(dwarf2expr_h) $(elf_dwarf2_h) \
 	$(frame_h) $(frame_base_h) $(frame_unwind_h) $(gdbcore_h) \
 	$(gdbtypes_h) $(symtab_h) $(objfiles_h) $(regcache_h) \
-	$(gdb_assert_h) $(gdb_string_h) $(complaints_h) $(dwarf2_frame_h)
+	$(gdb_assert_h) $(gdb_string_h) $(complaints_h) $(dwarf2_frame_h) \
+	$(value_h)
 dwarf2loc.o: dwarf2loc.c $(defs_h) $(ui_out_h) $(value_h) $(frame_h) \
 	$(gdbcore_h) $(target_h) $(inferior_h) $(ax_h) $(ax_gdb_h) \
 	$(regcache_h) $(objfiles_h) $(exceptions_h) $(elf_dwarf2_h) \
Index: dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.59
diff -u -p -r1.59 dwarf2-frame.c
--- dwarf2-frame.c	5 Apr 2006 20:01:19 -0000	1.59
+++ dwarf2-frame.c	5 May 2006 19:41:35 -0000
@@ -32,6 +32,7 @@
 #include "symtab.h"
 #include "objfiles.h"
 #include "regcache.h"
+#include "value.h"
 
 #include "gdb_assert.h"
 #include "gdb_string.h"
@@ -214,7 +215,13 @@ read_reg (void *baton, int reg)
 
   buf = alloca (register_size (gdbarch, regnum));
   frame_unwind_register (next_frame, regnum, buf);
-  return extract_typed_address (buf, builtin_type_void_data_ptr);
+
+  /* Convert the register to an integer.  This returns a LONGEST
+     rather than a CORE_ADDR, but unpack_pointer does the same thing
+     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);
 }
 
 static void

Re: [RFA/RFC] dwarf2-frame read_reg

[Daniel Jacobowitz]

On Fri, May 05, 2006 at 03:44:29PM -0400, Daniel Jacobowitz wrote:
> I think the thread consensus was to do the attached.  I've tested it on
> x86_64, and I'm fairly confident in it.  Any concerns?  (Concerns that
> come with test results for some other platform are the best kind!)

> 2006-05-05  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.

There weren't any concerns, so I have committed this.

-- 
Daniel Jacobowitz
CodeSourcery