[rfc] Wrap addresses in spu-gdb

[rfc] Wrap addresses in spu-gdb

[Markus Deuling]

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

Hi,

this patch makes GDB aware of a special feature of the SPU hardware. Every SPU has its own dedicated memory, called the Local Store (LS). Currently this is 256K of memory. If accessing memory above this 256K, the address is wrapped to fit in 256K (modulo calculation). So every memory address is valid. The size of the Local Store is found in the SPU's LSLR register.

This examples tries to access an address > LS size and runs into an error which is fixed by this patch:

(gdb) p *0xfffd0162
Error accessing memory address 0xfffd0162: Invalid argument.

The patch introduces three new functions to spu-tdep for address conversion which also will be used by the Cell Broadband Engine combined debugger.

ChangeLog:

	* spu-tdep.c (spu_address_to_pointer): New function.
	(spu_pointer_to_address): Likewise.
	(spu_integer_to_address): Likewise.
	(spu_gdbarch_init): Add spu_address_to_pointer, spu_pointer_to_address
	and spu_integer_to_address to gdbarch.


Tested on spu-elf. Testsuite showed no regression. 

Ok to commit?

-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com






[-- Attachment #2: diff-spu-wrap-addr --]
[-- Type: text/plain, Size: 2152 bytes --]

diff -urpN src/gdb/spu-tdep.c dev/gdb/spu-tdep.c
--- src/gdb/spu-tdep.c	2007-06-18 05:36:44.000000000 +0200
+++ dev/gdb/spu-tdep.c	2007-08-08 08:39:41.000000000 +0200
@@ -324,6 +324,47 @@ spu_register_reggroup_p (struct gdbarch 
   return default_register_reggroup_p (gdbarch, regnum, group);
 }
 
+/* Address conversion.  */
+
+static void
+spu_address_to_pointer (struct type *type, gdb_byte *buf, CORE_ADDR addr)
+{
+  store_unsigned_integer (buf, TYPE_LENGTH (type), addr);
+}
+
+static CORE_ADDR
+spu_pointer_to_address (struct type *type, const gdb_byte *buf)
+{
+  ULONGEST addr = extract_unsigned_integer (buf, TYPE_LENGTH (type));
+  ULONGEST lslr = SPU_LS_SIZE - 1; /* Hard-wired LS size.  */
+
+  if (target_has_registers && target_has_stack && target_has_memory)
+    {
+      struct frame_info *frame = get_selected_frame (NULL);
+      lslr = get_frame_register_unsigned (frame, SPU_LSLR_REGNUM);
+    }
+
+  return addr & lslr;
+}
+
+static CORE_ADDR
+spu_integer_to_address (struct gdbarch *gdbarch,
+			struct type *type, const gdb_byte *buf)
+{
+  ULONGEST addr = unpack_long (type, buf);
+  ULONGEST lslr = SPU_LS_SIZE - 1; /* Hard-wired LS size.  */
+
+  if (target_has_registers && target_has_stack && target_has_memory
+      /* FIXME: Currently needed for dwarf2_read_address to work.  */
+      && type != builtin_type_uint32)
+    {
+      struct frame_info *frame = get_selected_frame (NULL);
+      lslr = get_frame_register_unsigned (frame, SPU_LSLR_REGNUM);
+    }
+
+  return addr & lslr;
+}
+
 
 /* Decoding SPU instructions.  */
 
@@ -2008,6 +2049,11 @@ spu_gdbarch_init (struct gdbarch_info in
   set_gdbarch_double_format (gdbarch, floatformats_ieee_double);
   set_gdbarch_long_double_format (gdbarch, floatformats_ieee_double);
 
+  /* Address conversion.  */
+  set_gdbarch_address_to_pointer (gdbarch, spu_address_to_pointer);
+  set_gdbarch_pointer_to_address (gdbarch, spu_pointer_to_address);
+  set_gdbarch_integer_to_address (gdbarch, spu_integer_to_address);
+
   /* Inferior function calls.  */
   set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
   set_gdbarch_frame_align (gdbarch, spu_frame_align);

Re: [rfc] Wrap addresses in spu-gdb

[Daniel Jacobowitz]

On Wed, Aug 08, 2007 at 08:44:29AM +0200, Markus Deuling wrote:
> +  if (target_has_registers && target_has_stack && target_has_memory
> +      /* FIXME: Currently needed for dwarf2_read_address to work.  */
> +      && type != builtin_type_uint32)

I think that you shouldn't commit a patch with this hack in it.
It looks very fragile.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Wrap addresses in spu-gdb

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Wed, Aug 08, 2007 at 08:44:29AM +0200, Markus Deuling wrote:
> > +  if (target_has_registers && target_has_stack && target_has_memory
> > +      /* FIXME: Currently needed for dwarf2_read_address to work.  */
> > +      && type != builtin_type_uint32)
> 
> I think that you shouldn't commit a patch with this hack in it.
> It looks very fragile.

Agreed.  Markus, this is a special hack for the combined PPE/SPE
debugger, it shouldn't be necessary for the SPU-standalone (and
mainline) version.

I'll have to look into fixing the underlying issue cleanly when
merging the combined debugger ...

Bye,
Ulrich

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

Re: [rfc] Wrap addresses in spu-gdb

[Jim Blandy]

"Ulrich Weigand" <uweigand@de.ibm.com> writes:
> Daniel Jacobowitz wrote:
>> On Wed, Aug 08, 2007 at 08:44:29AM +0200, Markus Deuling wrote:
>> > +  if (target_has_registers && target_has_stack && target_has_memory
>> > +      /* FIXME: Currently needed for dwarf2_read_address to work.  */
>> > +      && type != builtin_type_uint32)
>> 
>> I think that you shouldn't commit a patch with this hack in it.
>> It looks very fragile.
>
> Agreed.  Markus, this is a special hack for the combined PPE/SPE
> debugger, it shouldn't be necessary for the SPU-standalone (and
> mainline) version.
>
> I'll have to look into fixing the underlying issue cleanly when
> merging the combined debugger ...

