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

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

On Fri, Apr 14 2017, Simon Marchi wrote:

> 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.

Right.  Now that you mention it, I realize that the test case doesn't
cover this yet.  So I'm enhancing it.

>
> 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;
> }

Maybe, but what would set_value_offset do then?

To be honest, I don't completely understand the definition of
value_offset, so I decided not to change anything there with this patch.
Maybe we should spend some time refactoring the value API, but I think
this is a separate task.

>
>> (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).

Right.

>
>>        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).

This is just to mirror the change in write_pieced_value.  See below for
the rationale of that.

>
>>        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).

Right.  Note that 'offset' may now be nonzero upon entering the loop.

>
>>      }
>>    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;
>> +

This is related to (2).  The old version assumed that 'offset' is still
zero when there are bits to skip, so a literal zero (instead of
'offset') could be copied into source_offset_bits.  This assumption
doesn't hold any longer, now that 'offset' may start with a nonzero
value.

>>        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).

Yes.  Note that this looks like a classical copy & paste error.  The
line was probably copied from read_pieced_value, where
'source_offset_bits' is actually correct.

>
>>        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).

Correct.

>
>>  	{
>>  	  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).

Also correct.

As you can see, the patch fixes 5 different, fairly independent bugs.
Thus it could have been split into 5 patches, but I found that a bit
extreme...

>
>>  			    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.

Yeah, OK, I can do that.

--
Andreas