diff mbox

[6/9] Fix handling of DWARF register pieces on big-endian targets

Message ID 1491586736-21296-7-git-send-email-arnez@linux.vnet.ibm.com
State New
Headers show

Commit Message

Andreas Arnez April 7, 2017, 5:38 p.m. UTC
For big-endian targets the logic in read/write_pieced_value tries to take
a register piece from the LSB end.  This requires offsets and sizes to be
adjusted accordingly, and that's where the current implementation has some
issues:

* The formulas for recalculating the bit- and byte-offsets into the
  register are wrong.  They just happen to yield correct results if
  everything is byte-aligned and the piece's last byte belongs to the
  given value.

* After recalculating the bit offset into the register, the number of
  bytes to be copied from the register is not recalculated.  Of course
  this does not matter if everything (particularly the piece size) is
  byte-aligned.

These issues are fixed.  The size calculation is performed with a new
helper function bits_to_bytes().  For simplification, the variable
buffer_size is dropped.

gdb/ChangeLog:

	* dwarf2loc.c (bits_to_bytes): New function.
	(read_pieced_value): Fix offset calculations for register pieces
	on big-endian targets.
	(write_pieced_value): Likewise.

gdb/testsuite/ChangeLog:

	* gdb.dwarf2/var-access.exp: Add test for non-byte-aligned
	register pieces.
---
 gdb/dwarf2loc.c                         | 74 +++++++++++++++++----------------
 gdb/testsuite/gdb.dwarf2/var-access.exp | 32 ++++++++++++++
 2 files changed, 70 insertions(+), 36 deletions(-)

Comments

