| Message ID | 1491586736-21296-6-git-send-email-arnez@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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)); 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); 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" +
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(-)