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

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

[Ulrich Weigand]

Hello,

this fixes another problem for the SPU port.  In value_assign,
when assigning to a register that is marked as CONVERT_REGISTER_P,
the front end always calls VALUE_TO_REGISTER, which always cleans
out the full previous contents of the register.

This happens even if the value is a bitfield that occupies only
part of the register, and the remaining contents should *not* be
changed.  The back end's VALUE_TO_REGISTER has no real chance to
do this right since it doesn't even get the value object as input,
and thus doesn't know that value_bitsize is nonzero.

This probably rarely triggers on other architectures, as those
registers that require CONVERT_REGISTER_P tend to be special 
registers that usually don't hold bitfield values.  On the SPU
however, every general-purpose register needs CONVERT_REGISTER_P
(since they are really 16-byte vector registers, and loading/
storing scalar values required a special conversion).

The patch below 'fixes' this for SPU by at least not calling
VALUE_TO_REGISTER for bitfield assignments, but falling back
to the default code.  This happens to work for SPU; other 
platforms with special conversion needs might need to get a
chance for the back-end to get involved even then.  I guess
when this happens we can think of an extended interface that
would pass the bitsize information through to the back-end ...

Tested on SPU (where it fixes about a dozen test cases).
Also tested without regressions on s390-ibm-linux and
s390x-ibm-linux.

OK?


Bye,
Ulrich


ChangeLog:

	* valops.c (value_assign): Do not call VALUE_TO_REGISTER
	for bitfield assignments.


