From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 75777 invoked by alias); 14 Apr 2017 05:18:14 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 69421 invoked by uid 89); 14 Apr 2017 05:16:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-23.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy= X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 14 Apr 2017 05:16:40 +0000 Received: by simark.ca (Postfix, from userid 33) id 1F8F61E4A2; Fri, 14 Apr 2017 01:16:40 -0400 (EDT) To: Andreas Arnez Subject: Re: [PATCH 5/9] Fix issues in write_pieced_value when targeting bit-fields X-PHP-Originating-Script: 33:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Fri, 14 Apr 2017 05:18:00 -0000 From: Simon Marchi Cc: gdb-patches@sourceware.org In-Reply-To: <1491586736-21296-6-git-send-email-arnez@linux.vnet.ibm.com> References: <1491586736-21296-1-git-send-email-arnez@linux.vnet.ibm.com> <1491586736-21296-6-git-send-email-arnez@linux.vnet.ibm.com> Message-ID: X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.4 X-IsSubscribed: yes X-SW-Source: 2017-04/txt/msg00460.txt.bz2 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 = , 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