From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12102 invoked by alias); 7 Dec 2011 23:18:49 -0000 Received: (qmail 12076 invoked by uid 22791); 7 Dec 2011 23:18:48 -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; Wed, 07 Dec 2011 23:18:34 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1RYQkz-0005Yu-Fd from Maciej_Rozycki@mentor.com for gdb-patches@sourceware.org; Wed, 07 Dec 2011 15:18:33 -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); Wed, 7 Dec 2011 15:18:33 -0800 Received: from [172.30.6.216] (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; Wed, 7 Dec 2011 23:18:30 +0000 Date: Wed, 07 Dec 2011 23:36:00 -0000 From: "Maciej W. Rozycki" To: Pedro Alves CC: Subject: Re: [PATCH] remote: Fix hw watchpoint address matching In-Reply-To: <201112071724.06407.pedro@codesourcery.com> Message-ID: References: <201112071724.06407.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: 2011-12/txt/msg00241.txt.bz2 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. 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. I can rewrite it to call remote_address_masked separately for addr and start if that makes you feel better, although frankly I don't see an obvious benefit from doing that and there's a small performance loss. Thanks for your review. Any further comments? 2011-12-07 Maciej W. Rozycki gdb/ * remote.c (remote_watchpoint_addr_within_range): New function. (init_remote_ops): Use it. Maciej gdb-remote-watch-range.diff Index: gdb-fsf-trunk-quilt/gdb/remote.c =================================================================== --- gdb-fsf-trunk-quilt.orig/gdb/remote.c 2011-12-07 21:07:11.000000000 +0000 +++ gdb-fsf-trunk-quilt/gdb/remote.c 2011-12-07 22:19:03.255601238 +0000 @@ -7808,6 +7808,18 @@ remote_insert_watchpoint (CORE_ADDR addr _("remote_insert_watchpoint: reached end of function")); } +/* Return 1 if ADDR is within the range of a watchpoint spanning LENGTH + bytes beginning at START, otherwise 0. */ + +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, @@ -10607,6 +10619,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;