diff -urN gdb-orig/gdb/valops.c gdb-head/gdb/valops.c
--- gdb-orig/gdb/valops.c	2006-10-09 21:28:14.000000000 +0200
+++ gdb-head/gdb/valops.c	2006-10-30 19:39:02.972619008 +0100
@@ -643,6 +643,7 @@
 	  error (_("Value being assigned to is no longer active."));
 	
 	if (VALUE_LVAL (toval) == lval_register
+	    && value_bitsize (toval) == 0
 	    && CONVERT_REGISTER_P (VALUE_REGNUM (toval), type))
 	  {
 	    /* If TOVAL is a special machine register requiring
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

[PING] Re: [RFA][2/5] New port: Cell BE SPU (valops.c fix)

[Ulrich Weigand]

http://sources.redhat.com/ml/gdb-patches/2006-11/msg00083.html

> 	* valops.c (value_assign): Do not call VALUE_TO_REGISTER
> 	for bitfield assignments.


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

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

[Daniel Jacobowitz]

On Sat, Nov 11, 2006 at 07:38:17PM +0100, Ulrich Weigand wrote:
> The patch below 'fixes' this for SPU by at least not calling
> VALUE_TO_REGISTER for bitfield assignments, but falling back
> to the default code.  This happens to work for SPU; other 
> platforms with special conversion needs might need to get a
> chance for the back-end to get involved even then.  I guess
> when this happens we can think of an extended interface that
> would pass the bitsize information through to the back-end ...

I've got to admit that I don't like it :-(

VALUE_TO_REGISTER has only one caller and eight definitions (plus some
documentation).  It shouldn't be hard to update it.  What additional
information do you need?  Would passing the two values instead of one
regnum and one contents do it?

-- 
Daniel Jacobowitz
CodeSourcery

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

[Jim Blandy]

Daniel Jacobowitz <drow@false.org> writes:
> On Sat, Nov 11, 2006 at 07:38:17PM +0100, Ulrich Weigand wrote:
>> The patch below 'fixes' this for SPU by at least not calling
>> VALUE_TO_REGISTER for bitfield assignments, but falling back
>> to the default code.  This happens to work for SPU; other 
>> platforms with special conversion needs might need to get a
>> chance for the back-end to get involved even then.  I guess
>> when this happens we can think of an extended interface that
>> would pass the bitsize information through to the back-end ...
>
> I've got to admit that I don't like it :-(

Yeah --- it'll mask problems if someone else has a convertible
register with bitfields.

> VALUE_TO_REGISTER has only one caller and eight definitions (plus some
> documentation).  It shouldn't be hard to update it.  What additional
> information do you need?  Would passing the two values instead of one
> regnum and one contents do it?

Or, possibly another gdbarch method, VALUE_TO_REGISTER_BITFIELD, which
can be left unset, provoking an internal error in value_assign?

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

[Daniel Jacobowitz]

On Wed, Nov 22, 2006 at 11:25:48AM -0800, Jim Blandy wrote:
> > VALUE_TO_REGISTER has only one caller and eight definitions (plus some
> > documentation).  It shouldn't be hard to update it.  What additional
> > information do you need?  Would passing the two values instead of one
> > regnum and one contents do it?
> 
> Or, possibly another gdbarch method, VALUE_TO_REGISTER_BITFIELD, which
> can be left unset, provoking an internal error in value_assign?

I'm not even quite sure how bitfields come into it.  The register
type's a builtin_type_vec128.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Wed, Nov 22, 2006 at 11:25:48AM -0800, Jim Blandy wrote:
> > Or, possibly another gdbarch method, VALUE_TO_REGISTER_BITFIELD, which
> > can be left unset, provoking an internal error in value_assign?
> 
> I'm not even quite sure how bitfields come into it.  The register
> type's a builtin_type_vec128.

The problem occurs if there is a source-level variable of bitfield type
that currently resides in such a register, and the GDB user attempts to
assign to that variable.

I guess one problem here is that there's two different problems intermixed
here:

- if an integral type (possibly bitfield) resides in a vector register,
  which parts of the vector register can I find it at (that's the platform
  specific part)

- where within the bitfield is the particular element I want to access
  (that's the generic part)

Maybe the best solution would be for GDB common code to call 
VALUE_TO_REGISTER / REGISTER_TO_VALUE to solve the first problem,
but still solve the second problem in common code.

Otherwise every target would have to implement all the bitfield fiddling
separately -- I'm not sure this is required.

Bye,
Ulrich

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

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

[Daniel Jacobowitz]

On Wed, Nov 22, 2006 at 08:55:02PM +0100, Ulrich Weigand wrote:
> The problem occurs if there is a source-level variable of bitfield type
> that currently resides in such a register, and the GDB user attempts to
> assign to that variable.

I'm not sure how you are using "bitfield type" here; could you post an
example?  i.e. do you mean:

   int x:7;

or

   struct { int x:7; int y:25; } z;

I'm wondering if you're describing the latter case, in which case the
problem is that we're calling value_to_register on only part of the
value we want in the register.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> I'm not sure how you are using "bitfield type" here; could you post an
> example?  i.e. do you mean:
> 
>    int x:7;
> 
> or
> 
>    struct { int x:7; int y:25; } z;

The latter; sorry for being vague.  The problem manifests in store.exp's
check_field test cases:

FAIL: gdb.base/store.exp: f_1.i
FAIL: gdb.base/store.exp: f_1.j
FAIL: gdb.base/store.exp: f_1.k
FAIL: gdb.base/store.exp: F_1.i
FAIL: gdb.base/store.exp: F_1.j
FAIL: gdb.base/store.exp: F_1.k
FAIL: gdb.base/store.exp: f_2.i
FAIL: gdb.base/store.exp: f_2.j
FAIL: gdb.base/store.exp: f_2.k
FAIL: gdb.base/store.exp: F_2.i
FAIL: gdb.base/store.exp: F_2.j
FAIL: gdb.base/store.exp: F_2.k
FAIL: gdb.base/store.exp: f_3.i
FAIL: gdb.base/store.exp: f_3.j
FAIL: gdb.base/store.exp: f_3.k
FAIL: gdb.base/store.exp: F_3.i
FAIL: gdb.base/store.exp: F_3.j
FAIL: gdb.base/store.exp: F_3.k
FAIL: gdb.base/store.exp: f_4.i
FAIL: gdb.base/store.exp: f_4.j
FAIL: gdb.base/store.exp: f_4.k
FAIL: gdb.base/store.exp: F_4.i
FAIL: gdb.base/store.exp: F_4.j
FAIL: gdb.base/store.exp: F_4.k

struct f_1 {unsigned i:1;unsigned j:1;unsigned k:1; } f_1 = {1,1,1}, F_1;

struct f_1
wack_field_1 (void)
{
  register struct f_1 u = f_1;
  return u;
}

tbreak wack_field_1
Breakpoint 26 at 0xdd0: file /home/uweigand/fsf/gdb-head/gdb/testsuite/gdb.base/store.c, line 227.
(gdb) PASS: gdb.base/store.exp: tbreak wack_field_1
continue
Continuing.
wack_field_1 () at /home/uweigand/fsf/gdb-head/gdb/testsuite/gdb.base/store.c:227
227       register struct f_1 u = f_1;
(gdb) PASS: gdb.base/store.exp: continue field 1
next
229     }
(gdb) PASS: gdb.base/store.exp: next field 1
print u
$73 = {i = 1, j = 1, k = 1}
(gdb) PASS: gdb.base/store.exp: old field 1
set variable u = F_1
(gdb) PASS: gdb.base/store.exp: set variable u = F_1
print u
$74 = {i = 0, j = 0, k = 0}
(gdb) PASS: gdb.base/store.exp: new field 1
set variable u = F_1, u.i = f_1.i
(gdb) PASS: gdb.base/store.exp: set variable u = F_1, u.i = f_1.i
print u
$75 = {i = 0, j = 0, k = 0}
(gdb) FAIL: gdb.base/store.exp: f_1.i


> I'm wondering if you're describing the latter case, in which case the
> problem is that we're calling value_to_register on only part of the
> value we want in the register.

Yes, that's what I was thinking.

Bye,
Ulrich

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

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

[Daniel Jacobowitz]

On Wed, Nov 22, 2006 at 09:43:06PM +0100, Ulrich Weigand wrote:
> > I'm wondering if you're describing the latter case, in which case the
> > problem is that we're calling value_to_register on only part of the
> > value we want in the register.
> 
> Yes, that's what I was thinking.

OK, that sounds like we need something different here.  I'm not sure
quite what.  We need to somehow reconstruct the entire value, but we
haven't got a handy parent pointer we can use to get at it.  We could
reread it, I suppose.

Does the same thing happen for struct { char w, x, y, z; }?

-- 
Daniel Jacobowitz
CodeSourcery

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

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> Does the same thing happen for struct { char w, x, y, z; }?

For some reason I seem unable to convince GCC to place such
a struct into a register.

However, when attempting to manipulate a register directly
using its type structure, I'm seeing the same problem:

(gdb) print $r3
$1 = {uint128 = 0x00000001000000000000000000000000, v4_float = {1.40129846e-45, 0, 0, 0}, v4_int32 = {1, 0, 0, 0},
  v8_int16 = {0, 1, 0, 0, 0, 0, 0, 0}, v16_int8 = "\000\000\000\001", '\0' <repeats 11 times>}
(gdb) print $r3.v4_int32
$2 = {1, 0, 0, 0}
(gdb) print $r3.v4_int32[2]
$3 = 0
(gdb) set variable $r3.v4_int32[2] = 5
(gdb) print $r3.v4_int32
$4 = {5, 0, 0, 0}


For registers without special conversion function, value_assign
performs a read-modify-write cycle: it reads the old contents 
of the register(s), modify the bits denoted by value_offset,
value_bitsize and value_bitpos, and writes the full register
contents back.

Maybe we need to do a similar cycle of REGISTER_TO_VALUE,
modify selected bits, VALUE_TO_REGISTER for the registers with
special conversion function?

As an aside, what is this code in value_assign supposed to do:

            /* Locate the first register that falls in the value that
               needs to be transfered.  Compute the offset of the
               value in that register.  */
            {
              int offset;
              for (reg_offset = value_reg, offset = 0;
                   offset + register_size (current_gdbarch, reg_offset) <= value_offset (toval);
                   reg_offset++);
              byte_offset = value_offset (toval) - offset;
            }

It seems clearly broken (offset remains constant 0), but I'm not
quite sure what the intent was.

Bye,
Ulrich

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

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

[Daniel Jacobowitz]

On Wed, Nov 22, 2006 at 11:13:09PM +0100, Ulrich Weigand wrote:
> For some reason I seem unable to convince GCC to place such
> a struct into a register.

Must be a mode thing... shame; it ought to be able to do this.

> For registers without special conversion function, value_assign
> performs a read-modify-write cycle: it reads the old contents 
> of the register(s), modify the bits denoted by value_offset,
> value_bitsize and value_bitpos, and writes the full register
> contents back.
> 
> Maybe we need to do a similar cycle of REGISTER_TO_VALUE,
> modify selected bits, VALUE_TO_REGISTER for the registers with
> special conversion function?

I think that's precisely it.  I'm worried that this will break some
other uses of VALUE_TO_REGISTER, though - how do we keep track at
this point in the code of "the whole struct is in the register"
or "this field is in the register"?  Right now, there's limited
flexibility on that, but Jim's been working on improved DW_OP_piece
support which would let us write to structure pieces.

> As an aside, what is this code in value_assign supposed to do:
> 
>             /* Locate the first register that falls in the value that
>                needs to be transfered.  Compute the offset of the
>                value in that register.  */
>             {
>               int offset;
>               for (reg_offset = value_reg, offset = 0;
>                    offset + register_size (current_gdbarch, reg_offset) <= value_offset (toval);
>                    reg_offset++);
>               byte_offset = value_offset (toval) - offset;
>             }
> 
> It seems clearly broken (offset remains constant 0), but I'm not
> quite sure what the intent was.

Um, yeah, that's definitely not sensible.  Perhaps it is supposed to
match the loop further down, in which offset would go up by
register_size?

-- 
Daniel Jacobowitz
CodeSourcery

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

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> I think that's precisely it.  I'm worried that this will break some
> other uses of VALUE_TO_REGISTER, though - how do we keep track at
> this point in the code of "the whole struct is in the register"
> or "this field is in the register"?  Right now, there's limited
> flexibility on that, but Jim's been working on improved DW_OP_piece
> support which would let us write to structure pieces.

Hmm, it looks like the enclosing type information is simply lost
currently.  Those values come from value_subscripted_rvalue
(which appears to return an lvalue despite its name?), which 
simply forgets about the base array type, and only updates
value_offset.

What value_assign would need is the information: I want to update
an object of type "int", which resides at offset 12 within an
object of type "array of four ints" which resides in register 42.
Then, we could call REGISTER_TO_VALUE on the enclosing type,
and do the read-update-write cycle.

However, we don't have the enclosing type in the data structure
at all, which makes it difficult.  I'm wondering whether we can
employ the value_enclosing_type (and related) fields for this
purpose, but I'm not completely sure how that would interact
with the C++ use of those.

Bye,
Ulrich

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

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

[Daniel Jacobowitz]

On Thu, Nov 23, 2006 at 02:57:19PM +0100, Ulrich Weigand wrote:
> However, we don't have the enclosing type in the data structure
> at all, which makes it difficult.  I'm wondering whether we can
> employ the value_enclosing_type (and related) fields for this
> purpose, but I'm not completely sure how that would interact
> with the C++ use of those.

Very badly, I suspect: parts of GDB would probably start assuming that
"int" was a base class of "array of four ints", which isn't quite
right.

I suppose there's times we want to destroy the rest of the register,
so knowing where it is in the register isn't enough?

-- 
Daniel Jacobowitz
CodeSourcery

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

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> I suppose there's times we want to destroy the rest of the register,
> so knowing where it is in the register isn't enough?

The problem is, we don't *know* where it is in the register.

For example, on the SPU "char" values are placed in byte 3 of
the 16 bytes of a general purpose register, "short" values are
placed in bytes 2 and 3, and "int" values are placed in bytes
0 .. 3.  ("long long" is placed in 0 .. 7.)

However, structs are placed into registers starting from 
byte 0 always.

So if we have

  struct { char x; char y; char z; char w; } s;
  char t;

and both s and t reside in registers, then a value to access
t would look exactly the same as a value to access s.x (i.e.
type "char", lval_regnum, value_offset == 0), but to access
them requires using different bytes of the register.

We might be able to fix this particular problem by having
value_from_register somehow set the value_offset to 3 when
retrieving a value of type "char" from a register.  However,
even though there is a comment saying "The assumption is that
REGISTER_TO_VALUE populates the entire value including the
location.", that isn't actually possible with the current
interface since REGISTER_TO_VALUE doesn't actually *get*
the value itself.

So maybe we can extend REGISTER_TO_VALUE by an argument to
return the byte offset of the original value in the register,
and extend VALUE_TO_REGISTER by an argument to pass the 
requested byte offset (which might point to a subobject of
the whole object), this could work ...

We'd still have to solve the bitfield problem, but that might
be possible in common code later on.

Bye,
Ulrich

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

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

[Mark Kettenis]

>  Daniel Jacobowitz wrote:
>
> > I suppose there's times we want to destroy the rest of the register,
> > so knowing where it is in the register isn't enough?
>
>  The problem is, we don't *know* where it is in the register.
>
>  For example, on the SPU "char" values are placed in byte 3 of
>  the 16 bytes of a general purpose register, "short" values are
>  placed in bytes 2 and 3, and "int" values are placed in bytes
>  0 .. 3.  ("long long" is placed in 0 .. 7.)
>
>  However, structs are placed into registers starting from
>  byte 0 always.

Which is the same way as structs are stored in memory isn't it?

>  So if we have
>
>    struct { char x; char y; char z; char w; } s;
>    char t;
>
>  and both s and t reside in registers, then a value to access
>  t would look exactly the same as a value to access s.x (i.e.
>  type "char", lval_regnum, value_offset == 0), but to access
>  them requires using different bytes of the register.

I actually think the problem is that you're thinking that s.x lives in
a register where it is actually s itself that lives in that register.
So VALUE_TO_REGISTER should be called for the struct itself, not its
char member.

Mark

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

[Daniel Jacobowitz]

On Thu, Nov 23, 2006 at 08:59:08PM +0100, Mark Kettenis wrote:
> I actually think the problem is that you're thinking that s.x lives in
> a register where it is actually s itself that lives in that register.
> So VALUE_TO_REGISTER should be called for the struct itself, not its
> char member.

This is at least the second time recently I've seen someone want to go
from value to parent value... I'm a little worried about consistency
between the two (e.g. do you update value_contents of one?  both?) but
it may be that we need to link them to do this right.  Or, just not use
lval_register for struct members / array subscripting.  Which may be
the same thing in the end?

-- 
Daniel Jacobowitz
CodeSourcery

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

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Thu, Nov 23, 2006 at 08:59:08PM +0100, Mark Kettenis wrote:
> > I actually think the problem is that you're thinking that s.x lives in
> > a register where it is actually s itself that lives in that register.
> > So VALUE_TO_REGISTER should be called for the struct itself, not its
> > char member.
> 
> This is at least the second time recently I've seen someone want to go
> from value to parent value... I'm a little worried about consistency
> between the two (e.g. do you update value_contents of one?  both?) but
> it may be that we need to link them to do this right.  Or, just not use
> lval_register for struct members / array subscripting.  Which may be
> the same thing in the end?


The following patch attemps to solve this problem by adding a new field
VALUE_REGTYPE to the value structure.  It is non-NULL if and only if the
value is an lvalue refering to a (subfield of a) register that uses
specical conversion functions.

VALUE_REGTYPE is set in value_from_register if CONVERT_REGISTER_P
return true; it is set to the type used for this access.  It is copied
across value_copy, value_primitive_field, and value_subscripted_rvalue;
note that always the original type remains unchanged.

This allows value_assign to use that original type of the full
register contents to perform a read/modify/write cycle on a register
that needs special conversion functions.

The patch fixes both the bitfield test cases, and the explicit access
to a register using its builtin_type_vec128 type.  Tested with no
regressions on spu-elf.

Is this a reasonable approach?

Bye,
Ulrich


ChangeLog:

	* value.h (VALUE_REGTYPE): New macro.
	(deprecated_value_regtype_hack): Add prototype.
	* value.c (struct value): New field regtype.
	(deprecated_value_regtype_hack): New function.
	(value_copy): Copy VALUE_REGTYPE.
	(value_primitive_field): Likewise.
	* valarith.c (value_subscripted_rvalue): Copy VALUE_REGTYPE.
	* findvar.c (value_from_register): Set VALUE_REGTYPE for values
	from register with special conversion functions.
	* valops.c (value_assign): When assigning to a register with
	special conversion functions, do a read/modify/write cycle
	using type VALUE_REGTYPE.

Index: gdb/findvar.c
===================================================================
RCS file: /cvs/src/src/gdb/findvar.c,v
retrieving revision 1.96
diff -u -p -r1.96 findvar.c
--- gdb/findvar.c	22 Nov 2006 13:44:45 -0000	1.96
+++ gdb/findvar.c	24 Nov 2006 14:25:51 -0000
@@ -648,6 +648,7 @@ value_from_register (struct type *type, 
       VALUE_LVAL (v) = lval_register;
       VALUE_FRAME_ID (v) = get_frame_id (frame);
       VALUE_REGNUM (v) = regnum;
+      VALUE_REGTYPE (v) = type;
     }
   else
     {
Index: gdb/valarith.c
===================================================================
RCS file: /cvs/src/src/gdb/valarith.c,v
retrieving revision 1.45
diff -u -p -r1.45 valarith.c
--- gdb/valarith.c	24 Jan 2006 21:21:12 -0000	1.45
+++ gdb/valarith.c	24 Nov 2006 14:25:53 -0000
@@ -280,6 +280,7 @@ value_subscripted_rvalue (struct value *
     VALUE_LVAL (v) = VALUE_LVAL (array);
   VALUE_ADDRESS (v) = VALUE_ADDRESS (array);
   VALUE_REGNUM (v) = VALUE_REGNUM (array);
+  VALUE_REGTYPE (v) = VALUE_REGTYPE (array);
   VALUE_FRAME_ID (v) = VALUE_FRAME_ID (array);
   set_value_offset (v, value_offset (array) + elt_offs);
   return v;
Index: gdb/valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.165
diff -u -p -r1.165 valops.c
--- gdb/valops.c	9 Oct 2006 19:28:14 -0000	1.165
+++ gdb/valops.c	24 Nov 2006 14:25:53 -0000
@@ -643,12 +643,31 @@ value_assign (struct value *toval, struc
 	  error (_("Value being assigned to is no longer active."));
 	
 	if (VALUE_LVAL (toval) == lval_register
-	    && CONVERT_REGISTER_P (VALUE_REGNUM (toval), type))
+	    && VALUE_REGTYPE (toval) != NULL
+	    && CONVERT_REGISTER_P (value_reg, VALUE_REGTYPE (toval)))
 	  {
-	    /* If TOVAL is a special machine register requiring
+	    /* TOVAL is a special machine register requiring
 	       conversion of program values to a special raw format.  */
-	    VALUE_TO_REGISTER (frame, VALUE_REGNUM (toval),
-			       type, value_contents (fromval));
+
+	    /* Determine type of full register contents.  Note that TOVAL
+	       may refer to a subfield of the full register.  */
+	    struct type *regtype = VALUE_REGTYPE (toval);
+	    gdb_byte *buffer = alloca (TYPE_LENGTH (regtype));
+
+	    /* Copy in old register contents.  */
+	    REGISTER_TO_VALUE (frame, value_reg, regtype, buffer);
+
+	    /* Modify what needs to be modified.  */
+	    if (value_bitsize (toval))
+	      modify_field (buffer + value_offset (toval),
+			    value_as_long (fromval),
+			    value_bitpos (toval), value_bitsize (toval));
+	    else
+	      memcpy (buffer + value_offset (toval),
+		      value_contents (fromval), TYPE_LENGTH (type));
+
+	    /* Write back the new register contents.  */
+	    VALUE_TO_REGISTER (frame, value_reg, regtype, buffer);
 	  }
 	else
 	  {
Index: gdb/value.c
===================================================================
RCS file: /cvs/src/src/gdb/value.c,v
retrieving revision 1.36
diff -u -p -r1.36 value.c
--- gdb/value.c	31 Mar 2006 10:36:18 -0000	1.36
+++ gdb/value.c	24 Nov 2006 14:25:53 -0000
@@ -135,6 +135,9 @@ struct value
      list.  */
   struct value *next;
 
+  /* Full type of register contents if the value is from a register.  */
+  struct type *regtype;
+
   /* Register number if the value is from a register.  */
   short regnum;
 
@@ -444,6 +447,12 @@ deprecated_value_regnum_hack (struct val
   return &value->regnum;
 }
 
+struct type **
+deprecated_value_regtype_hack (struct value *value)
+{
+  return &value->regtype;
+}
+
 int
 deprecated_value_modifiable (struct value *value)
 {
@@ -557,6 +566,7 @@ value_copy (struct value *arg)
   val->bitsize = arg->bitsize;
   VALUE_FRAME_ID (val) = VALUE_FRAME_ID (arg);
   VALUE_REGNUM (val) = VALUE_REGNUM (arg);
+  VALUE_REGTYPE (val) = VALUE_REGTYPE (arg);
   val->lazy = arg->lazy;
   val->optimized_out = arg->optimized_out;
   val->embedded_offset = value_embedded_offset (arg);
@@ -1347,6 +1357,7 @@ value_primitive_field (struct value *arg
     VALUE_LVAL (v) = lval_internalvar_component;
   VALUE_ADDRESS (v) = VALUE_ADDRESS (arg1);
   VALUE_REGNUM (v) = VALUE_REGNUM (arg1);
+  VALUE_REGTYPE (v) = VALUE_REGTYPE (arg1);
   VALUE_FRAME_ID (v) = VALUE_FRAME_ID (arg1);
 /*  VALUE_OFFSET (v) = VALUE_OFFSET (arg1) + offset
    + TYPE_FIELD_BITPOS (arg_type, fieldno) / 8; */
Index: gdb/value.h
===================================================================
RCS file: /cvs/src/src/gdb/value.h,v
retrieving revision 1.93
diff -u -p -r1.93 value.h
--- gdb/value.h	22 Nov 2006 13:44:45 -0000	1.93
+++ gdb/value.h	24 Nov 2006 14:25:53 -0000
@@ -220,6 +220,10 @@ extern struct frame_id *deprecated_value
 extern short *deprecated_value_regnum_hack (struct value *);
 #define VALUE_REGNUM(val) (*deprecated_value_regnum_hack (val))
 
+/* Full type of register contents if the value is from a register.  */
+extern struct type **deprecated_value_regtype_hack (struct value *);
+#define VALUE_REGTYPE(val) (*deprecated_value_regtype_hack (val))
+
 /* Convert a REF to the object referenced.  */
 
 extern struct value *coerce_ref (struct value *value);

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

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

[Daniel Jacobowitz]

On Fri, Nov 24, 2006 at 04:51:01PM +0100, Ulrich Weigand wrote:
> The following patch attemps to solve this problem by adding a new field
> VALUE_REGTYPE to the value structure.  It is non-NULL if and only if the
> value is an lvalue refering to a (subfield of a) register that uses
> specical conversion functions.

For the record, I haven't really looked at this patch, since we're
still discussing what these macros should really do.  If the discussion
trails off and this is the best way found to fix the bug, please ping
the patch.  Thanks.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Jim Blandy]

"Ulrich Weigand" <uweigand@de.ibm.com> writes:
> We might be able to fix this particular problem by having
> value_from_register somehow set the value_offset to 3 when
> retrieving a value of type "char" from a register.  However,
> even though there is a comment saying "The assumption is that
> REGISTER_TO_VALUE populates the entire value including the
> location.", that isn't actually possible with the current
> interface since REGISTER_TO_VALUE doesn't actually *get*
> the value itself.

It seems to me this is the problem to fix.  When value_from_register
retrieves a char from an SPU register, and that char is occupying byte
three of the register, then if that value doesn't have its
value_offset set, that seems wrong.  You're using CONVERTIBLE_P and
VALUE_TO_REGISTER / REGISTER_TO_VALUE to make up for that loss of
information; why not actually provide it?

I'd prefer that to adding a new field to struct value.

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

[Ulrich Weigand]

Jim Blandy wrote:

> It seems to me this is the problem to fix.  When value_from_register
> retrieves a char from an SPU register, and that char is occupying byte
> three of the register, then if that value doesn't have its
> value_offset set, that seems wrong.  You're using CONVERTIBLE_P and
> VALUE_TO_REGISTER / REGISTER_TO_VALUE to make up for that loss of
> information; why not actually provide it?

So just to make sure I understood correctly, you'd suggesting that
I should *not* be using CONVERT_REGISTER_P for those registers?

Instead, value_from_register should run into its default path,
and at the place where it computes the offset

      if (TARGET_BYTE_ORDER == BFD_ENDIAN_BIG
          && len < register_size (current_gdbarch, regnum))
        /* Big-endian, and we want less than full size.  */
        set_value_offset (v, register_size (current_gdbarch, regnum) - len);
      else
        set_value_offset (v, 0);

we add some architecture-specific way to set a different offset?

Bye,
Ulrich

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

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

[Jim Blandy]

"Ulrich Weigand" <uweigand@de.ibm.com> writes:
>> It seems to me this is the problem to fix.  When value_from_register
>> retrieves a char from an SPU register, and that char is occupying byte
>> three of the register, then if that value doesn't have its
>> value_offset set, that seems wrong.  You're using CONVERTIBLE_P and
>> VALUE_TO_REGISTER / REGISTER_TO_VALUE to make up for that loss of
>> information; why not actually provide it?
>
> So just to make sure I understood correctly, you'd suggesting that
> I should *not* be using CONVERT_REGISTER_P for those registers?
>
> Instead, value_from_register should run into its default path,
> and at the place where it computes the offset
>
>       if (TARGET_BYTE_ORDER == BFD_ENDIAN_BIG
>           && len < register_size (current_gdbarch, regnum))
>         /* Big-endian, and we want less than full size.  */
>         set_value_offset (v, register_size (current_gdbarch, regnum) - len);
>       else
>         set_value_offset (v, 0);
>
> we add some architecture-specific way to set a different offset?

I had to think it through a bit, but yes, I think that's the way to do
it.  Then, won't the non-convertible register code in value_assign do
the right read-modify-write thing without changes?

My motivation is that it seems to me that 'struct value' already has
stuff meant to handle these kinds of subregister references, but we're
not using it.  If we do use it, then value_struct_elt and
value_subscript will do the right thing for us.

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

[Ulrich Weigand]

Jim Blandy wrote:
> "Ulrich Weigand" <uweigand@de.ibm.com> writes:
> > So just to make sure I understood correctly, you'd suggesting that
> > I should *not* be using CONVERT_REGISTER_P for those registers?
> >
> > Instead, value_from_register should run into its default path,
> > and at the place where it computes the offset
> >
> >       if (TARGET_BYTE_ORDER == BFD_ENDIAN_BIG
> >           && len < register_size (current_gdbarch, regnum))
> >         /* Big-endian, and we want less than full size.  */
> >         set_value_offset (v, register_size (current_gdbarch, regnum) - len);
> >       else
> >         set_value_offset (v, 0);
> >
> > we add some architecture-specific way to set a different offset?
> 
> I had to think it through a bit, but yes, I think that's the way to do
> it.  Then, won't the non-convertible register code in value_assign do
> the right read-modify-write thing without changes?

Yes, that would probably work for SPU. 

> My motivation is that it seems to me that 'struct value' already has
> stuff meant to handle these kinds of subregister references, but we're
> not using it.  If we do use it, then value_struct_elt and
> value_subscript will do the right thing for us.

However, I still think there's something fundamentally broken in the
way value_assign calls VALUE_TO_REGISTER.   As the documentation says,
that gdbarch functions is supposed to "convert a data value of type
TYPE to register number REG's raw format".  Calling the conversion
function with a type that does not actually denote the type of the
register contents, but some subfield, must break all other implementations
of that routine as well ...

Bye,
Ulrich

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

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

[Jim Blandy]

"Ulrich Weigand" <uweigand@de.ibm.com> writes:
> However, I still think there's something fundamentally broken in the
> way value_assign calls VALUE_TO_REGISTER.   As the documentation says,
> that gdbarch functions is supposed to "convert a data value of type
> TYPE to register number REG's raw format".  Calling the conversion
> function with a type that does not actually denote the type of the
> register contents, but some subfield, must break all other implementations
> of that routine as well ...

You're clearly right...  Hmm.

What's important here is that values' offset and bitfield information
be correct, so that value_subscript and value_struct_elt actually
work.  Which macros originate and consume that information isn't so
important.

If REGISTER_TO_VALUE returned a fresh struct value, and
VALUE_TO_REGISTER accepted a struct value, then they'd become fully
general register access hooks.  This is what Daniel suggested at the
top of the thread, but with the difference that REGISTER_TO_VALUE
would be responsible for setting the value's offset and bitfield info,
which avoids the need for knowing about a parent pointer.

I'd worry a bit that we'd get arch-specific reimplementations of the
now-generic gathering and scattering code in value_from_register and
value_assign.  At the moment, you have to choose between using only
the generic code, or converting but not handling assignments to
subfields; making R2V and V2R operate on struct values would give you
a choice between using only the generic code, or doing all the work
yourself.

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

[Daniel Jacobowitz]

On Tue, Nov 28, 2006 at 12:23:34AM +0100, Ulrich Weigand wrote:
> However, I still think there's something fundamentally broken in the
> way value_assign calls VALUE_TO_REGISTER.   As the documentation says,
> that gdbarch functions is supposed to "convert a data value of type
> TYPE to register number REG's raw format".  Calling the conversion
> function with a type that does not actually denote the type of the
> register contents, but some subfield, must break all other implementations
> of that routine as well ...

Except that I suspect there are no other implementations of
VALUE_TO_REGISTER which offer any subfields.  It was usually used for
things like registers which can only hold a float by converting it to
double, and those floating point registers generally can't hold
arbitrary structs or arrays.

There might be some trouble on i386 though... I think you'd need
extraordinary bad luck to trigger it.  A field of a struct spread
across multiple registers.

These three hooks are simply not very clearly defined.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> On Tue, Nov 28, 2006 at 12:23:34AM +0100, Ulrich Weigand wrote:
> > However, I still think there's something fundamentally broken in the
> > way value_assign calls VALUE_TO_REGISTER.   As the documentation says,
> > that gdbarch functions is supposed to "convert a data value of type
> > TYPE to register number REG's raw format".  Calling the conversion
> > function with a type that does not actually denote the type of the
> > register contents, but some subfield, must break all other implementations
> > of that routine as well ...
> 
> Except that I suspect there are no other implementations of
> VALUE_TO_REGISTER which offer any subfields.  It was usually used for
> things like registers which can only hold a float by converting it to
> double, and those floating point registers generally can't hold
> arbitrary structs or arrays.
> 
> There might be some trouble on i386 though... I think you'd need
> extraordinary bad luck to trigger it.  A field of a struct spread
> across multiple registers.
> 
> These three hooks are simply not very clearly defined.

Here's an overview of what register_to_value / value_to_register
are currently being used for on various platforms:

   arch       float reg    float values     type of
                only          only         conversion
   alpha          Y             N            special
   i387 (*)       Y             Y            float
   i386           N             N            multi-reg
   ia64           Y             Y            float
   m68k           Y             Y            float
   mips           Y             Y            multi-reg
   rs6000         Y             Y            float
   s390           Y             N            offset
   spu            N             N            offset

   (*) on amd64 and i386

The default code extracts a value from a register using a certain set
of assumptions: the bits making up the value are the same for values
in registers as in memory, and there is well-defined way to *find* 
which bits of the register(s) make up that value.

Depending on which of those assumptions is violated, different 
platforms use r_t_v / v_t_r for different purposes:

- On some platforms, the bits in the register are the same as in 
  memory, they're just not at the default location:

  * For multi-register values, the value is not formed by registers
    concatenated like X, X+1, X+2, ...
      -> This is the case for 8-byte float values on mips, and for
         certain multi-register values on i386.  ("multi-reg")

  * For single-register values of size smaller than register size,
    the bits are not at the default offset
      -> This is the case for 4-byte values in s390 float registers,
         and for all values < 16 bytes on spu.  ("offset")

- On some platforms, the bits making up the value are themselves 
  different, i.e. a real conversion needs to take place:

  * Floating point values may need to be converted into a different
    floating point number format
     -> E.g. on i387, ia64, m68k, rs6000 ("float")

  * There's a particular special case on the alpha, where 4-byte
    integer values may reside in floating point registers, but still
    undergo a conversion as if to a different floating point format
    (however, that conversion is fully reversible, so when the value
    is stored back to memory, we still get the same integer).


Independently, we have the question of how to handle sub-types of
values in registers.  This may happen if the value residing in the
register is of aggregate type (struct / bitfield / array).  The
compiler is free to allocate such values into registers, and may
generally do so if the size of the aggregate equals the size of
an integral type that would otherwise be allowed to reside in the
register in question.

If this happens to a register under r_t_v / v_t_r conversion, the
current implementation is broken, since v_t_r may be called with
a different base type than r_t_v.

This is not an issue for those platforms that only ever accept
floating-point types in their r_t_v / v_t_r routines, since those
do not have sub-types.  However, it remains a problem, at least
in principle, for alpha, i386, s390, and spu.

Jim's suggestion was to allow the r_t_v / v_t_r routines to set
and access the value's offset and bitfield information.  This 
would enable both s390 and spu to encode the non-standard offset
in r_t_v, and otherwise use the default methods.

However, for i386 and alpha this still would not work, since
there's simply not enough information available.  Let's assume
we have an value of aggregate 8-byte type composed of two 4-byte
subfields of type "int".  Such a value could reside in a float
register on alpha, or the %eax/%edx register pair on i386.

Now, when accessing one sub-field of this value via type "int",
the target routine needs enough information to distinguish it
from the case where you simply have a plain "int" in a register.

You could attempt to recognize that using the offset field.
And in fact, on i386 it may happen to work after all, because
the platform is little-endian and there is no difference between
accessing the element at offset 0 in the pair %eax/%edx, and
just plain accessing an "int" in %eax.  It would definitely
break on a platform that swaps multi-register values (like mips
floating point registers), and still allows types with sub-fields.

On alpha, it doesn't work at all; accessing the value at offset
0 in the 8-byte struct needs completely different code than
accessing a simple "int" in the same register, and there is no
way how v_t_r can distinguish the two cases.


The patch I proposed to remember the type of the value residing
in the register would allow all the above cases to work without
changes to architecture code, and would in fact work correctly
with any conceivable implementation of the current r_t_v / v_t_r
interface.  However, it does have the disadvantage of requiring
an additional field in struct value.  (Maybe we can make up that
loss -- do we actually still require VALUE_ADDRESS for register
values?)

Any suggestions?


Bye,
Ulrich



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

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

[Daniel Jacobowitz]

On Wed, Dec 06, 2006 at 05:29:27PM +0100, Ulrich Weigand wrote:
> The patch I proposed to remember the type of the value residing
> in the register would allow all the above cases to work without
> changes to architecture code, and would in fact work correctly
> with any conceivable implementation of the current r_t_v / v_t_r
> interface.  However, it does have the disadvantage of requiring
> an additional field in struct value.  (Maybe we can make up that
> loss -- do we actually still require VALUE_ADDRESS for register
> values?)
> 
> Any suggestions?

Vladimir has actually been working on a similar change for a different
purpose.  He added a "parent value" pointer to values; bitfields then
are accessed by reading the enclosing structure and extracting bits
from value_contents.

I've been kind of waffling on Vladimir's patch because it has a nasty
bug that I just can't find any way to fix.  Adding pointers between
values messes up release_value / value_free; either we leak values or
we access uninitialized memory, or both.  As Vladimir has pointed out
several times, what we really need is a shared_ptr :-)

What do you think?  Would this solve the same problem as your patch?
Any bright ideas on the memory management?  We could always go whole
hog and add a refcount... I realize now that if we only need to
reference count one reference for whoever called release_value (or
being on the value chain) and one per child field, it wouldn't
be too hard.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> Vladimir has actually been working on a similar change for a different
> purpose.  He added a "parent value" pointer to values; bitfields then
> are accessed by reading the enclosing structure and extracting bits
> from value_contents.

Can you point me to the patch?  I didn't find it on the gdb-patches list ...

> What do you think?  Would this solve the same problem as your patch?

Depending on the circumstances when a "child" value is generated, it
may solve the same problem.  To solve the register value problem,
we would need to make sure that accessing a component of a value in
a register would create a "child" value, and v_t_r is always only 
ever called on the (grand-)parent that corresponds to the value
originally retrieved by r_t_v.

> Any bright ideas on the memory management?  We could always go whole
> hog and add a refcount... I realize now that if we only need to
> reference count one reference for whoever called release_value (or
> being on the value chain) and one per child field, it wouldn't
> be too hard.

I guess refcounts should be fine ...

Bye,
Ulrich

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

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

[Daniel Jacobowitz]

On Wed, Dec 06, 2006 at 06:10:04PM +0100, Ulrich Weigand wrote:
> Daniel Jacobowitz wrote:
> 
> > Vladimir has actually been working on a similar change for a different
> > purpose.  He added a "parent value" pointer to values; bitfields then
> > are accessed by reading the enclosing structure and extracting bits
> > from value_contents.
> 
> Can you point me to the patch?  I didn't find it on the gdb-patches list ...

It hasn't been posted yet - it's not quite done.

> > What do you think?  Would this solve the same problem as your patch?
> 
> Depending on the circumstances when a "child" value is generated, it
> may solve the same problem.  To solve the register value problem,
> we would need to make sure that accessing a component of a value in
> a register would create a "child" value, and v_t_r is always only 
> ever called on the (grand-)parent that corresponds to the value
> originally retrieved by r_t_v.

Right.  The current version only did this for bitfields in memory,
rather than fields in registers, but it shouldn't be hard to extend.
Does that make sense, Vlad?

-- 
Daniel Jacobowitz
CodeSourcery

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

[Vladimir Prus]

Daniel Jacobowitz wrote:

> On Wed, Dec 06, 2006 at 06:10:04PM +0100, Ulrich Weigand wrote:
>> Daniel Jacobowitz wrote:
>> 
>> > Vladimir has actually been working on a similar change for a different
>> > purpose.  He added a "parent value" pointer to values; bitfields then
>> > are accessed by reading the enclosing structure and extracting bits
>> > from value_contents.
>> 
>> Can you point me to the patch?  I didn't find it on the gdb-patches list
>> ...
> 
> It hasn't been posted yet - it's not quite done.
> 
>> > What do you think?  Would this solve the same problem as your patch?
>> 
>> Depending on the circumstances when a "child" value is generated, it
>> may solve the same problem.  To solve the register value problem,
>> we would need to make sure that accessing a component of a value in
>> a register would create a "child" value, and v_t_r is always only
>> ever called on the (grand-)parent that corresponds to the value
>> originally retrieved by r_t_v.
> 
> Right.  The current version only did this for bitfields in memory,
> rather than fields in registers, but it shouldn't be hard to extend.
> Does that make sense, Vlad?

It does. We'd only have to use value_to_register as opposed to write_memory
when sending the value back. Plus a bunch of unexpected problems but nothing
impossible.

- Volodya

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

[Jim Blandy]

That analysis of the current use of the r2v/v2r functions is really
helpful.  I feel very happy to have a thorough and detailed
explanation of what's going on.  Thank you, Ulrich.

It seems to me the hard case to deal with is the alpha STS/LDS
conversions.  If I understand properly, the alpha's issue here is as
follows:

- a 32-bit float stored in a floating-point register with LDS gets
  converted to a 64-bit floating-point value.  Extracting it with STS
  does the reverse, but in a way that preserves the exact bit pattern.
  So the LDS and STS instructions can be used to store 32-bit integers
  as well as 32-bit floats, although the integer's bits get
  re-interpreted as float fields and the value looks odd.  And this is
  the standard way to handle 32-bit integers stored in floating-point
  registers.

- a 64-bit structure can also be stored in a floating-point register,
  with no bit rearrangement.

So the alpha needs to distinguish three cases:

a) a 32-bit integer in a floating-point register, mangled as a float.

b) an integer of some size at some offset within the floating-point
  register, not mangled because it is part of a larger structure.

c) a 64-bit value stored in the register.

The alpha register_to_value function could represent a) as a value
with a bitpos of 64, indicating that special decoding is necessary.
It would represent b) and c) using normal bit positions and lengths.
Wouldn't this work?  Couldn't something similar be done for the 387?

