[RFA][1/5] New port: Cell BE SPU (dwarf2loc.c fix)

[RFA][1/5] New port: Cell BE SPU (dwarf2loc.c fix)

[Ulrich Weigand]

Hello,

this fixes a problem in dwarf2loc.c that triggers for the SPU port.

The code in dwarf_expr_read_reg currently expects that in order to
retrieve an address value from a register, it's OK to assume the
address can always be extracted using extract_unsigned_integer
on the full size of the register.

This fails on the SPU, because all registers are 16 bytes wide, and
a pointer is represented by simply using the uppermost 4 bytes and
ignoring the contents of the remaining 12 bytes.  I would assume
that there's also other architectures where the assumption in 
dwarf_expr_read_reg might fail, e.g. due to sign extension issues
or other required fiddling.

So, to fix this I thought that we already *know* how to extract
values of a certain type from a register: value_from_register does
just that, and provides all sorts of configurability for the back
end to have it just do the right thing.  Since the value retrieved
by dwarf_expr_read_reg is always a pointer to some data object 
(normally on the stack), it should be OK to get it by using
value_from_register (builtin_type_void_data_ptr, ...).

This is what the patch below does, and it works fine on SPU.
Additionally tested without regressions on s390-ibm-linux and
s390x-ibm-linux.

OK?

Bye,
Ulrich

	* dwarf2loc.c (dwarf_expr_read_reg): Use value_from_register to
	retrieve address from register.


diff -ur gdb-orig/gdb/dwarf2loc.c gdb-head/gdb/dwarf2loc.c
--- gdb-orig/gdb/dwarf2loc.c	2006-11-10 03:00:22.000000000 +0100
+++ gdb-head/gdb/dwarf2loc.c	2006-11-11 18:16:58.663849888 +0100
@@ -115,23 +115,23 @@
 /* Helper functions for dwarf2_evaluate_loc_desc.  */
 
 /* Using the frame specified in BATON, return the value of register
-   REGNUM, treated as an unsigned integer.  */
+   REGNUM, treated as a pointer.  */
 static CORE_ADDR
 dwarf_expr_read_reg (void *baton, int dwarf_regnum)
 {
   struct dwarf_expr_baton *debaton = (struct dwarf_expr_baton *) baton;
+  struct value *value;
   CORE_ADDR result;
-  gdb_byte *buf;
-  int regnum, regsize;
+  int regnum;
 
   regnum = DWARF2_REG_TO_REGNUM (dwarf_regnum);
-  regsize = register_size (current_gdbarch, regnum);
-  buf = alloca (regsize);
 
-  frame_register_read (debaton->frame, regnum, buf);
-  /* NOTE: cagney/2003-05-22: This extract is assuming that a DWARF 2
-     address is always unsigned.  That may or may not be true.  */
-  result = extract_unsigned_integer (buf, regsize);
+  value = value_from_register (builtin_type_void_data_ptr,
+			       regnum, debaton->frame);
+
+  result = value_as_address (value);
+  release_value (value);
+  value_free (value);
 
   return result;
 }
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFA][1/5] New port: Cell BE SPU (dwarf2loc.c fix)

[Jim Blandy]

"Ulrich Weigand" <uweigand@de.ibm.com> writes:
> this fixes a problem in dwarf2loc.c that triggers for the SPU port.
>
> The code in dwarf_expr_read_reg currently expects that in order to
> retrieve an address value from a register, it's OK to assume the
> address can always be extracted using extract_unsigned_integer
> on the full size of the register.
>
> This fails on the SPU, because all registers are 16 bytes wide, and
> a pointer is represented by simply using the uppermost 4 bytes and
> ignoring the contents of the remaining 12 bytes.  I would assume
> that there's also other architectures where the assumption in 
> dwarf_expr_read_reg might fail, e.g. due to sign extension issues
> or other required fiddling.
>
> So, to fix this I thought that we already *know* how to extract
> values of a certain type from a register: value_from_register does
> just that, and provides all sorts of configurability for the back
> end to have it just do the right thing.  Since the value retrieved
> by dwarf_expr_read_reg is always a pointer to some data object 
> (normally on the stack), it should be OK to get it by using
> value_from_register (builtin_type_void_data_ptr, ...).
>
> This is what the patch below does, and it works fine on SPU.
> Additionally tested without regressions on s390-ibm-linux and
> s390x-ibm-linux.

This seems fine.  I tested it on i686 Fedora Core 5 and didn't see any
new failures.

Would it be a good idea to introduce a new function, a companion to
value_from_register, that wraps up the composition of
value_from_register and value_as_address?

Re: [RFA][1/5] New port: Cell BE SPU (dwarf2loc.c fix)

[Ulrich Weigand]

Jim Blandy wrote:

> This seems fine.  I tested it on i686 Fedora Core 5 and didn't see any
> new failures.
> 
> Would it be a good idea to introduce a new function, a companion to
> value_from_register, that wraps up the composition of
> value_from_register and value_as_address?

