diff mbox

[2/2] Involve gdbarch in taking DWARF register pieces

Message ID m3a8kpx0ls.fsf@oc1027705133.ibm.com
State New, archived
Headers

Commit Message

Andreas Arnez April 19, 2016, 12:08 p.m. UTC 
  On Mon, Apr 18 2016, Pedro Alves wrote:

> How about this:
>
> Determine the physical placement of a type of size LEN within register
> *REGNUM, possibly overwriting *REGNUM.  (E.g., some ABIs lay vector
> types in registers pairs, and thus this method writes the correct pair
> element to *REGNUM).  Returns the byte offset of the data within the
> (possibly adjusted) register.  This method is used when determining
> the placement of a DWARF piece (DW_OP_piece), or when interpreting a
> register as a LEN-sized type, except when convert_register_p indicates
> that a special conversion is required instead.

Thanks, I like that wording.  However, I still see a slight potential
for misunderstanding:

* When using the definition "physical placement of a type of size LEN",
  the reader may be led to assume that LEN always specifies the length
  of the resulting type, not the length of an individual DWARF piece.

* I'm not sure about the example in parentheses.  It seems to imply that
  the input register may differ from the first resulting pair element.
  Is that based on a real scenario, or is it hypothetical?

* The disclaimer for convert_register_p may now be misinterpreted to
  apply to DWARF pieces as well.

Here's another attempt:

Determine the physical placement of a piece of size LEN within register
*REGNUM, possibly overwriting *REGNUM.  (E.g., some ABIs have unwindable
sub-registers embedded in non-unwindable full registers, and this method
diverts from the full register to the sub-register if possible.)
Returns the byte offset of the data within the (possibly adjusted)
register.  This method is used for determining the placement of a
LEN-sized DWARF piece (DW_OP_piece) and when interpreting a register as
a LEN-sized type (unless convert_register_p indicates that the type
needs a special conversion).

>
>>> I'd suggest even calling it "dwarf_register_piece_placement" for
>>> caller clarity?
>> 
>> Sure, but I find a name like 'gdbarch_dwarf_register_piece_placement' a
>> bit too unwieldy when trying to stick to an 80 char line size limit.
>> Maybe 'gdbarch_register_piece_placement'?
>
> Deal.  :-)

:-)

Updated patch below.

-- >8 --
Subject: [PATCH v2] Involve gdbarch in taking DWARF register pieces

On s390, when a 'long double' variable is located in a pair of FP
registers, its DWARF location looks something like this:

     DW_OP_reg24 (f8); DW_OP_piece: 8; DW_OP_reg25 (f10); DW_OP_piece: 8

If the target has a vector facility, each of its FP registers is
embedded left-aligned in a vector register with the same identical DWARF
register number.  But GDB always takes a register piece from the "least
significant" end, which is exactly opposite from where the FP register
is embedded.  This mismatch causes a regression with store.exp.

This change fixes this issue by taking s390 vector register pieces from
the left end instead.  Since this requires a suitable gdbarch interface,
the gdbarch method 'value_from_register' is replaced by a new method
'register_piece_placement' with similar functionality, but a slightly
different interface.  This method is then invoked in read_pieced_value and
write_pieced_value.

gdb/ChangeLog:

	* gdbarch.sh (value_from_register): Remove.
	(register_piece_placement): New.
	* gdbarch.c: Regenerate.
	* gdbarch.h: Likewise.
	* dwarf2loc.c (read_pieced_value): Get arch-specific placement of
	a register piece from gdbarch_register_piece_placement.
	(write_pieced_value): Likewise.
	* value.h (default_value_from_register): Remove.
	(default_register_piece_placement): Declare.
	* findvar.c (default_value_from_register): Remove.
	(default_register_piece_placement): New.
	(value_from_register): Call gdbarch_register_piece_placement
	instead of gdbarch_value_from_register.
	(address_from_register): Likewise.
	* s390-linux-tdep.c (s390_value_from_register): Remove.
	(s390_register_piece_placement): New.
	(s390_gdbarch_init): Set new gdbarch method
	register_piece_placement instead of value_from_register.
	* spu-tdep.c (spu_value_from_register): Remove.
	(spu_register_piece_placement): New.
	(spu_gdbarch_init): Set new gdbarch method
	register_piece_placement instead of value_from_register.
