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