Simon Marchi April 14, 2017, 2:10 p.m. UTC | #1
On 2017-04-07 13:38, Andreas Arnez wrote:
> For big-endian targets the logic in read/write_pieced_value tries to 
> take
> a register piece from the LSB end.  This requires offsets and sizes to 
> be
> adjusted accordingly, and that's where the current implementation has 
> some
> issues:
> 
> * The formulas for recalculating the bit- and byte-offsets into the
>   register are wrong.  They just happen to yield correct results if
>   everything is byte-aligned and the piece's last byte belongs to the
>   given value.
> 
> * After recalculating the bit offset into the register, the number of
>   bytes to be copied from the register is not recalculated.  Of course
>   this does not matter if everything (particularly the piece size) is
>   byte-aligned.
> 
> These issues are fixed.  The size calculation is performed with a new
> helper function bits_to_bytes().  For simplification, the variable
> buffer_size is dropped.
> 
> gdb/ChangeLog:
> 
> 	* dwarf2loc.c (bits_to_bytes): New function.
> 	(read_pieced_value): Fix offset calculations for register pieces
> 	on big-endian targets.
> 	(write_pieced_value): Likewise.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.dwarf2/var-access.exp: Add test for non-byte-aligned
> 	register pieces.
> ---
>  gdb/dwarf2loc.c                         | 74 
> +++++++++++++++++----------------
>  gdb/testsuite/gdb.dwarf2/var-access.exp | 32 ++++++++++++++
>  2 files changed, 70 insertions(+), 36 deletions(-)
> 
> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index 1f89a08..d13c0c5 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c
> @@ -1751,6 +1751,15 @@ copy_bitwise_tests (void)
> 
>  #endif /* GDB_SELF_TEST */
> 
> +/* Return the number of bytes overlapping a contiguous chunk of N_BITS
> +   bits whose first bit is located at bit offset START.  */
> +
> +static size_t
> +bits_to_bytes (ULONGEST start, ULONGEST n_bits)
> +{
> +  return (start % 8 + n_bits + 7) / 8;
> +}
> +
>  static void
>  read_pieced_value (struct value *v)
>  {
> @@ -1761,7 +1770,6 @@ read_pieced_value (struct value *v)
>    struct piece_closure *c
>      = (struct piece_closure *) value_computed_closure (v);
>    size_t type_len;
> -  size_t buffer_size = 0;
>    std::vector<gdb_byte> buffer;
>    int bits_big_endian
>      = gdbarch_bits_big_endian (get_type_arch (value_type (v)));
> @@ -1805,13 +1813,9 @@ read_pieced_value (struct value *v)
>        if (this_size_bits > type_len - offset)
>  	this_size_bits = type_len - offset;
> 
> -      this_size = (this_size_bits + source_offset_bits % 8 + 7) / 8;
> +      this_size = bits_to_bytes (source_offset_bits, this_size_bits);
> +      buffer.reserve (this_size);
>        source_offset = source_offset_bits / 8;
> -      if (buffer_size < this_size)
> -	{
> -	  buffer_size = this_size;
> -	  buffer.reserve (buffer_size);
> -	}

The removal of buffer_size and this "if" (and equivalent in 
write_pieced_value) seems to be unrelated to the problem you are fixing, 
but rather a simplification that could be done anyway.  If that's the 
case, you should bundle it with the similar changes from the previous 
patch, in a patch that only does some refactoring but not behavior 
changes.

>        intermediate_buffer = buffer.data ();
> 
>        /* Copy from the source to DEST_BUFFER.  */
> @@ -1822,22 +1826,22 @@ read_pieced_value (struct value *v)
>  	    struct frame_info *frame = frame_find_by_id (c->frame_id);
>  	    struct gdbarch *arch = get_frame_arch (frame);
>  	    int gdb_regnum = dwarf_reg_to_regnum_or_error (arch, p->v.regno);
> +	    ULONGEST reg_bits = 8 * register_size (arch, gdb_regnum);
>  	    int optim, unavail;
> -	    LONGEST reg_offset = source_offset;
> 
>  	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
> -		&& this_size < register_size (arch, gdb_regnum))
> +		&& p->size < reg_bits)
>  	      {
>  		/* 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 += reg_bits - p->size;
>  	      }
> +	    this_size = bits_to_bytes (source_offset_bits, this_size_bits);
> +	    buffer.reserve (this_size);
> 
> -	    if (!get_frame_register_bytes (frame, gdb_regnum, reg_offset,
> -					   this_size, buffer.data (),
> -					   &optim, &unavail))
> +	    if (!get_frame_register_bytes (frame, gdb_regnum,
> +					  source_offset_bits / 8,
> +					  this_size, buffer.data (),
> +					  &optim, &unavail))
>  	      {
>  		/* Just so garbage doesn't ever shine through.  */
>  		memset (buffer.data (), 0, this_size);
> @@ -1847,9 +1851,8 @@ read_pieced_value (struct value *v)
>  		if (unavail)
>  		  mark_value_bits_unavailable (v, offset, this_size_bits);
>  	      }
> -
>  	    copy_bitwise (contents, dest_offset_bits,
> -			  intermediate_buffer, source_offset_bits % 8,
> +			  buffer.data (), source_offset_bits % 8,
>  			  this_size_bits, bits_big_endian);
>  	  }
>  	  break;
> @@ -1927,7 +1930,6 @@ write_pieced_value (struct value *to, struct 
> value *from)
>    struct piece_closure *c
>      = (struct piece_closure *) value_computed_closure (to);
>    size_t type_len;
> -  size_t buffer_size = 0;
>    std::vector<gdb_byte> buffer;
>    int bits_big_endian
>      = gdbarch_bits_big_endian (get_type_arch (value_type (to)));
> @@ -1972,7 +1974,7 @@ write_pieced_value (struct value *to, struct 
> value *from)
>        if (this_size_bits > type_len - offset)
>  	this_size_bits = type_len - offset;
> 
> -      this_size = (this_size_bits + dest_offset_bits % 8 + 7) / 8;
> +      this_size = bits_to_bytes (dest_offset_bits, this_size_bits);
>        source_offset = source_offset_bits / 8;
>        dest_offset = dest_offset_bits / 8;
>        if (dest_offset_bits % 8 == 0 && source_offset_bits % 8 == 0
> @@ -1983,11 +1985,7 @@ write_pieced_value (struct value *to, struct 
> value *from)
>  	}
>        else
>  	{
> -	  if (buffer_size < this_size)
> -	    {
> -	      buffer_size = this_size;
> -	      buffer.reserve (buffer_size);
> -	    }
> +	  buffer.reserve (this_size);
>  	  source_buffer = buffer.data ();
>  	  need_bitwise = 1;
>  	}
> @@ -1999,20 +1997,24 @@ write_pieced_value (struct value *to, struct
> value *from)
>  	    struct frame_info *frame = frame_find_by_id (c->frame_id);
>  	    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;
> +	    ULONGEST reg_bits = 8 * register_size (arch, gdb_regnum);
> 
>  	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
> -		&& this_size <= register_size (arch, gdb_regnum))
> +		&& p->size <= reg_bits)
>  	      {
>  		/* Big-endian, and we want less than full size.  */
> -		reg_offset = register_size (arch, gdb_regnum) - this_size;
> +		dest_offset_bits += reg_bits - p->size;
>  	      }
> +	    this_size = bits_to_bytes (dest_offset_bits, this_size_bits);
> +	    buffer.reserve (this_size);
> 
> -	    if (need_bitwise)
> +	    if (dest_offset_bits % 8 != 0 || this_size_bits % 8 != 0)