There seem to be a number of cases where conversions between addresses
and pointers require live values from other registers --- for example,
when a pointer's bits are augmented with bits from some system
register, like a segment base register.  I'd want to work in the
context of real-world examples, but it might be a good idea to
introduce frame_and_pointer_to_address and
frame_and_address_to_pointer, and use them where they make sense.

Using such functions, clipping SPE addresses using the LSLR register
could be carried out in a reasonable way.

Re: [rfc] Wrap addresses in spu-gdb

[Markus Deuling]

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

Hi,

Ulrich Weigand schrieb:
> Daniel Jacobowitz wrote:
>> On Wed, Aug 08, 2007 at 08:44:29AM +0200, Markus Deuling wrote:
>>> +  if (target_has_registers && target_has_stack && target_has_memory
>>> +      /* FIXME: Currently needed for dwarf2_read_address to work.  */
>>> +      && type != builtin_type_uint32)
>> I think that you shouldn't commit a patch with this hack in it.
>> It looks very fragile.
> 
> Agreed.  Markus, this is a special hack for the combined PPE/SPE
> debugger, it shouldn't be necessary for the SPU-standalone (and
> mainline) version.
> 
> I'll have to look into fixing the underlying issue cleanly when
> merging the combined debugger ...

ok, thank you. I attached a patch with a SPU-only version. This can
later be replaced by the version suitable for the combined debugger.

Tested on spu-elf without regression.

ChangeLog:

	* spu-tdep.c (spu_pointer_to_address): New function.
	(spu_integer_to_address): Likewise.
	(spu_gdbarch_init): Add spu_pointer_to_address and 
	spu_integer_to_address to gdbarch.

Is this ok to commit?

-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com




