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

Simon Marchi
2017-04-14T14:11:04Z

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