I'm reluctant to get into storing original types and having reference
counts; it's a lot of complexity in the core code to handle
architectures that are doing odd things.

I've got unsubmitted patches for GDB that implement a new kind of
value, whose contents are read and written via functions provided by
the user, based on a generic closure pointer.  Future r2v / v2r
functions could produce values of this sort, instead of using odd
bitpos values.  So the kludge wouldn't last forever.

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

[Daniel Jacobowitz]

On Wed, Dec 06, 2006 at 01:21:48PM -0800, Jim Blandy wrote:
> I'm reluctant to get into storing original types and having reference
> counts; it's a lot of complexity in the core code to handle
> architectures that are doing odd things.

What about the fact that Vladimir wanted it for memory bitfields too?
Nothing architecture-specific about that.

> I've got unsubmitted patches for GDB that implement a new kind of
> value, whose contents are read and written via functions provided by
> the user, based on a generic closure pointer.  Future r2v / v2r
> functions could produce values of this sort, instead of using odd
> bitpos values.  So the kludge wouldn't last forever.

Then let's not add the kludge at all.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Jim Blandy]

Daniel Jacobowitz <drow@false.org> writes:
> On Wed, Dec 06, 2006 at 01:21:48PM -0800, Jim Blandy wrote:
>> I'm reluctant to get into storing original types and having reference
>> counts; it's a lot of complexity in the core code to handle
>> architectures that are doing odd things.
>
> What about the fact that Vladimir wanted it for memory bitfields too?
> Nothing architecture-specific about that.