[-- Attachment #2: diff-spu-wrap-addr --]
[-- Type: text/plain, Size: 1717 bytes --]

diff -urpN src/gdb/spu-tdep.c dev/gdb/spu-tdep.c
--- src/gdb/spu-tdep.c	2007-06-18 05:36:44.000000000 +0200
+++ dev/gdb/spu-tdep.c	2007-08-09 12:13:20.000000000 +0200
@@ -324,6 +324,35 @@ spu_register_reggroup_p (struct gdbarch 
   return default_register_reggroup_p (gdbarch, regnum, group);
 }
 
+/* Address conversion.  */
+
+static CORE_ADDR
+spu_pointer_to_address (struct type *type, const gdb_byte *buf)
+{
+  ULONGEST addr = extract_unsigned_integer (buf, TYPE_LENGTH (type));
+  ULONGEST lslr = SPU_LS_SIZE - 1; /* Hard-wired LS size.  */
+
+  if (target_has_registers && target_has_stack && target_has_memory)
+    lslr = get_frame_register_unsigned (get_selected_frame (NULL),
+					SPU_LSLR_REGNUM);
+
+  return addr & lslr;
+}
+
+static CORE_ADDR
+spu_integer_to_address (struct gdbarch *gdbarch,
+			struct type *type, const gdb_byte *buf)
+{
+  ULONGEST addr = unpack_long (type, buf);
+  ULONGEST lslr = SPU_LS_SIZE - 1; /* Hard-wired LS size.  */
+
+  if (target_has_registers && target_has_stack && target_has_memory)
+    lslr = get_frame_register_unsigned (get_selected_frame (NULL),
+					SPU_LSLR_REGNUM);
+
+  return addr & lslr;
+}
+
 
 /* Decoding SPU instructions.  */
 
@@ -2008,6 +2037,10 @@ spu_gdbarch_init (struct gdbarch_info in
   set_gdbarch_double_format (gdbarch, floatformats_ieee_double);
   set_gdbarch_long_double_format (gdbarch, floatformats_ieee_double);
 
+  /* Address conversion.  */
+  set_gdbarch_pointer_to_address (gdbarch, spu_pointer_to_address);
+  set_gdbarch_integer_to_address (gdbarch, spu_integer_to_address);
+
   /* Inferior function calls.  */
   set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
   set_gdbarch_frame_align (gdbarch, spu_frame_align);

Re: [rfc] Wrap addresses in spu-gdb

[Ulrich Weigand]

Markus Deuling wrote:

> Tested on spu-elf without regression.

Well, I'm seeing these regressions:
FAIL: gdb.cp/cp-relocate.exp: break *'int func<1>(int)'
FAIL: gdb.cp/cp-relocate.exp: break *'int func<2>(int)'

Could you investigate those?

Thanks,
Ulrich

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

Re: [rfc] Wrap addresses in spu-gdb

[Markus Deuling]

Hi,

Ulrich Weigand schrieb:
> Well, I'm seeing these regressions:
actually. I must have overseen these two.

> FAIL: gdb.cp/cp-relocate.exp: break *'int func<1>(int)'
> FAIL: gdb.cp/cp-relocate.exp: break *'int func<2>(int)'
> Could you investigate those?

In the gdb.cp/cp-relocate.exp testcase "add-symbol-file" tries to load two 
sections of an objfile at hard-wired addresses 0x40000 and 0x80000.

Within SPU these addresses are wrapped to the size of the Local Store
(which is btw 0x40000 :-) ) so 0x40000 and 0x80000 are wrapped to 0x0.

break *'int func<1>(int)'^M
Breakpoint 1 at 0x40: file ../../../src/gdb/testsuite/gdb.cp/cp-relocate.cc, line 21.^M
(gdb) FAIL: gdb.cp/cp-relocate.exp: break *'int func<1>(int)'
break *'int func<2>(int)'^M
Breakpoint 2 at 0x68: file ../../../src/gdb/testsuite/gdb.cp/cp-relocate.cc, line 21.^M (gdb) 
FAIL: gdb.cp/cp-relocate.exp: break *'int func<2>(in

As seen here the original addresses of the two functions (detected in the
beginning of the testcase: 0x40 and 0x68) remain as addresses for the functions.
The arent replaced by 0x0.

If the testcase addresses are changed to, for example, 0x40100 and 0x80100 the wrapping 
produces addresses <> 0x0. Now the adresses for the functions are set (correctly):

break *'int func<1>(int)'^M
Breakpoint 1 at 0x100: file ../../../src/gdb/testsuite/gdb.cp/cp-relocate.cc, line 21.^M
(gdb) FAIL: gdb.cp/cp-relocate.exp: break *'int func<1>(int)'
break *'int func<2>(int)'^M
Note: breakpoint 1 also set at pc 0x100.^M
Breakpoint 2 at 0x100: file ../../../src/gdb/testsuite/gdb.cp/cp-relocate.cc, line 21.^M
(gdb) FAIL: gdb.cp/cp-relocate.exp: break *'int func<2>(int)'

In this case both are set to 0x100 which is correct for SPU:
(0x40100 % SPU_LS_SIZE) == (0x80100 % SPU_LS_SIZE) == 0x100.

So there are three issues:

1) Should add-symbol-file set the function address to -in this case- 0x0 ?
   SPU hardware would handle both 0x40000 and 0x80000 as 0x0.

