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

[Andreas Arnez <arnez@linux.vnet.ibm.com>]

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