From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 54982 invoked by alias); 14 Apr 2017 14:11:04 -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 54948 invoked by uid 89); 14 Apr 2017 14:11:02 -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=1100, 0011, yield, adjusted 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 14:10:59 +0000 Received: by simark.ca (Postfix, from userid 33) id E732B1E4A3; Fri, 14 Apr 2017 10:10:59 -0400 (EDT) To: Andreas Arnez Subject: Re: [PATCH 6/9] Fix handling of DWARF register pieces on big-endian targets 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 14:11:00 -0000 From: Simon Marchi Cc: gdb-patches@sourceware.org In-Reply-To: <1491586736-21296-7-git-send-email-arnez@linux.vnet.ibm.com> References: <1491586736-21296-1-git-send-email-arnez@linux.vnet.ibm.com> <1491586736-21296-7-git-send-email-arnez@linux.vnet.ibm.com> Message-ID: <9a0ec20dcede60f09ee38c46b2f2ae9f@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.4 X-IsSubscribed: yes X-SW-Source: 2017-04/txt/msg00461.txt.bz2 On 2017-04-07 13:38, Andreas Arnez wrote: > For big-endian targets the logic in read/write_pieced_value tries to > take > a register piece from the LSB end. This requires offsets and sizes to > be > adjusted accordingly, and that's where the current implementation has > some > issues: > > * The formulas for recalculating the bit- and byte-offsets into the > register are wrong. They just happen to yield correct results if > everything is byte-aligned and the piece's last byte belongs to the > given value. > > * After recalculating the bit offset into the register, the number of > bytes to be copied from the register is not recalculated. Of course > this does not matter if everything (particularly the piece size) is > byte-aligned. > > These issues are fixed. The size calculation is performed with a new > helper function bits_to_bytes(). For simplification, the variable > buffer_size is dropped. > > gdb/ChangeLog: > > * dwarf2loc.c (bits_to_bytes): New function. > (read_pieced_value): Fix offset calculations for register pieces > on big-endian targets. > (write_pieced_value): Likewise. > > gdb/testsuite/ChangeLog: > > * gdb.dwarf2/var-access.exp: Add test for non-byte-aligned > register pieces. > --- > gdb/dwarf2loc.c | 74 > +++++++++++++++++---------------- > gdb/testsuite/gdb.dwarf2/var-access.exp | 32 ++++++++++++++ > 2 files changed, 70 insertions(+), 36 deletions(-) > > diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c > index 1f89a08..d13c0c5 100644 > --- a/gdb/dwarf2loc.c > +++ b/gdb/dwarf2loc.c > @@ -1751,6 +1751,15 @@ copy_bitwise_tests (void) > > #endif /* GDB_SELF_TEST */ > > +/* Return the number of bytes overlapping a contiguous chunk of N_BITS > + bits whose first bit is located at bit offset START. */ > + > +static size_t > +bits_to_bytes (ULONGEST start, ULONGEST n_bits) > +{ > + return (start % 8 + n_bits + 7) / 8; > +} > + > static void > read_pieced_value (struct value *v) > { > @@ -1761,7 +1770,6 @@ read_pieced_value (struct value *v) > struct piece_closure *c > = (struct piece_closure *) value_computed_closure (v); > size_t type_len; > - size_t buffer_size = 0; > std::vector buffer; > int bits_big_endian > = gdbarch_bits_big_endian (get_type_arch (value_type (v))); > @@ -1805,13 +1813,9 @@ read_pieced_value (struct value *v) > 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 = bits_to_bytes (source_offset_bits, this_size_bits); > + buffer.reserve (this_size); > source_offset = source_offset_bits / 8; > - if (buffer_size < this_size) > - { > - buffer_size = this_size; > - buffer.reserve (buffer_size); > - } The removal of buffer_size and this "if" (and equivalent in write_pieced_value) seems to be unrelated to the problem you are fixing, but rather a simplification that could be done anyway. If that's the case, you should bundle it with the similar changes from the previous patch, in a patch that only does some refactoring but not behavior changes. > intermediate_buffer = buffer.data (); > > /* Copy from the source to DEST_BUFFER. */ > @@ -1822,22 +1826,22 @@ read_pieced_value (struct value *v) > struct frame_info *frame = frame_find_by_id (c->frame_id); > struct gdbarch *arch = get_frame_arch (frame); > int gdb_regnum = dwarf_reg_to_regnum_or_error (arch, p->v.regno); > + ULONGEST reg_bits = 8 * register_size (arch, gdb_regnum); > int optim, unavail; > - LONGEST reg_offset = source_offset; > > if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG > - && this_size < register_size (arch, gdb_regnum)) > + && p->size < reg_bits) > { > /* Big-endian, and we want less than full size. */ > - reg_offset = register_size (arch, gdb_regnum) - this_size; > - /* We want the lower-order THIS_SIZE_BITS of the bytes > - we extract from the register. */ > - source_offset_bits += 8 * this_size - this_size_bits; > + source_offset_bits += reg_bits - p->size; > } > + this_size = bits_to_bytes (source_offset_bits, this_size_bits); > + buffer.reserve (this_size); > > - if (!get_frame_register_bytes (frame, gdb_regnum, reg_offset, > - this_size, buffer.data (), > - &optim, &unavail)) > + if (!get_frame_register_bytes (frame, gdb_regnum, > + source_offset_bits / 8, > + this_size, buffer.data (), > + &optim, &unavail)) > { > /* Just so garbage doesn't ever shine through. */ > memset (buffer.data (), 0, this_size); > @@ -1847,9 +1851,8 @@ read_pieced_value (struct value *v) > if (unavail) > mark_value_bits_unavailable (v, offset, this_size_bits); > } > - > copy_bitwise (contents, dest_offset_bits, > - intermediate_buffer, source_offset_bits % 8, > + buffer.data (), source_offset_bits % 8, > this_size_bits, bits_big_endian); > } > break; > @@ -1927,7 +1930,6 @@ write_pieced_value (struct value *to, struct > value *from) > struct piece_closure *c > = (struct piece_closure *) value_computed_closure (to); > size_t type_len; > - size_t buffer_size = 0; > std::vector buffer; > int bits_big_endian > = gdbarch_bits_big_endian (get_type_arch (value_type (to))); > @@ -1972,7 +1974,7 @@ write_pieced_value (struct value *to, struct > value *from) > if (this_size_bits > type_len - offset) > this_size_bits = type_len - offset; > > - this_size = (this_size_bits + dest_offset_bits % 8 + 7) / 8; > + this_size = bits_to_bytes (dest_offset_bits, this_size_bits); > source_offset = source_offset_bits / 8; > dest_offset = dest_offset_bits / 8; > if (dest_offset_bits % 8 == 0 && source_offset_bits % 8 == 0 > @@ -1983,11 +1985,7 @@ write_pieced_value (struct value *to, struct > value *from) > } > else > { > - if (buffer_size < this_size) > - { > - buffer_size = this_size; > - buffer.reserve (buffer_size); > - } > + buffer.reserve (this_size); > source_buffer = buffer.data (); > need_bitwise = 1; > } > @@ -1999,20 +1997,24 @@ write_pieced_value (struct value *to, struct > value *from) > struct frame_info *frame = frame_find_by_id (c->frame_id); > struct gdbarch *arch = get_frame_arch (frame); > int gdb_regnum = dwarf_reg_to_regnum_or_error (arch, p->v.regno); > - int reg_offset = dest_offset; > + ULONGEST reg_bits = 8 * register_size (arch, gdb_regnum); > > if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG > - && this_size <= register_size (arch, gdb_regnum)) > + && p->size <= reg_bits) > { > /* Big-endian, and we want less than full size. */ > - reg_offset = register_size (arch, gdb_regnum) - this_size; > + dest_offset_bits += reg_bits - p->size; > } > + this_size = bits_to_bytes (dest_offset_bits, this_size_bits); > + buffer.reserve (this_size); > > - if (need_bitwise) > + if (dest_offset_bits % 8 != 0 || this_size_bits % 8 != 0) I thought the "need_bitwise" variable name helped to understand. To compensate, could you add a comment explaining the "why" of that if? > { > + /* Need some bits from original register value. */ > int optim, unavail; > > - if (!get_frame_register_bytes (frame, gdb_regnum, reg_offset, > + if (!get_frame_register_bytes (frame, gdb_regnum, > + dest_offset_bits / 8, > this_size, buffer.data (), > &optim, &unavail)) > { > @@ -2027,14 +2029,14 @@ write_pieced_value (struct value *to, struct > value *from) > "bitfield; containing word " > "is unavailable")); > } > - copy_bitwise (buffer.data (), dest_offset_bits, > - contents, source_offset_bits, > - this_size_bits, > - bits_big_endian); > } > > - put_frame_register_bytes (frame, gdb_regnum, reg_offset, > - this_size, source_buffer); > + copy_bitwise (buffer.data (), dest_offset_bits % 8, > + contents, source_offset_bits, > + this_size_bits, bits_big_endian); > + put_frame_register_bytes (frame, gdb_regnum, > + dest_offset_bits / 8, > + this_size, buffer.data ()); > } > break; > case DWARF_VALUE_MEMORY: > diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp > b/gdb/testsuite/gdb.dwarf2/var-access.exp > index c6abc87..4787dfb 100644 > --- a/gdb/testsuite/gdb.dwarf2/var-access.exp > +++ b/gdb/testsuite/gdb.dwarf2/var-access.exp > @@ -192,6 +192,18 @@ Dwarf::assemble $asm_file { > piece 1 > } SPECIAL_expr} > } > + # Register pieces for bitfield access. > + DW_TAG_variable { > + {name "t2"} > + {type :$struct_t_label} > + {location { > + piece 4 > + regx [lindex $dwarf_regnum 0] > + piece 3 > + regx [lindex $dwarf_regnum 1] > + piece 1 > + } SPECIAL_expr} > + } > } > } > } > @@ -206,6 +218,15 @@ if ![runto_main] { > return -1 > } > > +# Determine endianness. > +set endian "little" > +gdb_test_multiple "show endian" "determine endianness" { > + -re ".* (big|little) endian.*$gdb_prompt $" { > + set endian $expect_out(1,string) > + pass "endianness: $endian" > + } > +} "pass" should be passed the test name, which should be stable as much as possible. The typical way to do it would be: # Determine endianness. set endian "little" set test "determine endianness" gdb_test_multiple "show endian" $test { -re ".* (big|little) endian.*$gdb_prompt $" { set endian $expect_out(1,string) pass $test } } > + > # Byte-aligned memory pieces. > gdb_test "print/d s1" " = \\{a = 2, b = 3, c = 0, d = 1\\}" \ > "s1 == re-ordered buf" > @@ -244,3 +265,14 @@ gdb_test_no_output "set var t1.y = 1234" > gdb_test "print t1" " = \\{u = , x = -7, y = 1234, z = > 340\\}" \ > "verify t1" > > +switch $endian { big {set val 0x7ffc} little {set val 0x3ffe00} } I had to draw this in order to understand, so it might be useful to others. For little endian (assuming the register is 32 bits): 3 f f e 0 0 xxxx xxxx 0011 1111 1111 1110 0000 0000 ^^ ^^^^ ^^^^ | ^^ ^^^^ ^^^^ ^^^ ` x | `- y `- part of z For big endian (assuming the register is 32 bits): 0 0 7 f f c xxxx xxxx 0000 0000 0111 1111 1111 1100 ^^^^ ^^^^ ^ ^^ `- x ^^^ ^^^^ ^^^^ ^^ `- part of z `- y Not having worked with a big endian machine before, I'm still confused in how bits are aligned in the register and which bit means what. > +gdb_test_no_output "set var \$[lindex $regname 0] = $val" \ > + "init t2, first piece" > +gdb_test_no_output "set var \$[lindex $regname 1] = 0" \ > + "init t2, second piece" > +gdb_test "print/d t2" " = \\{u = , x = 0, y = -1, z = > 0\\}" \ > + "initialized t2 from regs" > +gdb_test_no_output "set var t2.y = 2641" > +gdb_test_no_output "set var t2.z = -400" > +gdb_test_no_output "set var t2.x = 200" > +gdb_test "print t2.x + t2.y + t2.z" " = 2441" Thanks, Simon