From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5344 invoked by alias); 24 Feb 2012 23:49:23 -0000 Received: (qmail 5336 invoked by uid 22791); 24 Feb 2012 23:49:20 -0000 X-SWARE-Spam-Status: No, hits=-1.7 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, 24 Feb 2012 23:49:06 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1S14sq-0007X7-PS from Maciej_Rozycki@mentor.com ; Fri, 24 Feb 2012 15:49:04 -0800 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by svr-orw-fem-01.mgc.mentorg.com over TLS secured channel with Microsoft SMTPSVC(6.0.3790.4675); Fri, 24 Feb 2012 15:49:04 -0800 Received: from [172.30.11.220] (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.1.289.1; Fri, 24 Feb 2012 23:49:01 +0000 Date: Sat, 25 Feb 2012 01:54:00 -0000 From: "Maciej W. Rozycki" To: Pedro Alves CC: Subject: Re: [PATCH] remote: Fix hw watchpoint address matching In-Reply-To: <201112091415.08804.pedro@codesourcery.com> Message-ID: References: <201112071724.06407.pedro@codesourcery.com> <201112091415.08804.pedro@codesourcery.com> User-Agent: Alpine 1.10 (DEB 962 2008-03-14) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" 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: 2012-02/txt/msg00598.txt.bz2 Hi Pedro, Back to this change, was distracted by something else. On Fri, 9 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. Yes, this is about watchpoints, not breakpoints. ;) The address matched against comes from stop_reply->watch_data_address (see process_stop_reply). This doesn't appear to be masked anywhere in watchpoints_triggered before target_watchpoint_addr_within_range is called and remote_insert_watchpoint doesn't propagate the ultimate masked address passed down the remote channel back to loc->address either. Therefore my understanding is both arguments to remote_watchpoint_addr_within_range have to be treated as unmasked -- addr because it may have been sign-extended by the remote stub, and start (i.e. loc->address) because it has never been masked in the first place. > > 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. */ Umm, there aren't that many comments of this kind there actually... > This prevents comment bit rot whenever the main comment in the > interface declaration changes, but implementations' comments > are forgotten. Good point. > 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. Thanks for your review. I have applied the final changes below then, as separate commits, as after the comment adjustment they are not really functionally bound to each other. Maciej 2012-02-24 Maciej W. Rozycki gdb/ * target.h (target_watchpoint_addr_within_range): Document macro. gdb-target-watch-range-doc.diff Index: gdb-fsf-trunk-quilt/gdb/target.h =================================================================== --- gdb-fsf-trunk-quilt.orig/gdb/target.h 2012-02-24 15:23:42.000000000 +0000 +++ gdb-fsf-trunk-quilt/gdb/target.h 2012-02-24 23:30:01.565618432 +0000 @@ -1483,6 +1483,8 @@ extern int target_ranged_break_num_regis #define target_stopped_data_address(target, addr_p) \ (*target.to_stopped_data_address) (target, addr_p) +/* Return non-zero if ADDR is within the range of a watchpoint spanning + LENGTH bytes beginning at START. */ #define target_watchpoint_addr_within_range(target, addr, start, length) \ (*target.to_watchpoint_addr_within_range) (target, addr, start, length) 2012-02-24 Maciej W. Rozycki gdb/ * remote.c (remote_watchpoint_addr_within_range): New function. (init_remote_ops): Use it. gdb-remote-watch-range.diff Index: gdb-fsf-trunk-quilt/gdb/remote.c =================================================================== --- gdb-fsf-trunk-quilt.orig/gdb/remote.c 2012-02-24 15:41:43.000000000 +0000 +++ gdb-fsf-trunk-quilt/gdb/remote.c 2012-02-24 23:29:05.445646325 +0000 @@ -7844,6 +7844,15 @@ remote_insert_watchpoint (CORE_ADDR addr _("remote_insert_watchpoint: reached end of function")); } +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 < length; +} + static int remote_remove_watchpoint (CORE_ADDR addr, int len, int type, @@ -10704,6 +10713,8 @@ Specify the serial device it is connecte remote_ops.to_remove_breakpoint = remote_remove_breakpoint; remote_ops.to_stopped_by_watchpoint = remote_stopped_by_watchpoint; remote_ops.to_stopped_data_address = remote_stopped_data_address; + remote_ops.to_watchpoint_addr_within_range = + remote_watchpoint_addr_within_range; remote_ops.to_can_use_hw_breakpoint = remote_check_watch_resources; remote_ops.to_insert_hw_breakpoint = remote_insert_hw_breakpoint; remote_ops.to_remove_hw_breakpoint = remote_remove_hw_breakpoint;