From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24871 invoked by alias); 9 Dec 2011 14:15:48 -0000 Received: (qmail 24859 invoked by uid 22791); 9 Dec 2011 14:15:46 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 09 Dec 2011 14:15:13 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=EU1-MAIL.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1RZ1EG-0007Mn-8F from pedro_alves@mentor.com for gdb-patches@sourceware.org; Fri, 09 Dec 2011 06:15:12 -0800 Received: from scottsdale.localnet ([172.16.63.104]) by EU1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.1830); Fri, 9 Dec 2011 14:15:10 +0000 From: Pedro Alves To: "Maciej W. Rozycki" Subject: Re: [PATCH] remote: Fix hw watchpoint address matching Date: Fri, 09 Dec 2011 14:34:00 -0000 User-Agent: KMail/1.13.6 (Linux/2.6.38-13-generic; KDE/4.7.2; x86_64; ; ) Cc: gdb-patches@sourceware.org References: <201112071724.06407.pedro@codesourcery.com> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201112091415.08804.pedro@codesourcery.com> X-IsSubscribed: yes 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 X-SW-Source: 2011-12/txt/msg00288.txt.bz2 On Wednesday 07 December 2011 23:18:21, Maciej W. Rozycki wrote: > On Wed, 7 Dec 2011, Pedro Alves wrote: > > > > +static int > > > +remote_watchpoint_addr_within_range (struct target_ops *target, CORE_ADDR addr, > > > + CORE_ADDR start, int length) > > > +{ > > > + CORE_ADDR diff = remote_address_masked (addr - start); > > > + > > > + return diff >= 0 && diff < length; > > > +} > > > > CORE_ADDR is unsigned. `>= 0' is always true. > > Umm... > > > Wouldn't it be much more readable to: > > > > { > > CORE_ADDR start = remote_address_masked (start); > > > > return start <= addr && addr < start + length; > > } > > > > ? > > > > (assuming addr is already masked, since that was the address > > the target reported.) > > This makes me nervous. I think we should be liberal on what we accept. > In particular ILP32 ABIs on 64-bit targets may be affected. An example is > the MIPS n64 ABI where the width of general registers is 64 bits and > addresses are sign-extended 32 bits. When bit #31 is set in the address, > the remote stub may possibly report the value as truncated to 32 bits or > as a properly sign-extended 64-bit value. Not that I observed this > anywhere, but I think we should accept both. If such thing were possible, then wouldn't breakpoints break? We store the (masked) address of where we ended up putting the breakpoint in bp_tgt->placed_address (remote_insert_breakpoint), and if the target reported an address not exactly bp_tgt->placed_address, we wouldn't be able to match it up, resulting in spurious SIGTRAPs. Hmm, actually, it looks like breakpoint.c:bkpt_breakpoint_hit is broken in that it should be using bl->target_info.placed_address instead of bl->address ? How is this not breaking on cases that need breakpoint adjustment? I'm probably missing something. > Here's an updated version; I have annotated the function now too per > Joel's suggestion elsewhere even though these are rather scarce throughout > remote.c. Actually, for implementations of defined interfaces, such as the target vector or gdbarch callbacks, we prefer to leave the explanation of the interface to where the interface is defined, and, write something like /* Implementation of target method FOO. */ This prevents comment bit rot whenever the main comment in the interface declaration changes, but implementations' comments are forgotten. I see that target_watchpoint_addr_within_range is unfortunately undocumented in target.h. Fortunately, you've already written the necessary comment. :-) Could you place it there instead please? Okay with that change. Thanks. -- Pedro Alves