From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 81878 invoked by alias); 9 Mar 2017 17:22:21 -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 80713 invoked by uid 89); 9 Mar 2017 17:22:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 spammy=packing, interchange, fulfilling, Hx-languages-length:3230 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; Thu, 09 Mar 2017 17:22:19 +0000 Received: from pps.filterd (m0098413.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v29HJCV4045340 for ; Thu, 9 Mar 2017 12:22:18 -0500 Received: from e06smtp15.uk.ibm.com (e06smtp15.uk.ibm.com [195.75.94.111]) by mx0b-001b2d01.pphosted.com with ESMTP id 2937yaudf8-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 09 Mar 2017 12:22:18 -0500 Received: from localhost by e06smtp15.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 9 Mar 2017 17:22:16 -0000 Received: from b06cxnps4075.portsmouth.uk.ibm.com (9.149.109.197) by e06smtp15.uk.ibm.com (192.168.101.145) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Thu, 9 Mar 2017 17:22:14 -0000 Received: from d06av23.portsmouth.uk.ibm.com (d06av23.portsmouth.uk.ibm.com [9.149.105.59]) by b06cxnps4075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v29HMEHr16777232; Thu, 9 Mar 2017 17:22:14 GMT Received: from d06av23.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 36013A405B; Thu, 9 Mar 2017 17:22:09 +0000 (GMT) Received: from d06av23.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 198CAA405D; Thu, 9 Mar 2017 17:22:09 +0000 (GMT) Received: from oc1027705133.ibm.com (unknown [9.152.212.162]) by d06av23.portsmouth.uk.ibm.com (Postfix) with ESMTPS; Thu, 9 Mar 2017 17:22:09 +0000 (GMT) From: Andreas Arnez To: Simon Marchi Cc: Subject: Re: [PATCH 1/3] inf-ptrace: Do not stop memory transfers after a single word References: <1488816060-20776-1-git-send-email-arnez@linux.vnet.ibm.com> <1488816060-20776-2-git-send-email-arnez@linux.vnet.ibm.com> <06e5cf43-bb8b-6fa5-7201-414dc88388a3@ericsson.com> Date: Thu, 09 Mar 2017 17:22:00 -0000 In-Reply-To: <06e5cf43-bb8b-6fa5-7201-414dc88388a3@ericsson.com> (Simon Marchi's message of "Wed, 8 Mar 2017 14:09:44 -0500") 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: 17030917-0020-0000-0000-0000031A3AB7 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17030917-0021-0000-0000-000040B66897 Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-03-09_14:,, 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-1702020001 definitions=main-1703090130 X-IsSubscribed: yes X-SW-Source: 2017-03/txt/msg00121.txt.bz2 Simon, Thanks for your comments! On Wed, Mar 08 2017, Simon Marchi wrote: > On 17-03-06 11:00 AM, Andreas Arnez wrote: [...] >> This patch fixes the performance issue by attempting to fulfill the whole >> transfer request in inf_ptrace_xfer_partial, using a loop around the >> ptrace call. > > I think the idea is good. The xfer partial interface says that the > target should transfer up to len bytes. Transferring 1 word at the > time respects the contract, but we should try to be more efficient > when possible. Right, and I think the function now behaves more like you would expect (https://en.wikipedia.org/wiki/Principle_of_least_astonishment). Maybe at some point we should also fix the discrepancy between fulfilling the contract and still not working correctly, but that is another story. [...] >> + unsigned chunk; > > "unsigned" -> "unsigned int"? OK. [...] >> + /* Read the word, also when doing a partial word write. */ >> + if (readbuf || chunk < sizeof (PTRACE_TYPE_RET)) > > Use != NULL or == NULL when checking pointers. OK. (I thought I've seen patches that stopped following this rule after the C++ transition.) (1) [...] > This is not a comment specifically about your patch, since that's how > it was already, but maybe it would be a good time to address this. I > understand there's some level of overlap between the read and write > (like the offset/skip computation), but I don't think that handling > reading and writing in the same loop is very readable. It just adds a > bunch of branches and makes it hard to follow. If that code was split > in two functions (one for read, one for write), it would be way more > straightforward. That's probably a matter of taste. Note that we do have separate routines in gdbserver/linux-low.c that fulfill the equivalent function: linux_read_memory() and linux_write_memory(). IMO they have even worse readability *plus* suffer from heavy code duplication. Maybe that's just me, though. Another thought that crossed my mind is whether we should extract the whole peek/poke loop into a separate function instead of packing all the logic under a case statement. So far I didn't, because I wanted to keep the bug fix small. -- Andreas (1) The GDB C/C++ coding standards provide a dubious explanation for the "NULL Is Not Zero" rule (https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#NULL_Is_Not_Zero): "Zero constant (0) is not interchangeable with a null pointer constant (NULL) anywhere. GCC does not give a warning for such interchange." To me this seems to imply that the language does not support the interchangeability. But according to the C standard, it does: "An integer constant expression with the value 0, or such an expression cast to type void *, is called a null pointer constant." C++ has a similar definition and specifies boolean conversion from pointer types as well. See also Stroustrup's style FAQ "should I use NULL or 0?": http://www.stroustrup.com/bs_faq2.html#null So maybe we want to support non-conforming compilers? Or is this in fact a GDB-specific style rule? In either case we should adjust the explanation, I think.