From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 35010 invoked by alias); 27 Apr 2017 17:54:50 -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 34998 invoked by uid 89); 27 Apr 2017 17:54:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy=slip, transferring, fraction, honest X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0a-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.156.1) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 27 Apr 2017 17:54:46 +0000 Received: from pps.filterd (m0098394.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v3RHrY34095605 for ; Thu, 27 Apr 2017 13:54:47 -0400 Received: from e06smtp15.uk.ibm.com (e06smtp15.uk.ibm.com [195.75.94.111]) by mx0a-001b2d01.pphosted.com with ESMTP id 2a3jbd12j1-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 27 Apr 2017 13:54:47 -0400 Received: from localhost by e06smtp15.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 27 Apr 2017 18:54:45 +0100 Received: from b06cxnps3074.portsmouth.uk.ibm.com (9.149.109.194) by e06smtp15.uk.ibm.com (192.168.101.145) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Thu, 27 Apr 2017 18:54:43 +0100 Received: from d06av26.portsmouth.uk.ibm.com (d06av26.portsmouth.uk.ibm.com [9.149.105.62]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v3RHsg6F7733748; Thu, 27 Apr 2017 17:54:42 GMT Received: from d06av26.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 5B3F2AE045; Thu, 27 Apr 2017 18:53:17 +0100 (BST) Received: from d06av26.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 2BEC0AE051; Thu, 27 Apr 2017 18:53:17 +0100 (BST) Received: from oc1027705133.ibm.com (unknown [9.152.212.162]) by d06av26.portsmouth.uk.ibm.com (Postfix) with ESMTPS; Thu, 27 Apr 2017 18:53:17 +0100 (BST) From: Andreas Arnez To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 5/9] Fix issues in write_pieced_value when targeting bit-fields References: <1491586736-21296-1-git-send-email-arnez@linux.vnet.ibm.com> <1491586736-21296-6-git-send-email-arnez@linux.vnet.ibm.com> Date: Thu, 27 Apr 2017 17:54:00 -0000 User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-TM-AS-GCONF: 00 x-cbid: 17042717-0020-0000-0000-00000354E7DF X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17042717-0021-0000-0000-0000417AC46D Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-04-27_16:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1704270291 X-IsSubscribed: yes X-SW-Source: 2017-04/txt/msg00748.txt.bz2 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 = , 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