From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11382 invoked by alias); 16 Nov 2010 04:01:41 -0000 Received: (qmail 11373 invoked by uid 22791); 16 Nov 2010 04:01:40 -0000 X-SWARE-Spam-Status: No, hits=-6.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_BF,TW_CP,TW_XB,TW_XF,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 16 Nov 2010 04:01:35 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id oAG41U2r023857 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 15 Nov 2010 23:01:30 -0500 Received: from host0.dyn.jankratochvil.net (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id oAG41Pvh026656 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 15 Nov 2010 23:01:29 -0500 Received: from host0.dyn.jankratochvil.net (localhost.localdomain [127.0.0.1]) by host0.dyn.jankratochvil.net (8.14.4/8.14.4) with ESMTP id oAG41Nbb020745; Tue, 16 Nov 2010 05:01:23 +0100 Received: (from jkratoch@localhost) by host0.dyn.jankratochvil.net (8.14.4/8.14.4/Submit) id oAG41GnC020744; Tue, 16 Nov 2010 05:01:16 +0100 Date: Tue, 16 Nov 2010 04:01:00 -0000 From: Jan Kratochvil To: Thiago Jung Bauermann Cc: Eli Zaretskii , gdb-patches@sourceware.org Subject: Re: [patch 2/2] Implement support for PowerPC BookE masked and ranged watchpoints Message-ID: <20101116040116.GA19243@host0.dyn.jankratochvil.net> References: <1282074110.2606.703.camel@hactar> <1287807761.10521.423.camel@hactar> <838w1p8egs.fsf@gnu.org> <1288403952.2598.58.camel@hactar> <83wrp05f2f.fsf@gnu.org> <1288648026.3377.7.camel@hactar> <83d3qo5qkj.fsf@gnu.org> <1288905125.14606.4.camel@hactar> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1288905125.14606.4.camel@hactar> User-Agent: Mutt/1.5.21 (2010-09-15) 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: 2010-11/txt/msg00193.txt.bz2 # awatch-range 0xbffff8e8,0xbffff8f8 [...] # awatch *0xbffff8e8 mask 0xffffff00 What is the difference of this masked range versus? awatch-range 0xbffff800, 0xbffff8ff If there is no difference then the mask address feature is useful only for masks which are not of the form (-1 << n). On Thu, 04 Nov 2010 22:12:05 +0100, Thiago Jung Bauermann wrote: > +watch-range START_ADDR,+LENGTH | START_ADDR, END_ADDR > +rwatch-range START_ADDR,+LENGTH | START_ADDR, END_ADDR > +awatch-range START_ADDR,+LENGTH | START_ADDR, END_ADDR > + Set a hardware watchpoint for an address range. > + The watchpoint will stop execution of your program whenever the inferior > + writes, reads, or accesses (respectively for watch-range, awatch-range > + and rwatch-range) any address within the specified range. What is the problem with the syntax? watch *array@elementcount OR watch *(char *)array@bytescount People already must know the @ operator just for printing arrays. And some nifty user friendly is provided by FE (graphical front end) and not CLI. I may be biased but I would not try to invent new commands unless necessary, GDB has already enough of them no user knows them all. > + target_resources_ok = target_can_use_hardware_watchpoint two spaces. > +static int > +insert_ranged_watchpoint (struct bp_location *bpt) Like going to send in the other mail, `bpt' is (mostly) used for `struct breakpoint'. For bp_location please use `bl', `bploc' or some other bp_location-suggesting names used in GDB. > + c = add_com ("watch-range", class_breakpoint, watch_range_command, _("\ > +Set a hardware watchpoint for an address range.\n\ > +The address range should be specified in one of the following formats:\n\ > +\n\ > + start-address, end-address\n\ > + start-address, +length\n\ > +\n\ > +The watchpoint will stop execution of the inferior whenever it\n\ > +writes to any address within the [start-address, end-address] range.")); I would prefer some explicit "both inclusively" statement there. One could expect `end-address' to be the exclusive one. > +/* Special types of hardware breakpoints/watchpoints. */ > +enum hw_point_flag { Incorrect GNU formatting. > + HW_POINT_RANGED_WATCH, /* Hardware ranged watchpoint. */ > + HW_POINT_MASKED_WATCH /* Hardware masked watchpoint. */ > +}; > + p.version = PPC_DEBUG_CURRENT_VERSION; These extraneous spaces do not conform to GNU formatting. > + p.trigger_type = get_trigger_type (rw); > + p.addr_mode = PPC_BREAKPOINT_MODE_MASK; > + p.condition_mode = PPC_BREAKPOINT_CONDITION_NONE; > + p.addr = (uint64_t) addr; > + p.addr2 = (uint64_t) mask; Probably excessive cast. > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -601,11 +601,16 @@ update_current_target (void) > INHERIT (to_files_info, t); > INHERIT (to_insert_breakpoint, t); > INHERIT (to_remove_breakpoint, t); > + INHERIT (to_can_use_special_hw_point, t); There are now two target interface styles in use. This inheriting one and the runtime-inheriting one (see target_pid_to_str and others). I was told the target_pid_to_str style is now preferred and it makes sense to me. Please convert the new target vector methods to the new style. > + if (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT)) > + { Wrong GNU formatting. > + address_start &= ((CORE_ADDR) 1 << addr_bit) - 1; > + address_end &= ((CORE_ADDR) 1 << addr_bit) - 1; > + } > + > + /* FIXME: cagney/2002-05-03: Need local_address_string() function > + that returns the language localized string formatted to a width > + based on gdbarch_addr_bit. */ > + if (addr_bit <= 32) > + { Wrong GNU formatting. > + strcpy (addstr, "["); > + strcat (addstr, hex_string_custom (address_start, 8)); > + strcat (addstr, ", "); > + strcat (addstr, hex_string_custom (address_end, 8)); > + strcat (addstr, "]"); > + } > + else > + { Wrong GNU formatting. > --- a/gdb/ui-out.h > +++ b/gdb/ui-out.h > @@ -113,6 +113,12 @@ extern void ui_out_field_fmt_int (struct ui_out *uiout, int width, > enum ui_align align, const char *fldname, > int value); > > +extern void ui_out_field_range_core_addr (struct ui_out *uiout, > + const char *fldname, > + struct gdbarch *gdbarch, Spaces used instead of tabs. > + CORE_ADDR address_start, > + CORE_ADDR length); > + Thanks, Jan