I thought the "need_bitwise" variable name helped to understand.  To 
compensate, could you add a comment explaining the "why" of that if?

>  	      {
> +		/* Need some bits from original register value.  */
>  		int optim, unavail;
> 
> -		if (!get_frame_register_bytes (frame, gdb_regnum, reg_offset,
> +		if (!get_frame_register_bytes (frame, gdb_regnum,
> +					       dest_offset_bits / 8,
>  					       this_size, buffer.data (),
>  					       &optim, &unavail))
>  		  {
> @@ -2027,14 +2029,14 @@ write_pieced_value (struct value *to, struct
> value *from)
>  				     "bitfield; containing word "
>  				     "is unavailable"));
>  		  }
> -		copy_bitwise (buffer.data (), dest_offset_bits,
> -			      contents, source_offset_bits,
> -			      this_size_bits,
> -			      bits_big_endian);
>  	      }
> 
> -	    put_frame_register_bytes (frame, gdb_regnum, reg_offset,
> -				      this_size, source_buffer);
> +	    copy_bitwise (buffer.data (), dest_offset_bits % 8,
> +			  contents, source_offset_bits,
> +			  this_size_bits, bits_big_endian);
> +	    put_frame_register_bytes (frame, gdb_regnum,
> +				      dest_offset_bits / 8,
> +				      this_size, buffer.data ());
>  	  }
>  	  break;
>  	case DWARF_VALUE_MEMORY:
> diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp
> b/gdb/testsuite/gdb.dwarf2/var-access.exp
> index c6abc87..4787dfb 100644
> --- a/gdb/testsuite/gdb.dwarf2/var-access.exp
> +++ b/gdb/testsuite/gdb.dwarf2/var-access.exp
> @@ -192,6 +192,18 @@ Dwarf::assemble $asm_file {
>  			piece 1
>  		    } SPECIAL_expr}
>  		}
> +		# Register pieces for bitfield access.
> +		DW_TAG_variable {
> +		    {name "t2"}
> +		    {type :$struct_t_label}
> +		    {location {
> +			piece 4
> +			regx [lindex $dwarf_regnum 0]
> +			piece 3
> +			regx [lindex $dwarf_regnum 1]
> +			piece 1
> +		    } SPECIAL_expr}
> +		}
>  	    }
>  	}
>      }
> @@ -206,6 +218,15 @@ if ![runto_main] {
>      return -1
>  }
> 
> +# Determine endianness.
> +set endian "little"
> +gdb_test_multiple "show endian" "determine endianness" {
> +    -re ".* (big|little) endian.*$gdb_prompt $" {
> +	set endian $expect_out(1,string)
> +	pass "endianness: $endian"
> +    }
> +}

"pass" should be passed the test name, which should be stable as much as 
possible.  The typical way to do it would be:

   # Determine endianness.
   set endian "little"
   set test "determine endianness"
   gdb_test_multiple "show endian" $test {
       -re ".* (big|little) endian.*$gdb_prompt $" {
           set endian $expect_out(1,string)
           pass $test
       }
   }

> +
>  # Byte-aligned memory pieces.
>  gdb_test "print/d s1" " = \\{a = 2, b = 3, c = 0, d = 1\\}" \
>      "s1 == re-ordered buf"
> @@ -244,3 +265,14 @@ gdb_test_no_output "set var t1.y = 1234"
>  gdb_test "print t1" " = \\{u = <optimized out>, x = -7, y = 1234, z = 
> 340\\}" \
>      "verify t1"
> 
> +switch $endian { big {set val 0x7ffc} little {set val 0x3ffe00} }