I'm not sure I think it's a great idea there, either.  I want to write
up an explanation of why (essentially, because you're just replacing
the problem of too many reads with the problem of too few reads), but
I'm crunched at the moment, and haven't had time to make sure I
actually understand what everyone has written.

>> I've got unsubmitted patches for GDB that implement a new kind of
>> value, whose contents are read and written via functions provided by
>> the user, based on a generic closure pointer.  Future r2v / v2r
>> functions could produce values of this sort, instead of using odd
>> bitpos values.  So the kludge wouldn't last forever.
>
> Then let's not add the kludge at all.

If the alternative you have in mind is, let's give r2v the ability to
construct any kind of value it wants, and v2r access to the whole
value, thus solving Ulrich's problem, and then get computed lvalues in
to solve the Alpha's problem, then I'm in agreement.

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

[Ulrich Weigand]

Jim Blandy wrote:

> So the alpha needs to distinguish three cases:
> 
> a) a 32-bit integer in a floating-point register, mangled as a float.
> 
> b) an integer of some size at some offset within the floating-point
>   register, not mangled because it is part of a larger structure.
> 
> c) a 64-bit value stored in the register.
> 
> The alpha register_to_value function could represent a) as a value
> with a bitpos of 64, indicating that special decoding is necessary.
> It would represent b) and c) using normal bit positions and lengths.
> Wouldn't this work?  Couldn't something similar be done for the 387?

