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

Simon Marchi
2017-05-03T13:59:34Z

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