You mean something like the patch below?  Tested on spu, s390-ibm-linux
and s390x-ibm-linux.

Bye,
Ulrich


ChangeLog:

	* findvar.c (address_from_register): New function.
	* value.h (address_from_register): Add prototype.
	* dwarf2loc.c (dwarf_expr_read_reg): Use address_from_register.

diff -ur gdb-orig/gdb/dwarf2loc.c gdb-head/gdb/dwarf2loc.c
--- gdb-orig/gdb/dwarf2loc.c	2006-11-10 03:00:22.000000000 +0100
+++ gdb-head/gdb/dwarf2loc.c	2006-11-15 22:12:11.624037552 +0100
@@ -115,24 +115,17 @@
 /* Helper functions for dwarf2_evaluate_loc_desc.  */
 
 /* Using the frame specified in BATON, return the value of register
-   REGNUM, treated as an unsigned integer.  */
+   REGNUM, treated as a pointer.  */
 static CORE_ADDR
 dwarf_expr_read_reg (void *baton, int dwarf_regnum)
 {
   struct dwarf_expr_baton *debaton = (struct dwarf_expr_baton *) baton;
   CORE_ADDR result;
-  gdb_byte *buf;
-  int regnum, regsize;
+  int regnum;
 
   regnum = DWARF2_REG_TO_REGNUM (dwarf_regnum);
-  regsize = register_size (current_gdbarch, regnum);
-  buf = alloca (regsize);
-
-  frame_register_read (debaton->frame, regnum, buf);
-  /* NOTE: cagney/2003-05-22: This extract is assuming that a DWARF 2
-     address is always unsigned.  That may or may not be true.  */
-  result = extract_unsigned_integer (buf, regsize);
-
+  result = address_from_register (builtin_type_void_data_ptr,
+				  regnum, debaton->frame);
   return result;
 }
 
diff -ur gdb-orig/gdb/findvar.c gdb-head/gdb/findvar.c
--- gdb-orig/gdb/findvar.c	2006-01-17 23:30:29.000000000 +0100
+++ gdb-head/gdb/findvar.c	2006-11-15 22:09:11.884934888 +0100
@@ -728,6 +728,26 @@
   return v;
 }
 
+/* Return contents of register REGNUM in frame FRAME as address,
+   interpreted as value of type TYPE.   Will abort if register
+   value is not available.  */
+
+CORE_ADDR
+address_from_register (struct type *type, int regnum, struct frame_info *frame)
+{
+  struct value *value;
+  CORE_ADDR result;
+
+  value = value_from_register (type, regnum, frame);
+  gdb_assert (value);
+
+  result = value_as_address (value);
+  release_value (value);
+  value_free (value);
+
+  return result;
+}
+
 \f
 /* Given a struct symbol for a variable or function,
    and a stack frame id, 
diff -ur gdb-orig/gdb/value.h gdb-head/gdb/value.h
--- gdb-orig/gdb/value.h	2006-07-13 06:31:42.000000000 +0200
+++ gdb-head/gdb/value.h	2006-11-15 22:10:31.813034144 +0100
@@ -282,6 +282,9 @@
 extern struct value *value_from_register (struct type *type, int regnum,
 					  struct frame_info *frame);
 
+extern CORE_ADDR address_from_register (struct type *type, int regnum,
+					struct frame_info *frame);
+
 extern struct value *value_of_variable (struct symbol *var, struct block *b);
 
 extern struct value *value_of_register (int regnum, struct frame_info *frame);

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

Re: [RFA][1/5] New port: Cell BE SPU (dwarf2loc.c fix)

[Jim Blandy]

"Ulrich Weigand" <uweigand@de.ibm.com> writes:
>> Would it be a good idea to introduce a new function, a companion to
>> value_from_register, that wraps up the composition of
>> value_from_register and value_as_address?
>
> You mean something like the patch below?  Tested on spu, s390-ibm-linux
> and s390x-ibm-linux.

Yes, that's great.  Please commit.

It seems a shame to be creating a value and then throwing it away, but
the composition of those two functions has the semantics we want, so I
don't see a good alternative without some more restructuring.

Re: [RFA][1/5] New port: Cell BE SPU (dwarf2loc.c fix)

[Daniel Jacobowitz]

On Tue, Nov 21, 2006 at 05:43:05PM -0800, Jim Blandy wrote:
> It seems a shame to be creating a value and then throwing it away, but
> the composition of those two functions has the semantics we want, so I
> don't see a good alternative without some more restructuring.

Actually, I think we should be doing it more often.  It doesn't live
long, there's a clearly defined mechanism for throwing it away, and it
encapsulates that the object should have target semantics.  Eventually,
if we ever get around to disallowing math on CORE_ADDR, I think this is
what we'll end up with.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA][1/5] New port: Cell BE SPU (dwarf2loc.c fix)

[Ulrich Weigand]

Jim Blandy wrote:
> Yes, that's great.  Please commit.

I've committed the patch now.  Many thanks for reviewing!

Bye,
Ulrich

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