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

[Simon Marchi <simon.marchi@polymtl.ca>]

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