Message ID | 1491586736-21296-3-git-send-email-arnez@linux.vnet.ibm.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 52404 invoked by alias); 7 Apr 2017 17:40:26 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 52384 invoked by uid 89); 7 Apr 2017 17:40:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.2 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=composed, accounting, Hx-languages-length:2519, bringing 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; Fri, 07 Apr 2017 17:40:24 +0000 Received: from pps.filterd (m0098399.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v37HcYfH101141 for <gdb-patches@sourceware.org>; Fri, 7 Apr 2017 13:40:24 -0400 Received: from e06smtp12.uk.ibm.com (e06smtp12.uk.ibm.com [195.75.94.108]) by mx0a-001b2d01.pphosted.com with ESMTP id 29paefh3m5-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for <gdb-patches@sourceware.org>; Fri, 07 Apr 2017 13:40:23 -0400 Received: from localhost by e06smtp12.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for <gdb-patches@sourceware.org> from <arnez@linux.vnet.ibm.com>; Fri, 7 Apr 2017 18:40:21 +0100 Received: from b06cxnps3075.portsmouth.uk.ibm.com (9.149.109.195) by e06smtp12.uk.ibm.com (192.168.101.142) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Fri, 7 Apr 2017 18:40:20 +0100 Received: from d06av25.portsmouth.uk.ibm.com (d06av25.portsmouth.uk.ibm.com [9.149.105.61]) by b06cxnps3075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v37HeJNh8782104 for <gdb-patches@sourceware.org>; Fri, 7 Apr 2017 17:40:19 GMT Received: from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id CB81711C04A for <gdb-patches@sourceware.org>; Fri, 7 Apr 2017 18:39:40 +0100 (BST) Received: from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id A7AC311C052 for <gdb-patches@sourceware.org>; Fri, 7 Apr 2017 18:39:40 +0100 (BST) Received: from oc1027705133.ibm.com (unknown [9.152.212.162]) by d06av25.portsmouth.uk.ibm.com (Postfix) with ESMTP for <gdb-patches@sourceware.org>; Fri, 7 Apr 2017 18:39:40 +0100 (BST) From: Andreas Arnez <arnez@linux.vnet.ibm.com> To: gdb-patches@sourceware.org Subject: [PATCH 2/9] Fix size capping in write_pieced_value Date: Fri, 7 Apr 2017 19:38:09 +0200 In-Reply-To: <1491586736-21296-1-git-send-email-arnez@linux.vnet.ibm.com> References: <1491586736-21296-1-git-send-email-arnez@linux.vnet.ibm.com> X-TM-AS-GCONF: 00 x-cbid: 17040717-0008-0000-0000-0000041FA113 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17040717-0009-0000-0000-00001D2B87E4 Message-Id: <1491586736-21296-3-git-send-email-arnez@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2017-04-07_15:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=1 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1702020001 definitions=main-1704070144 X-IsSubscribed: yes |
Commit Message
Andreas Arnez
April 7, 2017, 5:38 p.m. UTC
A field in a structure composed of DWARF pieces overlaps one or more of those pieces. When writing to the field, the beginning of the first and the end of the last of those pieces may have to be skipped. But the logic in write_pieced_value for handling this is flawed when there are actually bits to skip at the beginning of the first piece: it truncates the piece size towards the end *before* accounting for the skipped bits at the beginning instead of the other way around. Note that the same bug was already found in read_pieced_value and fixed there (but not in write_pieced_value), see PR 15391. This patch swaps the calculations, bringing them into the same (correct) order as in read_pieced_value. gdb/ChangeLog: * dwarf2loc.c (write_pieced_value): Fix order of calculations for size capping. gdb/testsuite/ChangeLog: * gdb.dwarf2/var-pieces.exp: Add test case for modifying a variable at nonzero offset. --- gdb/dwarf2loc.c | 4 ++-- gdb/testsuite/gdb.dwarf2/var-access.exp | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-)
Comments
Andreas Arnez <arnez@linux.vnet.ibm.com> writes: > A field in a structure composed of DWARF pieces overlaps one or more of > those pieces. When writing to the field, the beginning of the first and > the end of the last of those pieces may have to be skipped. But the Do you have an example? > logic in write_pieced_value for handling this is flawed when there are > actually bits to skip at the beginning of the first piece: it truncates > the piece size towards the end *before* accounting for the skipped bits > at the beginning instead of the other way around. > > Note that the same bug was already found in read_pieced_value and fixed > there (but not in write_pieced_value), see PR 15391. Can we share the code in write_pieced_value and read_pieced_value? The code computing offsets and bits should be shared. Also, we need more comments in the code to explain these offsets and bits, a diagram about the relationships of these bits and offsets is quite helpful.
On Thu, Apr 13 2017, Yao Qi wrote: > Andreas Arnez <arnez@linux.vnet.ibm.com> writes: > >> A field in a structure composed of DWARF pieces overlaps one or more of >> those pieces. When writing to the field, the beginning of the first and >> the end of the last of those pieces may have to be skipped. But the > > Do you have an example? OK, I guess the commit message should be improved a bit. How about this? A field f in a structure composed of DWARF pieces may be located in multiple pieces, where the first and last of those may contain bits from other fields as well. So when writing to f, the beginning of the first and the end of the last of those pieces may have to be skipped. But the logic in write_pieced_value for handling one of those pieces is flawed when the first and last piece are the same, i.e., f is contained in a single piece: < - - - - - - - - - piece_size - - - - - - - - - -> +-------------------------------------------------+ | skipped_bits | f_bits | / / / / / / / / / / | +-------------------------------------------------+ The current logic determines the size of the sub-piece to operate on by limiting the piece size to the bit size of f and then subtracting the skipped bits: max (piece_size, f_bits) - skipped_bits Instead of: max (piece_size - skipped_bits, f_bits) So the resulting sub-piece size is corrupted, leading to wrong handling of this piece in write_pieced_value. > >> logic in write_pieced_value for handling this is flawed when there are >> actually bits to skip at the beginning of the first piece: it truncates >> the piece size towards the end *before* accounting for the skipped bits >> at the beginning instead of the other way around. >> >> Note that the same bug was already found in read_pieced_value and fixed >> there (but not in write_pieced_value), see PR 15391. > > Can we share the code in write_pieced_value and read_pieced_value? The > code computing offsets and bits should be shared. Yes. I have another patch (not posted yet) that merges these two functions. I moved that towards the end of the patch series, so the individual fixes can be incremental. > Also, we need more comments in the code to explain these offsets and > bits, a diagram about the relationships of these bits and offsets is > quite helpful. OK. Some of the offset variables are removed by my patches, so I guess I'll postpone that to the merged version. I'll see what I can come up with and include it in v2. -- Andreas
Andreas Arnez <arnez@linux.vnet.ibm.com> writes: > OK, I guess the commit message should be improved a bit. How about > this? > Hi Andreas, The description to the logic can go to comments, so that we don't need to do "git blame/log" to understand the code. > A field f in a structure composed of DWARF pieces may be located in > multiple pieces, where the first and last of those may contain bits > from other fields as well. So when writing to f, the beginning of the > first and the end of the last of those pieces may have to be skipped. > But the logic in write_pieced_value for handling one of those pieces > is flawed when the first and last piece are the same, i.e., f is > contained in a single piece: > > < - - - - - - - - - piece_size - - - - - - - - - -> > +-------------------------------------------------+ > | skipped_bits | f_bits | / / / / / / / / / / | > +-------------------------------------------------+ > > The current logic determines the size of the sub-piece to operate on > by limiting the piece size to the bit size of f and then subtracting > the skipped bits: > > max (piece_size, f_bits) - skipped_bits > > Instead of: > > max (piece_size - skipped_bits, f_bits) > Given this example, the result is the same, which is "piece_size - skipped_bits", am I missing something? > So the resulting sub-piece size is corrupted, leading to wrong > handling of this piece in write_pieced_value. > >> >>> logic in write_pieced_value for handling this is flawed when there are >>> actually bits to skip at the beginning of the first piece: it truncates >>> the piece size towards the end *before* accounting for the skipped bits >>> at the beginning instead of the other way around. >>> >>> Note that the same bug was already found in read_pieced_value and fixed >>> there (but not in write_pieced_value), see PR 15391. >> >> Can we share the code in write_pieced_value and read_pieced_value? The >> code computing offsets and bits should be shared. > > Yes. I have another patch (not posted yet) that merges these two > functions. I moved that towards the end of the patch series, so the > individual fixes can be incremental. > I'd like to merge the code first, then don't need to fix the same problem in two functions read_pieced_value and write_pieced_value (your patch 4/9 ~ 9/9 touches both two functions). >> Also, we need more comments in the code to explain these offsets and >> bits, a diagram about the relationships of these bits and offsets is >> quite helpful. > > OK. Some of the offset variables are removed by my patches, so I guess > I'll postpone that to the merged version. I'll see what I can come up > with and include it in v2. Please include it in V2.
On Wed, Apr 19 2017, Yao Qi wrote: > Andreas Arnez <arnez@linux.vnet.ibm.com> writes: > >> OK, I guess the commit message should be improved a bit. How about >> this? >> > > Hi Andreas, > The description to the logic can go to comments, so that we don't need > to do "git blame/log" to understand the code. Right, I'll add some general explanation and a diagram about the various bits and offsets (as requested below). However, most of the commit message explains a specific bug in a piece of code that won't exist any more. This aspect doesn't make sense to be included in the comments, I think. > >> A field f in a structure composed of DWARF pieces may be located in >> multiple pieces, where the first and last of those may contain bits >> from other fields as well. So when writing to f, the beginning of the >> first and the end of the last of those pieces may have to be skipped. >> But the logic in write_pieced_value for handling one of those pieces >> is flawed when the first and last piece are the same, i.e., f is >> contained in a single piece: >> >> < - - - - - - - - - piece_size - - - - - - - - - -> >> +-------------------------------------------------+ >> | skipped_bits | f_bits | / / / / / / / / / / | >> +-------------------------------------------------+ >> >> The current logic determines the size of the sub-piece to operate on >> by limiting the piece size to the bit size of f and then subtracting >> the skipped bits: >> >> max (piece_size, f_bits) - skipped_bits >> >> Instead of: >> >> max (piece_size - skipped_bits, f_bits) >> > > Given this example, the result is the same, which is > "piece_size - skipped_bits", am I missing something? Argh, the "max" above is obviously meant to be "min". Sorry for the confusion. > >> So the resulting sub-piece size is corrupted, leading to wrong >> handling of this piece in write_pieced_value. >> >>> >>>> logic in write_pieced_value for handling this is flawed when there are >>>> actually bits to skip at the beginning of the first piece: it truncates >>>> the piece size towards the end *before* accounting for the skipped bits >>>> at the beginning instead of the other way around. >>>> >>>> Note that the same bug was already found in read_pieced_value and fixed >>>> there (but not in write_pieced_value), see PR 15391. >>> >>> Can we share the code in write_pieced_value and read_pieced_value? The >>> code computing offsets and bits should be shared. >> >> Yes. I have another patch (not posted yet) that merges these two >> functions. I moved that towards the end of the patch series, so the >> individual fixes can be incremental. >> > > I'd like to merge the code first, then don't need to fix the same > problem in two functions read_pieced_value and write_pieced_value (your > patch 4/9 ~ 9/9 touches both two functions). Not sure I understand. Do you mean to merge the functions first while preserving existing logic, including all the bugs and differences? I had started along this path and gave up on it, because I found it too complicated. From that attempt I've concluded that the current approach is much less error-prone and easier to follow. > >>> Also, we need more comments in the code to explain these offsets and >>> bits, a diagram about the relationships of these bits and offsets is >>> quite helpful. >> >> OK. Some of the offset variables are removed by my patches, so I guess >> I'll postpone that to the merged version. I'll see what I can come up >> with and include it in v2. > > Please include it in V2. Sure. -- Andreas
Andreas Arnez <arnez@linux.vnet.ibm.com> writes: >> Hi Andreas, >> The description to the logic can go to comments, so that we don't need >> to do "git blame/log" to understand the code. > > Right, I'll add some general explanation and a diagram about the various > bits and offsets (as requested below). > > However, most of the commit message explains a specific bug in a piece > of code that won't exist any more. This aspect doesn't make sense to be > included in the comments, I think. > OK, no problem. >>>>> logic in write_pieced_value for handling this is flawed when there are >>>>> actually bits to skip at the beginning of the first piece: it truncates >>>>> the piece size towards the end *before* accounting for the skipped bits >>>>> at the beginning instead of the other way around. >>>>> >>>>> Note that the same bug was already found in read_pieced_value and fixed >>>>> there (but not in write_pieced_value), see PR 15391. >>>> >>>> Can we share the code in write_pieced_value and read_pieced_value? The >>>> code computing offsets and bits should be shared. >>> >>> Yes. I have another patch (not posted yet) that merges these two >>> functions. I moved that towards the end of the patch series, so the >>> individual fixes can be incremental. >>> >> >> I'd like to merge the code first, then don't need to fix the same >> problem in two functions read_pieced_value and write_pieced_value (your >> patch 4/9 ~ 9/9 touches both two functions). > > Not sure I understand. Do you mean to merge the functions first while > preserving existing logic, including all the bugs and differences? I What I meant is that 1) make two parts identical (but not introducing new bugs) 2) merge the code to a single function, 3) fix the rest of bugs in the single function, > had started along this path and gave up on it, because I found it too > complicated. From that attempt I've concluded that the current approach > is much less error-prone and easier to follow. There may be some complexity I didn't realize. I don't want to make your life harder. If you are comfortable on this approach, fine by me.
diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c index 93c45a7..496400a 100644 --- a/gdb/dwarf2loc.c +++ b/gdb/dwarf2loc.c @@ -1963,8 +1963,6 @@ write_pieced_value (struct value *to, struct value *from) bits_to_skip -= this_size_bits; continue; } - if (this_size_bits > type_len - offset) - this_size_bits = type_len - offset; if (bits_to_skip > 0) { dest_offset_bits = bits_to_skip; @@ -1977,6 +1975,8 @@ write_pieced_value (struct value *to, struct value *from) dest_offset_bits = 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; source_offset = source_offset_bits / 8; diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp b/gdb/testsuite/gdb.dwarf2/var-access.exp index ee93b93..56a635a 100644 --- a/gdb/testsuite/gdb.dwarf2/var-access.exp +++ b/gdb/testsuite/gdb.dwarf2/var-access.exp @@ -174,6 +174,11 @@ gdb_test "print/d s1" " = \\{a = 63, b = 3, c = 0, d = 1\\}" \ "verify s1.a" gdb_test "print/d a" " = \\{0, 1, 63, 3, 4, 5, 6, 7\\}" \ "verify s1.a through a" +gdb_test_no_output "set var s1.b = 42" +gdb_test "print/d s1" " = \\{a = 63, b = 42, c = 0, d = 1\\}" \ + "verify s1.b" +gdb_test "print/d a" " = \\{0, 1, 63, 42, 4, 5, 6, 7\\}" \ + "verify s1.b through a" # Byte-aligned register- and memory pieces. gdb_test_no_output "set var \$[lindex $regname 0] = 81" \