2) As seen in my example "add-symbol-file" loads two functions to the same addresses 0x100.
   Is this valid? 

3) Should the used addresses in the testcase be changed to, for example,
   0x10000 and 0x20000? This would work for SPU, too. Or shall I introduce
   variables for this addresses and set them to a range < SPU_LS_SIZE for
   SPU targets only?

What do you think?

-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com

Re: [rfc] Wrap addresses in spu-gdb

[Daniel Jacobowitz]

On Tue, Aug 21, 2007 at 11:56:07AM +0200, Markus Deuling wrote:
> So there are three issues:
> 
> 1) Should add-symbol-file set the function address to -in this case- 0x0 ?
>   SPU hardware would handle both 0x40000 and 0x80000 as 0x0.

I don't know the answer to this one, but the current behavior is
definitely strange.  My guess is that something has wrapped the values
to zero and decided that meant unspecified.

> 2) As seen in my example "add-symbol-file" loads two functions to the same 
> addresses 0x100.
>   Is this valid? 

Yes.

> 3) Should the used addresses in the testcase be changed to, for example,
>   0x10000 and 0x20000? This would work for SPU, too. Or shall I introduce
>   variables for this addresses and set them to a range < SPU_LS_SIZE for
>   SPU targets only?

I think changing the addresses is fine.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Wrap addresses in spu-gdb

[Markus Deuling]

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

Hi,

Daniel Jacobowitz schrieb:
> 
>> 3) Should the used addresses in the testcase be changed to, for example,
>>   0x10000 and 0x20000? This would work for SPU, too. Or shall I introduce
>>   variables for this addresses and set them to a range < SPU_LS_SIZE for
>>   SPU targets only?
> 
> I think changing the addresses is fine.
> 

attached is the original patch for address wrapping in SPU plus a change of the addresses
in gdb.cp/cp-relocate.exp to match size of SPU Local Store.

This patch showed no regression on SPU-

ChangeLog gdb/: 

	* spu-tdep.c (spu_pointer_to_address): New function.
	(spu_integer_to_address): Likewise.
	(spu_gdbarch_init): Add spu_pointer_to_address and 
	spu_integer_to_address to gdbarch.


ChangeLog gdb/testsuite/:

	* gdb.cp/cp-relocate.exp (add-symbol-file): Change addresses
	to fit into SPU Local Store memory.


Is this ok ?

-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com