This misses the case

a') an integer (bitfield) of some size < 32 bit that is part of a
    32-bit integer in a floating-point register mangled as a float


> I'm reluctant to get into storing original types and having reference
> counts; it's a lot of complexity in the core code to handle
> architectures that are doing odd things.

Note that my original proposal did not require refcounts, since it 
did not store pointers to struct value, only to types.  (And value
already contains pointers to types, so this is nothing new.)

> I've got unsubmitted patches for GDB that implement a new kind of
> value, whose contents are read and written via functions provided by
> the user, based on a generic closure pointer.  Future r2v / v2r
> functions could produce values of this sort, instead of using odd
> bitpos values.  So the kludge wouldn't last forever.

I'm not sure I see how this would solve the problem at hand: assume
r2v creates a value containing special functions to read and write
the register.  Then common code goes and creates a value refering
to a sub-field of that value.  How do we access that derived value?
Using the same access functions as provided for the full value --
but how do they know they should operate only on a part (which part)?
It would appear that this is exactly the same problem as we're
currently discussing ...

Bye,
Ulrich

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

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

[Jim Blandy]

"Ulrich Weigand" <uweigand@de.ibm.com> writes:
>> I've got unsubmitted patches for GDB that implement a new kind of
>> value, whose contents are read and written via functions provided by
>> the user, based on a generic closure pointer.  Future r2v / v2r
>> functions could produce values of this sort, instead of using odd
>> bitpos values.  So the kludge wouldn't last forever.
>
> I'm not sure I see how this would solve the problem at hand: assume
> r2v creates a value containing special functions to read and write
> the register.  Then common code goes and creates a value refering
> to a sub-field of that value.  How do we access that derived value?
> Using the same access functions as provided for the full value --
> but how do they know they should operate only on a part (which part)?
> It would appear that this is exactly the same problem as we're
> currently discussing ...