I had to draw this in order to understand, so it might be useful to 
others.

For little endian (assuming the register is 32 bits):

             3    f     f    e     0    0
xxxx xxxx  0011 1111  1111 1110  0000 0000
            ^^                    ^^^^ ^^^^
            | ^^ ^^^^  ^^^^ ^^^   ` x
            | `- y
            `- part of z


For big endian (assuming the register is 32 bits):

             0    0     7    f     f    c
xxxx xxxx  0000 0000  0111 1111  1111 1100
            ^^^^ ^^^^  ^                 ^^
            `- x        ^^^ ^^^^  ^^^^ ^^ `- part of z
                        `- y

Not having worked with a big endian machine before, I'm still confused 
in how bits are aligned in the register and which bit means what.

> +gdb_test_no_output "set var \$[lindex $regname 0] = $val" \
> +    "init t2, first piece"
> +gdb_test_no_output "set var \$[lindex $regname 1] = 0" \
> +    "init t2, second piece"
> +gdb_test "print/d t2" " = \\{u = <optimized out>, x = 0, y = -1, z = 
> 0\\}" \
> +    "initialized t2 from regs"
> +gdb_test_no_output "set var t2.y = 2641"
> +gdb_test_no_output "set var t2.z = -400"
> +gdb_test_no_output "set var t2.x = 200"
> +gdb_test "print t2.x + t2.y + t2.z" " = 2441"

Thanks,

Simon
Andreas Arnez April 19, 2017, 6:03 p.m. UTC | #2
On Fri, Apr 14 2017, Simon Marchi wrote:

> On 2017-04-07 13:38, Andreas Arnez wrote:

[...]

>> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
>> index 1f89a08..d13c0c5 100644
>> --- a/gdb/dwarf2loc.c
>> +++ b/gdb/dwarf2loc.c

[...]

>> @@ -1805,13 +1813,9 @@ read_pieced_value (struct value *v)
>>        if (this_size_bits > type_len - offset)
>>  	this_size_bits = type_len - offset;
>>
>> -      this_size = (this_size_bits + source_offset_bits % 8 + 7) / 8;
>> +      this_size = bits_to_bytes (source_offset_bits, this_size_bits);
>> +      buffer.reserve (this_size);
>>        source_offset = source_offset_bits / 8;
>> -      if (buffer_size < this_size)
>> -	{
>> -	  buffer_size = this_size;
>> -	  buffer.reserve (buffer_size);
>> -	}
>
> The removal of buffer_size and this "if" (and equivalent in
> write_pieced_value) seems to be unrelated to the problem you are fixing,
> but rather a simplification that could be done anyway.  If that's the
> case, you should bundle it with the similar changes from the previous
> patch, in a patch that only does some refactoring but not behavior
> changes.

This particular simplification is triggered by the fact that we'd
otherwise have to duplicate this code, now that the buffer reservation
is potentially repeated.  So it is in fact related to the fix.

But you're right that it could be done anyway.  Thus I'll extract a tiny
patch just with that change and insert it before this one.

[...]

>> @@ -1999,20 +1997,24 @@ write_pieced_value (struct value *to, struct
>> value *from)
>>  	    struct frame_info *frame = frame_find_by_id (c->frame_id);
>>  	    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;
>> +	    ULONGEST reg_bits = 8 * register_size (arch, gdb_regnum);
>>
>>  	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
>> -		&& this_size <= register_size (arch, gdb_regnum))
>> +		&& p->size <= reg_bits)
>>  	      {
>>  		/* Big-endian, and we want less than full size.  */
>> -		reg_offset = register_size (arch, gdb_regnum) - this_size;
>> +		dest_offset_bits += reg_bits - p->size;
>>  	      }
>> +	    this_size = bits_to_bytes (dest_offset_bits, this_size_bits);
>> +	    buffer.reserve (this_size);
>>
>> -	    if (need_bitwise)
>> +	    if (dest_offset_bits % 8 != 0 || this_size_bits % 8 != 0)
>
> I thought the "need_bitwise" variable name helped to understand.  To
> compensate, could you add a comment explaining the "why" of that if?

That's what the next comment is intended for.  I can enhance it, though:

		/* Data is copied non-byte-aligned into the register.
		   Need some bits from original register value.  */