---
 gdb/dwarf2loc.c       | 35 +++++++++++++++++------------------
 gdb/findvar.c         | 42 +++++++++++++++++++++---------------------
 gdb/gdbarch.c         | 28 ++++++++++++++--------------
 gdb/gdbarch.h         | 21 +++++++++++++--------
 gdb/gdbarch.sh        | 17 +++++++++++------
 gdb/s390-linux-tdep.c | 32 ++++++++++++++------------------
 gdb/spu-tdep.c        | 21 +++++++--------------
 gdb/value.h           |  6 ++----
 8 files changed, 99 insertions(+), 103 deletions(-)

Comments

Andreas Arnez April 28, 2016, 1:24 p.m. UTC | #1 
Ping:

  https://sourceware.org/ml/gdb-patches/2016-04/msg00437.html

IIRC, there was some uncertainty about the clarity/meaning of the new
gdbarch method's description below.  Or has this cleared up by now?

On Tue, Apr 19 2016, Andreas Arnez wrote:

> Here's another attempt:
>
> Determine the physical placement of a piece of size LEN within register
> *REGNUM, possibly overwriting *REGNUM.  (E.g., some ABIs have unwindable
> sub-registers embedded in non-unwindable full registers, and this method
> diverts from the full register to the sub-register if possible.)
> Returns the byte offset of the data within the (possibly adjusted)
> register.  This method is used for determining the placement of a
> LEN-sized DWARF piece (DW_OP_piece) and when interpreting a register as
> a LEN-sized type (unless convert_register_p indicates that the type
> needs a special conversion).

[...]

> gdb/ChangeLog:
>
> 	* gdbarch.sh (value_from_register): Remove.
> 	(register_piece_placement): New.
> 	* gdbarch.c: Regenerate.
> 	* gdbarch.h: Likewise.
> 	* dwarf2loc.c (read_pieced_value): Get arch-specific placement of
> 	a register piece from gdbarch_register_piece_placement.
> 	(write_pieced_value): Likewise.
> 	* value.h (default_value_from_register): Remove.
> 	(default_register_piece_placement): Declare.
> 	* findvar.c (default_value_from_register): Remove.
> 	(default_register_piece_placement): New.
> 	(value_from_register): Call gdbarch_register_piece_placement
> 	instead of gdbarch_value_from_register.
> 	(address_from_register): Likewise.
> 	* s390-linux-tdep.c (s390_value_from_register): Remove.
> 	(s390_register_piece_placement): New.
> 	(s390_gdbarch_init): Set new gdbarch method
> 	register_piece_placement instead of value_from_register.
> 	* spu-tdep.c (spu_value_from_register): Remove.
> 	(spu_register_piece_placement): New.
> 	(spu_gdbarch_init): Set new gdbarch method
> 	register_piece_placement instead of value_from_register.
Pedro Alves April 28, 2016, 2:47 p.m. UTC | #2 
On 04/28/2016 02:24 PM, Andreas Arnez wrote:
> Ping:
> 
>   https://sourceware.org/ml/gdb-patches/2016-04/msg00437.html
> 
> IIRC, there was some uncertainty about the clarity/meaning of the new
> gdbarch method's description below.  Or has this cleared up by now?

Sorry, I've spent the last hour trying to wrap my head around this,
but I'm still confused.  :-/  I'm sorry to be blocking this.

> 
> On Tue, Apr 19 2016, Andreas Arnez wrote:
> 
>> Here's another attempt:
>>
>> Determine the physical placement of a piece of size LEN within register
>> *REGNUM, possibly overwriting *REGNUM.  (E.g., some ABIs have unwindable
>> sub-registers embedded in non-unwindable full registers, and this method
>> diverts from the full register to the sub-register if possible.)

I couldn't find any reference to "sub-register" in the codebase.
I'd assume it's something like "eax" being a sub part of "rax"
on x86-64.  But I'm not certain that's the case here?  On a machine with
vector registers, is a FP register really a chunk of the vector
register, or is it a real separate physical register?

My main confusion revolves I think, around how these points
are addressed:

 - FP registers and vector registers have the same identical
   DWARF register number.

 - If the object stored is <= 8 bytes, we should find it in
   the FP register; otherwise get it from the vector register.

I'd naively think that the fix for something like that would be
to make dwarf_reg_to_regnum return the gdb FP register number instead 
of the vector number, when the type fits in a FP register, instead of
the need for an extra diversion step.  Ignoring the fact that we don't
currently pass the type/size to gdbarch_dwarf_reg_to_regnum.

It may be that the end result is the same, but it's all blurry to
me still.

Thanks,
Pedro Alves
Andreas Arnez April 28, 2016, 4:51 p.m. UTC | #3 
On Thu, Apr 28 2016, Pedro Alves wrote:

> I couldn't find any reference to "sub-register" in the codebase.
> I'd assume it's something like "eax" being a sub part of "rax"
> on x86-64.  But I'm not certain that's the case here?  On a machine with
> vector registers, is a FP register really a chunk of the vector
> register, or is it a real separate physical register?