r2v would create a computed lvalue V whose closure indicates how the
value is encoded in the register.  V's contents are the decoded
register contents.  Common code goes and creates a value W referring
to a sub-field of the value.  W contains offset, bitpos and bitsize
values indicating the sub-field's position within the decoded
contents.

When W is read, its 'read' function is called, which reads the
register, does the decoding, and then extracts the appropriate
portion.

When W is assigned to, its 'write' function is called, which can do
the appropriate decode-modify-encode steps, and write the re-encoded
value back to the register.

The key is that r2v knows how to both decode the register's contents,
and how to re-encode both the full contents, or a portion of the
contents, so it can leave enough information in the closure to tell
v2r how to handle component writes.

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

[Ulrich Weigand]

Jim Blandy wrote:

> The key is that r2v knows how to both decode the register's contents,
> and how to re-encode both the full contents, or a portion of the
> contents, so it can leave enough information in the closure to tell
> v2r how to handle component writes.

I see.  So basically, instead of encoding the original type in the
struct value, you have accessor functions in the struct value, and
the back-end can install different versions of those functions if
it needs to handle types differently.  That would work as well.

I've prepared a set of two patches that fix my spu problem in a way
that I feel would be a step into the direction you describe.  I'll
post those shortly ...

Bye,
Ulrich

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