>
>>  	      {
>> +		/* Need some bits from original register value.  */
>>  		int optim, unavail;
>>

[...]

>> diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp
>> b/gdb/testsuite/gdb.dwarf2/var-access.exp
>> index c6abc87..4787dfb 100644
>> --- a/gdb/testsuite/gdb.dwarf2/var-access.exp
>> +++ b/gdb/testsuite/gdb.dwarf2/var-access.exp

[...]

>> @@ -206,6 +218,15 @@ if ![runto_main] {
>>      return -1
>>  }
>>
>> +# Determine endianness.
>> +set endian "little"
>> +gdb_test_multiple "show endian" "determine endianness" {
>> +    -re ".* (big|little) endian.*$gdb_prompt $" {
>> +	set endian $expect_out(1,string)
>> +	pass "endianness: $endian"
>> +    }
>> +}
>
> "pass" should be passed the test name, which should be stable as much as
> possible.  The typical way to do it would be:
>
>   # Determine endianness.
>   set endian "little"
>   set test "determine endianness"
>   gdb_test_multiple "show endian" $test {
>       -re ".* (big|little) endian.*$gdb_prompt $" {
>           set endian $expect_out(1,string)
>           pass $test
>       }
>   }

The endianness determination should be extracted into a library function
anyway, since it exists many times already.  I'll do that in V2.

>
>> +
>>  # Byte-aligned memory pieces.
>>  gdb_test "print/d s1" " = \\{a = 2, b = 3, c = 0, d = 1\\}" \
>>      "s1 == re-ordered buf"
>> @@ -244,3 +265,14 @@ gdb_test_no_output "set var t1.y = 1234"
>>  gdb_test "print t1" " = \\{u = <optimized out>, x = -7, y = 1234, z =
>> 340\\}" \
>>      "verify t1"
>>
>> +switch $endian { big {set val 0x7ffc} little {set val 0x3ffe00} }
>
> I had to draw this in order to understand, so it might be useful to
> others.
>
> For little endian (assuming the register is 32 bits):
>
>             3    f     f    e     0    0
> xxxx xxxx  0011 1111  1111 1110  0000 0000
>            ^^                    ^^^^ ^^^^
>            | ^^ ^^^^  ^^^^ ^^^   ` x
>            | `- y
>            `- part of z
>
>
> For big endian (assuming the register is 32 bits):
>
>             0    0     7    f     f    c
> xxxx xxxx  0000 0000  0111 1111  1111 1100
>            ^^^^ ^^^^  ^                 ^^
>            `- x        ^^^ ^^^^  ^^^^ ^^ `- part of z
>                        `- y

Very nice :-)

>
> Not having worked with a big endian machine before, I'm still confused in
> how bits are aligned in the register and which bit means what.

Note that your little-endian picture shows bytes in reversed addressing
order, i.e., highest-addressed byte left, and lowest-addressed byte
right.  This is OK, and the Intel ISA does it in the same way, but I
find this aspect of the little-endian notation confusing as well ;-)

>
>> +gdb_test_no_output "set var \$[lindex $regname 0] = $val" \
>> +    "init t2, first piece"
>> +gdb_test_no_output "set var \$[lindex $regname 1] = 0" \
>> +    "init t2, second piece"
>> +gdb_test "print/d t2" " = \\{u = <optimized out>, x = 0, y = -1, z =
>> 0\\}" \
>> +    "initialized t2 from regs"
>> +gdb_test_no_output "set var t2.y = 2641"
>> +gdb_test_no_output "set var t2.z = -400"
>> +gdb_test_no_output "set var t2.x = 200"
>> +gdb_test "print t2.x + t2.y + t2.z" " = 2441"
>
> Thanks,
>
> Simon

Thanks,
Andreas
diff mbox

Patch

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 1f89a08..d13c0c5 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1751,6 +1751,15 @@  copy_bitwise_tests (void)
 
 #endif /* GDB_SELF_TEST */
 
