This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 5/9] Fix issues in write_pieced_value when targeting bit-fields
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