[-- Attachment #2: diff-spu-wrap-addr --]
[-- Type: text/plain, Size: 2836 bytes --]

diff -urpN src/gdb/spu-tdep.c dev/gdb/spu-tdep.c
--- src/gdb/spu-tdep.c	2007-08-24 04:24:10.000000000 +0200
+++ dev/gdb/spu-tdep.c	2007-08-27 13:57:37.000000000 +0200
@@ -322,6 +322,35 @@ spu_register_reggroup_p (struct gdbarch 
   return default_register_reggroup_p (gdbarch, regnum, group);
 }
 
+/* Address conversion.  */
+
+static CORE_ADDR
+spu_pointer_to_address (struct type *type, const gdb_byte *buf)
+{
+  ULONGEST addr = extract_unsigned_integer (buf, TYPE_LENGTH (type));
+  ULONGEST lslr = SPU_LS_SIZE - 1; /* Hard-wired LS size.  */
+
+  if (target_has_registers && target_has_stack && target_has_memory)
+    lslr = get_frame_register_unsigned (get_selected_frame (NULL),
+					SPU_LSLR_REGNUM);
+
+  return addr & lslr;
+}
+
+static CORE_ADDR
+spu_integer_to_address (struct gdbarch *gdbarch,
+			struct type *type, const gdb_byte *buf)
+{
+  ULONGEST addr = unpack_long (type, buf);
+  ULONGEST lslr = SPU_LS_SIZE - 1; /* Hard-wired LS size.  */
+
+  if (target_has_registers && target_has_stack && target_has_memory)
+    lslr = get_frame_register_unsigned (get_selected_frame (NULL),
+					SPU_LSLR_REGNUM);
+
+  return addr & lslr;
+}
+
 
 /* Decoding SPU instructions.  */
 
@@ -2006,6 +2035,10 @@ spu_gdbarch_init (struct gdbarch_info in
   set_gdbarch_double_format (gdbarch, floatformats_ieee_double);
   set_gdbarch_long_double_format (gdbarch, floatformats_ieee_double);
 
+  /* Address conversion.  */
+  set_gdbarch_pointer_to_address (gdbarch, spu_pointer_to_address);
+  set_gdbarch_integer_to_address (gdbarch, spu_integer_to_address);
+
   /* Inferior function calls.  */
   set_gdbarch_call_dummy_location (gdbarch, ON_STACK);
   set_gdbarch_frame_align (gdbarch, spu_frame_align);
diff -urpN src/gdb/testsuite/gdb.cp/cp-relocate.exp dev/gdb/testsuite/gdb.cp/cp-relocate.exp
--- src/gdb/testsuite/gdb.cp/cp-relocate.exp	2007-08-24 04:24:29.000000000 +0200
+++ dev/gdb/testsuite/gdb.cp/cp-relocate.exp	2007-08-27 13:57:37.000000000 +0200
@@ -123,7 +123,7 @@ gdb_exit
 gdb_start
 gdb_reinitialize_dir $srcdir/$subdir
 
-gdb_test "add-symbol-file ${binfile} 0 -s ${func1_sec} 0x40000 -s ${func2_sec} 0x80000" \
+gdb_test "add-symbol-file ${binfile} 0 -s ${func1_sec} 0x10000 -s ${func2_sec} 0x20000" \
 	"Reading symbols from .*${testfile}\\.o\\.\\.\\.done\\.(|\r\nUsing host libthread_db library .*libthread_db.so.*\\.)" \
 	"add-symbol-file ${testfile}.o" \
 	"add symbol table from file \".*${testfile}\\.o\" at.*\\(y or n\\) " \
@@ -131,6 +131,6 @@ gdb_test "add-symbol-file ${binfile} 0 -
 
 # Make sure the function addresses were updated.
 gdb_test "break *'$func1_name'" \
-    "Breakpoint $decimal at 0x4....: file .*"
+    "Breakpoint $decimal at 0x1....: file .*"
 gdb_test "break *'$func2_name'" \
-    "Breakpoint $decimal at 0x8....: file .*"
+    "Breakpoint $decimal at 0x2....: file .*"

Re: [rfc] Wrap addresses in spu-gdb

[Ulrich Weigand]

Markus Deuling wrote:

> ChangeLog gdb/: 
> 
> 	* spu-tdep.c (spu_pointer_to_address): New function.
> 	(spu_integer_to_address): Likewise.
> 	(spu_gdbarch_init): Add spu_pointer_to_address and 
> 	spu_integer_to_address to gdbarch.
> 
> 
> ChangeLog gdb/testsuite/:
> 
> 	* gdb.cp/cp-relocate.exp (add-symbol-file): Change addresses
> 	to fit into SPU Local Store memory.

I've committed this now, thanks!

Bye,
Ulrich

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