Re: [PATCH 5/9] Fix issues in write_pieced_value when targeting bit-fields

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

On 2017-04-07 13:38, Andreas Arnez wrote:
> There are multiple issues in write_pieced_value when dealing with a
> bit-field as the target value:
> 
> (1) The number of bits preceding the bit-field is calculated without
>     considering the relative offset of the value's parent.

If others are wondering when this can happen:

struct s {
     uint64_t foo;
     struct {
	uint32_t bar;
	uint32_t bf : 10;
     } baz;
};

If "val" is a struct value representing bf:

  - value_offset(val) == 4 (sizeof bar)
  - val->parent represents the whole baz structure
  - value_offset(val->parent) == 8 (sizeof foo)

If bf was a "standard", non-bitfield variable, its offset would be 12 
directly.

There are multiple places that do "value_offset (parent) + value_offset 
(value)" when value is a bitfield.  Isn't it what we want most of the 
time?  If so, I wonder if eventually value_offset shouldn't instead 
return the computed offset, like:

LONGEST
value_offset (const struct value *value)
{
   if (value_bitsize (value))
     return value->offset + value_offset (value_parent (value));
   else
     return value->offset;
}

> (2) On big-endian targets the source value's *most* significant bits 
> are
>     transferred to the target value, instead of its least significant
>     bits.
> 
> (3) The number of bytes containing a portion of the bit-field in a 
> given
>     piece is calculated with the wrong starting offset; thus the result
>     may be off by one.
> 
> (4) When checking whether the data can be transferred byte-wise, the
>     transfer size is not verified to be byte-aligned.
> 
> (5) When transferring the data via a buffer, the bit offset within the
>     target value is not reduced to its sub-byte fraction before using 
> it
>     as a bit offset into the buffer.
> 
> These issues are fixed.  For consistency, the affected logic that 
> exists
> in read_pieced_value as well is changed there in the same way.
> 
> gdb/ChangeLog:
> 
> 	* dwarf2loc.c (write_pieced_value): Fix various issues with
> 	bit-field handling.
> 	(read_pieced_value): Sync with changes in write_pieced_value.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.dwarf2/var-access.exp: Add test for accessing bit-fields
> 	whose containing structure is located in several DWARF pieces.
> ---
>  gdb/dwarf2loc.c                         | 54 
> +++++++++++++++------------------
>  gdb/testsuite/gdb.dwarf2/var-access.exp | 50 
> +++++++++++++++++++++++++++++-
>  2 files changed, 74 insertions(+), 30 deletions(-)
> 
> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index 76a58a3..1f89a08 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c
> @@ -1775,7 +1775,8 @@ read_pieced_value (struct value *v)
>    bits_to_skip = 8 * value_offset (v);
>    if (value_bitsize (v))
>      {
> -      bits_to_skip += value_bitpos (v);
> +      bits_to_skip += (8 * value_offset (value_parent (v))
> +		       + value_bitpos (v));

I guess this is related to (1).

>        type_len = value_bitsize (v);
>      }
>    else
> @@ -1796,18 +1797,11 @@ read_pieced_value (struct value *v)
>  	  bits_to_skip -= this_size_bits;
>  	  continue;
>  	}
> -      if (bits_to_skip > 0)
> -	{
> -	  dest_offset_bits = 0;
> -	  source_offset_bits = bits_to_skip;
> -	  this_size_bits -= bits_to_skip;
> -	  bits_to_skip = 0;
> -	}
> -      else
> -	{
> -	  dest_offset_bits = offset;
> -	  source_offset_bits = 0;
> -	}
> +      source_offset_bits = bits_to_skip;
> +      this_size_bits -= bits_to_skip;
> +      bits_to_skip = 0;
> +      dest_offset_bits = offset;
> +

Is this snippet related to one of the problems you have described?  It 
seems to me like it's just simplifying the code, but it's not changing 
the behavior.  If that's the case, I'd suggest putting it in its own 
patch (along with its equivalent in write_pieced_value).

>        if (this_size_bits > type_len - offset)
>  	this_size_bits = type_len - offset;
> 
> @@ -1942,8 +1936,16 @@ write_pieced_value (struct value *to, struct 
> value *from)
>    bits_to_skip = 8 * value_offset (to);
>    if (value_bitsize (to))
>      {
> -      bits_to_skip += value_bitpos (to);
> +      bits_to_skip += (8 * value_offset (value_parent (to))
> +		       + value_bitpos (to));
>        type_len = value_bitsize (to);
> +      /* Use the least significant bits of FROM.  */
> +      if (gdbarch_byte_order (get_type_arch (value_type (from)))
> +	  == BFD_ENDIAN_BIG)
> +	{
> +	  offset = 8 * TYPE_LENGTH (value_type (from)) - type_len;
> +	  type_len += offset;
> +	}

I guess this is related to (1) and (2).

>      }
>    else
>      type_len = 8 * TYPE_LENGTH (value_type (to));
> @@ -1962,25 +1964,19 @@ write_pieced_value (struct value *to, struct
> value *from)
>  	  bits_to_skip -= this_size_bits;
>  	  continue;
>  	}
> -      if (bits_to_skip > 0)
> -	{
> -	  dest_offset_bits = bits_to_skip;
> -	  source_offset_bits = 0;
> -	  this_size_bits -= bits_to_skip;
> -	  bits_to_skip = 0;
> -	}
> -      else
> -	{
> -	  dest_offset_bits = 0;
> -	  source_offset_bits = offset;
> -	}
> +      dest_offset_bits = bits_to_skip;
> +      this_size_bits -= bits_to_skip;
> +      bits_to_skip = 0;
> +      source_offset_bits = offset;
> +
>        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 = (this_size_bits + dest_offset_bits % 8 + 7) / 8;

