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

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

On 2017-04-27 13:54, Andreas Arnez wrote:
> 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.

I did not realize that, but I'm glad you did :).

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

Indeed, it's probably better not to break the symmetry.

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

Yes, it was just brainstorming for future enhancements.

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

Ok, I though it was just some simplification.  The new code is 
equivalent to both branches of the if/else that's being removed for 
dest_offset_bits, this_size_bits and bits_to_skip.  But you're right, 
for source_offset_bits it changes the behaviour.  In any case, I like 
the new code better, less branches :).

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

I think it's worth splitting it*, it will help understand each issue and 
corresponding fix independently.  It's some non-trivial work you're 
doing here, so think about external observers that want to take a look 
at it :). Patches are cheap after all.

*unless it adds to much complexity to have these intermediary states.

Thanks,

Simon