[5/9] Fix issues in write_pieced_value when targeting bit-fields
Commit Message
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.
(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(-)
Comments
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
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
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
@@ -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));
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;
+
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;
+ }
}
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;
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)
{
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,
contents, source_offset_bits,
this_size_bits,
bits_big_endian);
@@ -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"
+