I guess this is related to (3).

>        source_offset = source_offset_bits / 8;
>        dest_offset = dest_offset_bits / 8;
> -      if (dest_offset_bits % 8 == 0 && source_offset_bits % 8 == 0)
> +      if (dest_offset_bits % 8 == 0 && source_offset_bits % 8 == 0
> +	  && this_size_bits % 8 == 0)

... and this to (4).

>  	{
>  	  source_buffer = contents + source_offset;
>  	  need_bitwise = 0;
> @@ -2049,7 +2045,7 @@ write_pieced_value (struct value *to, struct 
> value *from)
>  	      read_memory (p->v.mem.addr + dest_offset, buffer.data (), 1);
>  	      read_memory (p->v.mem.addr + dest_offset + this_size - 1,
>  			   &buffer[this_size - 1], 1);
> -	      copy_bitwise (buffer.data (), dest_offset_bits,
> +	      copy_bitwise (buffer.data (), dest_offset_bits % 8,

... and this to (5).

>  			    contents, source_offset_bits,
>  			    this_size_bits,
>  			    bits_big_endian);
> diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp
> b/gdb/testsuite/gdb.dwarf2/var-access.exp
> index 56a635a..c6abc87 100644
> --- a/gdb/testsuite/gdb.dwarf2/var-access.exp
> +++ b/gdb/testsuite/gdb.dwarf2/var-access.exp
> @@ -62,7 +62,7 @@ Dwarf::assemble $asm_file {
>  	} {
>  	    declare_labels char_type_label
>  	    declare_labels int_type_label short_type_label
> -	    declare_labels array_a8_label struct_s_label
> +	    declare_labels array_a8_label struct_s_label struct_t_label
> 
>  	    char_type_label: base_type {
>  		{name "char"}
> @@ -111,6 +111,34 @@ Dwarf::assemble $asm_file {
>  		}
>  	    }
> 
> +	    struct_t_label: structure_type {
> +		{name "t"}
> +		{byte_size 8 DW_FORM_sdata}
> +	    } {
> +		member {
> +		    {name u}
> +		    {type :$int_type_label}
> +		}
> +		member {
> +		    {name x}
> +		    {type :$int_type_label}
> +		    {data_member_location 4 DW_FORM_udata}
> +		    {bit_size 9 DW_FORM_udata}
> +		}
> +		member {
> +		    {name y}
> +		    {type :$int_type_label}
> +		    {data_bit_offset 41 DW_FORM_udata}
> +		    {bit_size 13 DW_FORM_udata}
> +		}
> +		member {
> +		    {name z}
> +		    {type :$int_type_label}
> +		    {data_bit_offset 54 DW_FORM_udata}
> +		    {bit_size 10 DW_FORM_udata}
> +		}
> +	    }
> +
>  	    DW_TAG_subprogram {
>  		{name "main"}
>  		{DW_AT_external 1 flag}
> @@ -152,6 +180,18 @@ Dwarf::assemble $asm_file {
>  			piece 1
>  		    } SPECIAL_expr}
>  		}
> +		# Memory pieces for bitfield access.
> +		DW_TAG_variable {
> +		    {name "t1"}
> +		    {type :$struct_t_label}
> +		    {location {
> +			piece 4
> +			addr "$buf_var + 1"
> +			piece 3
> +			addr "$buf_var"
> +			piece 1
> +		    } SPECIAL_expr}
> +		}
>  	    }
>  	}
>      }
> @@ -196,3 +236,11 @@ gdb_test_no_output "set var s2 = {191, 73, 231, 
> 123}" \
>      "re-initialize s2"
>  gdb_test "print/d s2"  " = \\{a = 191, b = 73, c = 231, d = 123\\}" \
>      "verify re-initialized s2"
> +
> +# Unaligned bitfield access through byte-aligned pieces.
> +gdb_test_no_output "set var t1.x = -7"
> +gdb_test_no_output "set var t1.z = 340"
> +gdb_test_no_output "set var t1.y = 1234"
> +gdb_test "print t1" " = \\{u = <optimized out>, x = -7, y = 1234, z = 
> 340\\}" \
> +    "verify t1"

Maybe I'm missing something, but if there's the same bug in the write 
and read implementations, it's possible it would slip through, isn't it? 
  For example, if the "set var" part messes up the bit ordering and the 
"print" part messes it up the same way, it would appear correct when 
it's not.  Reading actual data from a progam won't work.

In the other tests, you tested against known data already in memory, 
which made sense I think.

Thanks,

Simon