From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24855 invoked by alias); 19 Apr 2017 14:36:00 -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 24819 invoked by uid 89); 19 Apr 2017 14:36:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.1 required=5.0 tests=AWL,BAYES_00,KAM_ASCII_DIVIDERS,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 spammy=operate, capping, concluded X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0b-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.158.5) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 19 Apr 2017 14:35:58 +0000 Received: from pps.filterd (m0098416.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v3JEY4ol047450 for ; Wed, 19 Apr 2017 10:35:58 -0400 Received: from e06smtp14.uk.ibm.com (e06smtp14.uk.ibm.com [195.75.94.110]) by mx0b-001b2d01.pphosted.com with ESMTP id 29x6yv9041-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 19 Apr 2017 10:35:58 -0400 Received: from localhost by e06smtp14.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 19 Apr 2017 15:35:56 +0100 Received: from b06cxnps4074.portsmouth.uk.ibm.com (9.149.109.196) by e06smtp14.uk.ibm.com (192.168.101.144) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Wed, 19 Apr 2017 15:35:55 +0100 Received: from d06av23.portsmouth.uk.ibm.com (d06av23.portsmouth.uk.ibm.com [9.149.105.59]) by b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v3JEZtZM39452704; Wed, 19 Apr 2017 14:35:55 GMT Received: from d06av23.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id B0F96A4065; Wed, 19 Apr 2017 15:34:55 +0100 (BST) Received: from d06av23.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 88D78A4055; Wed, 19 Apr 2017 15:34:55 +0100 (BST) Received: from oc1027705133.ibm.com (unknown [9.152.212.162]) by d06av23.portsmouth.uk.ibm.com (Postfix) with ESMTPS; Wed, 19 Apr 2017 15:34:55 +0100 (BST) From: Andreas Arnez To: Yao Qi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 2/9] Fix size capping in write_pieced_value References: <1491586736-21296-1-git-send-email-arnez@linux.vnet.ibm.com> <1491586736-21296-3-git-send-email-arnez@linux.vnet.ibm.com> <86r30wafft.fsf@gmail.com> <86k26gahcg.fsf@gmail.com> Date: Wed, 19 Apr 2017 14:36:00 -0000 In-Reply-To: <86k26gahcg.fsf@gmail.com> (Yao Qi's message of "Wed, 19 Apr 2017 10:15:27 +0100") 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: 17041914-0016-0000-0000-00000480C573 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17041914-0017-0000-0000-0000274EC8E5 Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-04-19_12:,, 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-1704190123 X-IsSubscribed: yes X-SW-Source: 2017-04/txt/msg00564.txt.bz2 On Wed, Apr 19 2017, Yao Qi wrote: > Andreas Arnez 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