It's exactly comparable with eax and rax.  Or consider the SSE registers
xmm0-xmm15, which are embedded in their double-wide AVX counterparts
ymm0-ymm15.  With z/Architecture, each 64-bit FP register is just a
"chunk" ("sub-register" / "part" / "slice" / ...) of a 128-bit vector
register.  The ASCII art in section 2.1 of this article illustrates
this:

  https://sourceware.org/ml/gdb/2016-01/msg00013.html

(BTW, I still didn't get much feedback on that article...)

And if there is a better (or wider used) term than "sub-register", I'll
be happy to change the wording.

> My main confusion revolves I think, around how these points
> are addressed:
>
>  - FP registers and vector registers have the same identical
>    DWARF register number.
>
>  - If the object stored is <= 8 bytes, we should find it in
>    the FP register; otherwise get it from the vector register.
>
> I'd naively think that the fix for something like that would be
> to make dwarf_reg_to_regnum return the gdb FP register number instead 
> of the vector number, when the type fits in a FP register, instead of
> the need for an extra diversion step.  Ignoring the fact that we don't
> currently pass the type/size to gdbarch_dwarf_reg_to_regnum.

Right, ignoring that fact ;-)

Also, IMHO the "actual" placement of an object within a register does
not conceptually depend on where the register number came from.  It
could come from DWARF, from some other debug format, from the user, or
from wherever.  Adjusting the placement only for objects with a DWARF
location seems wrong to me.

--
Andreas
Andreas Arnez April 28, 2016, 6:16 p.m. UTC | #4 
On Thu, Apr 28 2016, Andreas Arnez wrote:

> On Thu, Apr 28 2016, Pedro Alves wrote:
>
>> I'd naively think that the fix for something like that would be
>> to make dwarf_reg_to_regnum return the gdb FP register number instead 
>> of the vector number, when the type fits in a FP register, instead of
>> the need for an extra diversion step.  Ignoring the fact that we don't
>> currently pass the type/size to gdbarch_dwarf_reg_to_regnum.
>
> Right, ignoring that fact ;-)
>
> Also, IMHO the "actual" placement of an object within a register does
> not conceptually depend on where the register number came from.  It
> could come from DWARF, from some other debug format, from the user, or
> from wherever.  Adjusting the placement only for objects with a DWARF
> location seems wrong to me.

There's another aspect I should probably mention here.  The adjustment
of the register number in this gdbarch method is really just a
circumvention around the register cache's inability of representing
*partially* valid registers.  Otherwise unwinding would result in the
first 64 bits of such a vector register being "valid" and the others
"invalid", and then we could slice any value types from the valid bits.

In the long run maybe we want to extend the regcache in such a
direction, but I didn't see that in the scope of this fix.

--
Andreas
Pedro Alves April 28, 2016, 10:14 p.m. UTC | #5 
On 04/28/2016 05:51 PM, Andreas Arnez wrote:
> On Thu, Apr 28 2016, Pedro Alves wrote:
> 
>> I couldn't find any reference to "sub-register" in the codebase.
>> I'd assume it's something like "eax" being a sub part of "rax"
>> on x86-64.  But I'm not certain that's the case here?  On a machine with
>> vector registers, is a FP register really a chunk of the vector
>> register, or is it a real separate physical register?
> 
> It's exactly comparable with eax and rax.  Or consider the SSE registers
> xmm0-xmm15, which are embedded in their double-wide AVX counterparts
> ymm0-ymm15.  With z/Architecture, each 64-bit FP register is just a
> "chunk" ("sub-register" / "part" / "slice" / ...) of a 128-bit vector
> register.  The ASCII art in section 2.1 of this article illustrates
> this:
> 
>   https://sourceware.org/ml/gdb/2016-01/msg00013.html

Thanks, this helps a lot.