+/* Return the number of bytes overlapping a contiguous chunk of N_BITS
+   bits whose first bit is located at bit offset START.  */
+
+static size_t
+bits_to_bytes (ULONGEST start, ULONGEST n_bits)
+{
+  return (start % 8 + n_bits + 7) / 8;
+}
+
 static void
 read_pieced_value (struct value *v)
 {
@@ -1761,7 +1770,6 @@  read_pieced_value (struct value *v)
   struct piece_closure *c
     = (struct piece_closure *) value_computed_closure (v);
   size_t type_len;
-  size_t buffer_size = 0;
   std::vector<gdb_byte> buffer;
   int bits_big_endian
     = gdbarch_bits_big_endian (get_type_arch (value_type (v)));
@@ -1805,13 +1813,9 @@  read_pieced_value (struct value *v)
       if (this_size_bits > type_len - offset)
 	this_size_bits = type_len - offset;
 
-      this_size = (this_size_bits + source_offset_bits % 8 + 7) / 8;
+      this_size = bits_to_bytes (source_offset_bits, this_size_bits);
+      buffer.reserve (this_size);
       source_offset = source_offset_bits / 8;
-      if (buffer_size < this_size)
-	{
-	  buffer_size = this_size;
-	  buffer.reserve (buffer_size);
-	}
       intermediate_buffer = buffer.data ();
 
       /* Copy from the source to DEST_BUFFER.  */
@@ -1822,22 +1826,22 @@  read_pieced_value (struct value *v)
 	    struct frame_info *frame = frame_find_by_id (c->frame_id);
 	    struct gdbarch *arch = get_frame_arch (frame);
 	    int gdb_regnum = dwarf_reg_to_regnum_or_error (arch, p->v.regno);
+	    ULONGEST reg_bits = 8 * register_size (arch, gdb_regnum);
 	    int optim, unavail;
-	    LONGEST reg_offset = source_offset;
 
 	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
-		&& this_size < register_size (arch, gdb_regnum))
+		&& p->size < reg_bits)
 	      {
 		/* 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 += reg_bits - p->size;
 	      }
+	    this_size = bits_to_bytes (source_offset_bits, this_size_bits);
+	    buffer.reserve (this_size);
 
-	    if (!get_frame_register_bytes (frame, gdb_regnum, reg_offset,
-					   this_size, buffer.data (),
-					   &optim, &unavail))
+	    if (!get_frame_register_bytes (frame, gdb_regnum,
+					  source_offset_bits / 8,
+					  this_size, buffer.data (),
+					  &optim, &unavail))
 	      {
 		/* Just so garbage doesn't ever shine through.  */
 		memset (buffer.data (), 0, this_size);
@@ -1847,9 +1851,8 @@  read_pieced_value (struct value *v)
 		if (unavail)
 		  mark_value_bits_unavailable (v, offset, this_size_bits);
 	      }
-
 	    copy_bitwise (contents, dest_offset_bits,
-			  intermediate_buffer, source_offset_bits % 8,
+			  buffer.data (), source_offset_bits % 8,
 			  this_size_bits, bits_big_endian);
 	  }
 	  break;
@@ -1927,7 +1930,6 @@  write_pieced_value (struct value *to, struct value *from)
   struct piece_closure *c
     = (struct piece_closure *) value_computed_closure (to);
   size_t type_len;
-  size_t buffer_size = 0;
   std::vector<gdb_byte> buffer;
   int bits_big_endian
     = gdbarch_bits_big_endian (get_type_arch (value_type (to)));
@@ -1972,7 +1974,7 @@  write_pieced_value (struct value *to, struct value *from)
       if (this_size_bits > type_len - offset)
 	this_size_bits = type_len - offset;
 
-      this_size = (this_size_bits + dest_offset_bits % 8 + 7) / 8;
+      this_size = bits_to_bytes (dest_offset_bits, this_size_bits);
       source_offset = source_offset_bits / 8;
       dest_offset = dest_offset_bits / 8;
       if (dest_offset_bits % 8 == 0 && source_offset_bits % 8 == 0
@@ -1983,11 +1985,7 @@  write_pieced_value (struct value *to, struct value *from)
 	}
       else
 	{
-	  if (buffer_size < this_size)
-	    {
-	      buffer_size = this_size;
-	      buffer.reserve (buffer_size);
-	    }
+	  buffer.reserve (this_size);
 	  source_buffer = buffer.data ();
 	  need_bitwise = 1;
 	}
@@ -1999,20 +1997,24 @@  write_pieced_value (struct value *to, struct value *from)
 	    struct frame_info *frame = frame_find_by_id (c->frame_id);
 	    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;
+	    ULONGEST reg_bits = 8 * register_size (arch, gdb_regnum);
 
 	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
-		&& this_size <= register_size (arch, gdb_regnum))
+		&& p->size <= reg_bits)
 	      {
 		/* Big-endian, and we want less than full size.  */
-		reg_offset = register_size (arch, gdb_regnum) - this_size;
+		dest_offset_bits += reg_bits - p->size;
 	      }
+	    this_size = bits_to_bytes (dest_offset_bits, this_size_bits);
+	    buffer.reserve (this_size);
 
-	    if (need_bitwise)
+	    if (dest_offset_bits % 8 != 0 || this_size_bits % 8 != 0)
 	      {
+		/* Need some bits from original register value.  */
 		int optim, unavail;
 
-		if (!get_frame_register_bytes (frame, gdb_regnum, reg_offset,
+		if (!get_frame_register_bytes (frame, gdb_regnum,
+					       dest_offset_bits / 8,
 					       this_size, buffer.data (),
 					       &optim, &unavail))
 		  {
@@ -2027,14 +2029,14 @@  write_pieced_value (struct value *to, struct value *from)
 				     "bitfield; containing word "
 				     "is unavailable"));
 		  }
-		copy_bitwise (buffer.data (), dest_offset_bits,
-			      contents, source_offset_bits,
-			      this_size_bits,
-			      bits_big_endian);
 	      }
 
-	    put_frame_register_bytes (frame, gdb_regnum, reg_offset, 
-				      this_size, source_buffer);
+	    copy_bitwise (buffer.data (), dest_offset_bits % 8,
+			  contents, source_offset_bits,
+			  this_size_bits, bits_big_endian);
+	    put_frame_register_bytes (frame, gdb_regnum,
+				      dest_offset_bits / 8,
+				      this_size, buffer.data ());
 	  }
 	  break;
 	case DWARF_VALUE_MEMORY:
diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp b/gdb/testsuite/gdb.dwarf2/var-access.exp
index c6abc87..4787dfb 100644
--- a/gdb/testsuite/gdb.dwarf2/var-access.exp
+++ b/gdb/testsuite/gdb.dwarf2/var-access.exp
@@ -192,6 +192,18 @@  Dwarf::assemble $asm_file {
 			piece 1
 		    } SPECIAL_expr}
 		}
+		# Register pieces for bitfield access.
+		DW_TAG_variable {
+		    {name "t2"}
+		    {type :$struct_t_label}
+		    {location {
+			piece 4
+			regx [lindex $dwarf_regnum 0]
+			piece 3
+			regx [lindex $dwarf_regnum 1]
+			piece 1
+		    } SPECIAL_expr}
+		}
 	    }
 	}
     }
@@ -206,6 +218,15 @@  if ![runto_main] {
     return -1
 }
 
+# Determine endianness.
+set endian "little"
+gdb_test_multiple "show endian" "determine endianness" {
+    -re ".* (big|little) endian.*$gdb_prompt $" {
+	set endian $expect_out(1,string)
+	pass "endianness: $endian"
+    }
+}
+
 # Byte-aligned memory pieces.
 gdb_test "print/d s1" " = \\{a = 2, b = 3, c = 0, d = 1\\}" \
     "s1 == re-ordered buf"
@@ -244,3 +265,14 @@  gdb_test_no_output "set var t1.y = 1234"
 gdb_test "print t1" " = \\{u = <optimized out>, x = -7, y = 1234, z = 340\\}" \
     "verify t1"
 
+switch $endian { big {set val 0x7ffc} little {set val 0x3ffe00} }
+gdb_test_no_output "set var \$[lindex $regname 0] = $val" \
+    "init t2, first piece"
+gdb_test_no_output "set var \$[lindex $regname 1] = 0" \
+    "init t2, second piece"
+gdb_test "print/d t2" " = \\{u = <optimized out>, x = 0, y = -1, z = 0\\}" \
+    "initialized t2 from regs"
+gdb_test_no_output "set var t2.y = 2641"
+gdb_test_no_output "set var t2.z = -400"
+gdb_test_no_output "set var t2.x = 200"
+gdb_test "print t2.x + t2.y + t2.z" " = 2441"