> 
> (BTW, I still didn't get much feedback on that article...)
> 
> And if there is a better (or wider used) term than "sub-register", I'll
> be happy to change the wording.

No, that's fine terminology.  I was just confused because I wasn't very
clear whether we're talking about completely different registers.
Thanks,
Pedro Alves
Pedro Alves April 28, 2016, 10:15 p.m. UTC | #6 
On 04/28/2016 07:16 PM, Andreas Arnez wrote:

>> Also, IMHO the "actual" placement of an object within a register does
>> not conceptually depend on where the register number came from.  It
>> could come from DWARF, from some other debug format, from the user, or
>> from wherever.  Adjusting the placement only for objects with a DWARF
>> location seems wrong to me.
> 
> There's another aspect I should probably mention here.  The adjustment
> of the register number in this gdbarch method is really just a
> circumvention around the register cache's inability of representing
> *partially* valid registers.  Otherwise unwinding would result in the
> first 64 bits of such a vector register being "valid" and the others
> "invalid", and then we could slice any value types from the valid bits.

Ah, when I saw your other email pointing at the ascii art, the thought
of partially valid registers crossed my mind too.

But why do you say we don't support this?

The register cache only cares about registers in the current frame
(IOW, the real current state of a thread's registers).  And in the
current frame, the whole vector register is always valid.  So I don't
think we need to worry about the register cache.

The unwinding machinery instead works with struct values, and those _do_
support being marked partially optimized out (not saved).  You do that by
calling mark_value_bits_optimized_out on the part of the value that
is not saved.

Maybe there's e.g., some value_optimized_out() checks on common code
that might need to be replaced with a more finer-grained "these bytes/bit
here are optimized-out" check, but what else would be necessary?

> In the long run maybe we want to extend the regcache in such a
> direction, but I didn't see that in the scope of this fix.

Thanks,
Pedro Alves
Jan Kratochvil June 14, 2016, 5:03 p.m. UTC | #7 
On Thu, 28 Apr 2016 15:24:08 +0200, Andreas Arnez wrote:
> IIRC, there was some uncertainty about the clarity/meaning of the new
> gdbarch method's description below.  Or has this cleared up by now?

Ping (to Andreas Arnez) as this thread remained unfinished.


Jan
diff mbox

Patch

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index ba6ed42..d258a41 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1715,6 +1715,7 @@  read_pieced_value (struct value *v)
   for (i = 0; i < c->n_pieces && offset < type_len; i++)
     {
       struct dwarf_expr_piece *p = &c->pieces[i];
+      size_t piece_bytes = (p->size + 7) / 8;
       size_t this_size, this_size_bits;
       long dest_offset_bits, source_offset_bits, source_offset;
       const gdb_byte *intermediate_buffer;
@@ -1759,17 +1760,14 @@  read_pieced_value (struct value *v)
 	    struct gdbarch *arch = get_frame_arch (frame);
 	    int gdb_regnum = dwarf_reg_to_regnum_or_error (arch, p->v.regno);
 	    int optim, unavail;
-	    int reg_offset = source_offset;
+	    int reg_offset = gdbarch_register_piece_placement (arch,
+							       piece_bytes,
+							       &gdb_regnum);
 
-	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
-		&& this_size < register_size (arch, gdb_regnum))
-	      {
-		/* Big-endian, and we want less than full size.  */
-		reg_offset = register_size (arch, gdb_regnum) - this_size;
-		/* We want the lower-order THIS_SIZE_BITS of the bytes
-		   we extract from the register.  */
-		source_offset_bits += 8 * this_size - this_size_bits;
-	      }
+	    source_offset_bits += 8 * reg_offset;
+	    if (bits_big_endian)
+	      source_offset_bits += 8 * piece_bytes - this_size_bits;
+	    reg_offset = source_offset_bits / 8;
 
 	    if (!get_frame_register_bytes (frame, gdb_regnum, reg_offset,
 					   this_size, buffer,
@@ -1890,6 +1888,7 @@  write_pieced_value (struct value *to, struct value *from)
   for (i = 0; i < c->n_pieces && offset < type_len; i++)
     {
       struct dwarf_expr_piece *p = &c->pieces[i];
+      size_t piece_bytes = (p->size + 7) / 8;
       size_t this_size_bits, this_size;
       long dest_offset_bits, source_offset_bits, dest_offset, source_offset;
       int need_bitwise;
@@ -1941,14 +1940,14 @@  write_pieced_value (struct value *to, struct value *from)
 	  {
 	    struct gdbarch *arch = get_frame_arch (frame);
 	    int gdb_regnum = dwarf_reg_to_regnum_or_error (arch, p->v.regno);
-	    int reg_offset = dest_offset;
-
-	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
-		&& this_size <= register_size (arch, gdb_regnum))
-	      {
-		/* Big-endian, and we want less than full size.  */
-		reg_offset = register_size (arch, gdb_regnum) - this_size;
-	      }
+	    int reg_offset = gdbarch_register_piece_placement (arch,
+							       piece_bytes,
+							       &gdb_regnum);
+
+	    source_offset_bits += 8 * reg_offset;
+	    if (bits_big_endian)
+	      source_offset_bits += 8 * piece_bytes - this_size_bits;
+	    reg_offset = source_offset_bits / 8;
 
 	    if (need_bitwise)
 	      {
diff --git a/gdb/findvar.c b/gdb/findvar.c
index a39d897..0a305f5 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -797,29 +797,19 @@  read_var_value (struct symbol *var, const struct block *var_block,
 
 /* Install default attributes for register values.  */
 
-struct value *
-default_value_from_register (struct gdbarch *gdbarch, struct type *type,
-                             int regnum, struct frame_id frame_id)
+int
+default_register_piece_placement (struct gdbarch *gdbarch, int len, int *regnum)
 {
-  int len = TYPE_LENGTH (type);
-  struct value *value = allocate_value (type);
-
-  VALUE_LVAL (value) = lval_register;
-  VALUE_FRAME_ID (value) = frame_id;
-  VALUE_REGNUM (value) = regnum;
-
   /* Any structure stored in more than one register will always be
      an integral number of registers.  Otherwise, you need to do
      some fiddling with the last register copied here for little
      endian machines.  */
   if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG
-      && len < register_size (gdbarch, regnum))
+      && len < register_size (gdbarch, *regnum))
     /* Big-endian, and we want less than full size.  */
-    set_value_offset (value, register_size (gdbarch, regnum) - len);
-  else
-    set_value_offset (value, 0);
+    return register_size (gdbarch, *regnum) - len;
 
-  return value;
+  return 0;
 }
 
 /* VALUE must be an lval_register value.  If regnum is the value's
@@ -877,6 +867,10 @@  value_from_register (struct type *type, int regnum, struct frame_info *frame)
   struct type *type1 = check_typedef (type);
   struct value *v;
 
+  v = allocate_value (type);
+  VALUE_LVAL (v) = lval_register;
+  VALUE_FRAME_ID (v) = get_frame_id (frame);
+
   if (gdbarch_convert_register_p (gdbarch, regnum, type1))
     {
       int optim, unavail, ok;
@@ -888,9 +882,6 @@  value_from_register (struct type *type, int regnum, struct frame_info *frame)
          the corresponding [integer] type (see Alpha).  The assumption
          is that gdbarch_register_to_value populates the entire value
          including the location.  */
-      v = allocate_value (type);
-      VALUE_LVAL (v) = lval_register;
-      VALUE_FRAME_ID (v) = get_frame_id (frame);
       VALUE_REGNUM (v) = regnum;
       ok = gdbarch_register_to_value (gdbarch, frame, regnum, type1,
 				      value_contents_raw (v), &optim,
@@ -907,8 +898,11 @@  value_from_register (struct type *type, int regnum, struct frame_info *frame)
   else
     {
       /* Construct the value.  */
-      v = gdbarch_value_from_register (gdbarch, type,
-				       regnum, get_frame_id (frame));
+      int offset = gdbarch_register_piece_placement (gdbarch,
+						     TYPE_LENGTH (type),
+						     &regnum);
+      VALUE_REGNUM (v) = regnum;
+      set_value_offset (v, offset);
 
       /* Get the data.  */
       read_frame_register_value (v, frame);
@@ -927,6 +921,7 @@  address_from_register (int regnum, struct frame_info *frame)
   struct type *type = builtin_type (gdbarch)->builtin_data_ptr;
   struct value *value;
   CORE_ADDR result;
+  int offset;
   int regnum_max_excl = (gdbarch_num_regs (gdbarch)
 			 + gdbarch_num_pseudo_regs (gdbarch));
 
@@ -962,7 +957,12 @@  address_from_register (int regnum, struct frame_info *frame)
       return unpack_long (type, buf);
     }
 
-  value = gdbarch_value_from_register (gdbarch, type, regnum, null_frame_id);
+  offset = gdbarch_register_piece_placement (gdbarch, TYPE_LENGTH (type),
+					     &regnum);
+  value = allocate_value (type);
+  VALUE_LVAL (value) = lval_register;
+  VALUE_REGNUM (value) = regnum;
+  set_value_offset (value, offset);
   read_frame_register_value (value, frame);
 
   if (value_optimized_out (value))
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index bd0b48c..156373c 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -216,7 +216,7 @@  struct gdbarch
   gdbarch_convert_register_p_ftype *convert_register_p;
   gdbarch_register_to_value_ftype *register_to_value;
   gdbarch_value_to_register_ftype *value_to_register;
-  gdbarch_value_from_register_ftype *value_from_register;
+  gdbarch_register_piece_placement_ftype *register_piece_placement;
   gdbarch_pointer_to_address_ftype *pointer_to_address;
   gdbarch_address_to_pointer_ftype *address_to_pointer;
   gdbarch_integer_to_address_ftype *integer_to_address;
@@ -393,7 +393,7 @@  gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->cannot_fetch_register = cannot_register_not;
   gdbarch->cannot_store_register = cannot_register_not;
   gdbarch->convert_register_p = generic_convert_register_p;
-  gdbarch->value_from_register = default_value_from_register;
+  gdbarch->register_piece_placement = default_register_piece_placement;
   gdbarch->pointer_to_address = unsigned_pointer_to_address;
   gdbarch->address_to_pointer = unsigned_address_to_pointer;
   gdbarch->return_in_first_hidden_param_p = default_return_in_first_hidden_param_p;
@@ -560,7 +560,7 @@  verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of cannot_store_register, invalid_p == 0 */
   /* Skip verify of get_longjmp_target, has predicate.  */
   /* Skip verify of convert_register_p, invalid_p == 0 */
-  /* Skip verify of value_from_register, invalid_p == 0 */
+  /* Skip verify of register_piece_placement, invalid_p == 0 */
   /* Skip verify of pointer_to_address, invalid_p == 0 */
   /* Skip verify of address_to_pointer, invalid_p == 0 */
   /* Skip verify of integer_to_address, has predicate.  */
@@ -1242,6 +1242,9 @@  gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
                       "gdbarch_dump: register_name = <%s>\n",
                       host_address_to_string (gdbarch->register_name));
   fprintf_unfiltered (file,
+                      "gdbarch_dump: register_piece_placement = <%s>\n",
+                      host_address_to_string (gdbarch->register_piece_placement));
+  fprintf_unfiltered (file,
                       "gdbarch_dump: register_reggroup_p = <%s>\n",
                       host_address_to_string (gdbarch->register_reggroup_p));
   fprintf_unfiltered (file,
@@ -1398,9 +1401,6 @@  gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
                       "gdbarch_dump: unwind_sp = <%s>\n",
                       host_address_to_string (gdbarch->unwind_sp));
   fprintf_unfiltered (file,
-                      "gdbarch_dump: value_from_register = <%s>\n",
-                      host_address_to_string (gdbarch->value_from_register));
-  fprintf_unfiltered (file,
                       "gdbarch_dump: value_to_register = <%s>\n",
                       host_address_to_string (gdbarch->value_to_register));
   fprintf_unfiltered (file,
@@ -2514,21 +2514,21 @@  set_gdbarch_value_to_register (struct gdbarch *gdbarch,
   gdbarch->value_to_register = value_to_register;
 }
 
-struct value *
-gdbarch_value_from_register (struct gdbarch *gdbarch, struct type *type, int regnum, struct frame_id frame_id)
+int
+gdbarch_register_piece_placement (struct gdbarch *gdbarch, int len, int *regnum)
 {
   gdb_assert (gdbarch != NULL);
-  gdb_assert (gdbarch->value_from_register != NULL);
+  gdb_assert (gdbarch->register_piece_placement != NULL);
   if (gdbarch_debug >= 2)
-    fprintf_unfiltered (gdb_stdlog, "gdbarch_value_from_register called\n");
-  return gdbarch->value_from_register (gdbarch, type, regnum, frame_id);
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_register_piece_placement called\n");
+  return gdbarch->register_piece_placement (gdbarch, len, regnum);
 }
 
 void
-set_gdbarch_value_from_register (struct gdbarch *gdbarch,
-                                 gdbarch_value_from_register_ftype value_from_register)
+set_gdbarch_register_piece_placement (struct gdbarch *gdbarch,
+                                      gdbarch_register_piece_placement_ftype register_piece_placement)
 {
-  gdbarch->value_from_register = value_from_register;
+  gdbarch->register_piece_placement = register_piece_placement;
 }
 
 CORE_ADDR
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 252fc4b..b867635 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -450,14 +450,19 @@  typedef void (gdbarch_value_to_register_ftype) (struct frame_info *frame, int re
 extern void gdbarch_value_to_register (struct gdbarch *gdbarch, struct frame_info *frame, int regnum, struct type *type, const gdb_byte *buf);
 extern void set_gdbarch_value_to_register (struct gdbarch *gdbarch, gdbarch_value_to_register_ftype *value_to_register);
 
-/* Construct a value representing the contents of register REGNUM in
-   frame FRAME_ID, interpreted as type TYPE.  The routine needs to
-   allocate and return a struct value with all value attributes
-   (but not the value contents) filled in. */
-
-typedef struct value * (gdbarch_value_from_register_ftype) (struct gdbarch *gdbarch, struct type *type, int regnum, struct frame_id frame_id);
-extern struct value * gdbarch_value_from_register (struct gdbarch *gdbarch, struct type *type, int regnum, struct frame_id frame_id);
-extern void set_gdbarch_value_from_register (struct gdbarch *gdbarch, gdbarch_value_from_register_ftype *value_from_register);
+/* Determine the physical placement of a piece of size LEN within register
+   *REGNUM, possibly overwriting *REGNUM.  (E.g., some ABIs have unwindable
+   sub-registers embedded in non-unwindable full registers, and this method
+   diverts from the full register to the sub-register if possible.)
+   Returns the byte offset of the data within the (possibly adjusted)
+   register.  This method is used for determining the placement of a
+   LEN-sized DWARF piece (DW_OP_piece) and when interpreting a register as
+   a LEN-sized type (unless convert_register_p indicates that the type
+   needs a special conversion). */
+
+typedef int (gdbarch_register_piece_placement_ftype) (struct gdbarch *gdbarch, int len, int *regnum);
+extern int gdbarch_register_piece_placement (struct gdbarch *gdbarch, int len, int *regnum);
+extern void set_gdbarch_register_piece_placement (struct gdbarch *gdbarch, gdbarch_register_piece_placement_ftype *register_piece_placement);
 
 typedef CORE_ADDR (gdbarch_pointer_to_address_ftype) (struct gdbarch *gdbarch, struct type *type, const gdb_byte *buf);
 extern CORE_ADDR gdbarch_pointer_to_address (struct gdbarch *gdbarch, struct type *type, const gdb_byte *buf);
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 37f59b7..8f02a1a 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -24,7 +24,6 @@ 
 LANG=C ; export LANG
 LC_ALL=C ; export LC_ALL
 
-
 compare_new ()
 {
     file=$1
@@ -506,11 +505,17 @@  v:int:believe_pcc_promotion:::::::
 m:int:convert_register_p:int regnum, struct type *type:regnum, type:0:generic_convert_register_p::0
 f:int:register_to_value:struct frame_info *frame, int regnum, struct type *type, gdb_byte *buf, int *optimizedp, int *unavailablep:frame, regnum, type, buf, optimizedp, unavailablep:0
 f:void:value_to_register:struct frame_info *frame, int regnum, struct type *type, const gdb_byte *buf:frame, regnum, type, buf:0
-# Construct a value representing the contents of register REGNUM in
-# frame FRAME_ID, interpreted as type TYPE.  The routine needs to
-# allocate and return a struct value with all value attributes
-# (but not the value contents) filled in.
-m:struct value *:value_from_register:struct type *type, int regnum, struct frame_id frame_id:type, regnum, frame_id::default_value_from_register::0
+
+# Determine the physical placement of a piece of size LEN within register
+# *REGNUM, possibly overwriting *REGNUM.  (E.g., some ABIs have unwindable
+# sub-registers embedded in non-unwindable full registers, and this method
+# diverts from the full register to the sub-register if possible.)
+# Returns the byte offset of the data within the (possibly adjusted)
+# register.  This method is used for determining the placement of a
+# LEN-sized DWARF piece (DW_OP_piece) and when interpreting a register as
+# a LEN-sized type (unless convert_register_p indicates that the type
+# needs a special conversion).
+m:int:register_piece_placement:int len, int *regnum:len, regnum::default_register_piece_placement::0
 #
 m:CORE_ADDR:pointer_to_address:struct type *type, const gdb_byte *buf:type, buf::unsigned_pointer_to_address::0
 m:void:address_to_pointer:struct type *type, gdb_byte *buf, CORE_ADDR addr:type, buf, addr::unsigned_address_to_pointer::0
diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
index 8b4efb1..cb2baf1 100644
--- a/gdb/s390-linux-tdep.c
+++ b/gdb/s390-linux-tdep.c
@@ -513,28 +513,24 @@  s390_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache,
    registers, even though we are otherwise a big-endian platform.  The
    same applies to a 'float' value within a vector.  */
 
-static struct value *
-s390_value_from_register (struct gdbarch *gdbarch, struct type *type,
-			  int regnum, struct frame_id frame_id)
+static int
+s390_register_piece_placement (struct gdbarch *gdbarch, int len,
+			       int *regnum)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
-  unsigned int len = TYPE_LENGTH (check_typedef (type));
-  struct value *value;
 
   /* If the value fits in a sub-register, use that instead.  */
-  if (regnum_is_vxr_full (tdep, regnum) && len <= 8)
-    regnum = regnum - tdep->v0_full_regnum + S390_F0_REGNUM;
-  else if (regnum_is_gpr_full (tdep, regnum) && len <= 4)
-    regnum = regnum - tdep->gpr_full_regnum + S390_R0_REGNUM;
-
-  value = default_value_from_register (gdbarch, type, regnum, frame_id);
-
-  if ((regnum >= S390_F0_REGNUM && regnum <= S390_F15_REGNUM && len < 8)
-      || regnum_is_vxr_full (tdep, regnum)
-      || (regnum >= S390_V16_REGNUM && regnum <= S390_V31_REGNUM))
-    set_value_offset (value, 0);
+  if (regnum_is_vxr_full (tdep, *regnum) && len <= 8)
+    *regnum = *regnum - tdep->v0_full_regnum + S390_F0_REGNUM;
+  else if (regnum_is_gpr_full (tdep, *regnum) && len <= 4)
+    *regnum = *regnum - tdep->gpr_full_regnum + S390_R0_REGNUM;
+
+  if ((*regnum >= S390_F0_REGNUM && *regnum <= S390_F15_REGNUM && len < 8)
+      || regnum_is_vxr_full (tdep, *regnum)
+      || (*regnum >= S390_V16_REGNUM && *regnum <= S390_V31_REGNUM))
+    return 0;
 
-  return value;
+  return default_register_piece_placement (gdbarch, len, regnum);
 }
 
 /* Register groups.  */
@@ -8005,7 +8001,7 @@  s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_fp0_regnum (gdbarch, S390_F0_REGNUM);
   set_gdbarch_stab_reg_to_regnum (gdbarch, s390_dwarf_reg_to_regnum);
   set_gdbarch_dwarf2_reg_to_regnum (gdbarch, s390_dwarf_reg_to_regnum);
-  set_gdbarch_value_from_register (gdbarch, s390_value_from_register);
+  set_gdbarch_register_piece_placement (gdbarch, s390_register_piece_placement);
   set_gdbarch_core_read_description (gdbarch, s390_core_read_description);
   set_gdbarch_iterate_over_regset_sections (gdbarch,
 					    s390_iterate_over_regset_sections);
diff --git a/gdb/spu-tdep.c b/gdb/spu-tdep.c
index 8dad5c3..15ca5ac 100644
--- a/gdb/spu-tdep.c
+++ b/gdb/spu-tdep.c
@@ -357,21 +357,14 @@  spu_ax_pseudo_register_push_stack (struct gdbarch *gdbarch,
 
 /* Value conversion -- access scalar values at the preferred slot.  */
 
-static struct value *
-spu_value_from_register (struct gdbarch *gdbarch, struct type *type,
-			 int regnum, struct frame_id frame_id)
+static int
+spu_register_piece_placement (struct gdbarch *gdbarch, int len,
+			      int *regnum)
 {
-  struct value *value = default_value_from_register (gdbarch, type,
-						     regnum, frame_id);
-  int len = TYPE_LENGTH (type);
-
-  if (regnum < SPU_NUM_GPRS && len < 16)
-    {
-      int preferred_slot = len < 4 ? 4 - len : 0;
-      set_value_offset (value, preferred_slot);
-    }
+  if (*regnum < SPU_NUM_GPRS && len < 16)
+    return len < 4 ? 4 - len : 0;
 
-  return value;
+  return default_register_piece_placement (gdbarch, len, regnum);
 }
 
 /* Register groups.  */
@@ -2738,7 +2731,7 @@  spu_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_register_type (gdbarch, spu_register_type);
   set_gdbarch_pseudo_register_read (gdbarch, spu_pseudo_register_read);
   set_gdbarch_pseudo_register_write (gdbarch, spu_pseudo_register_write);
-  set_gdbarch_value_from_register (gdbarch, spu_value_from_register);
+  set_gdbarch_register_piece_placement (gdbarch, spu_register_piece_placement);
   set_gdbarch_register_reggroup_p (gdbarch, spu_register_reggroup_p);
   set_gdbarch_dwarf2_reg_to_regnum (gdbarch, spu_dwarf_reg_to_regnum);
   set_gdbarch_ax_pseudo_register_collect
diff --git a/gdb/value.h b/gdb/value.h
index f8ec854..c7fc28e 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -647,10 +647,8 @@  extern struct value *value_from_contents_and_address (struct type *,
 						      CORE_ADDR);
 extern struct value *value_from_contents (struct type *, const gdb_byte *);
 
-extern struct value *default_value_from_register (struct gdbarch *gdbarch,
-						  struct type *type,
-						  int regnum,
-						  struct frame_id frame_id);
+extern int default_register_piece_placement (struct gdbarch *gdbarch,
+					     int len, int *regnum);
 
 extern void read_frame_register_value (struct value *value,
 				       struct frame_info *frame);