From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2225 invoked by alias); 30 Oct 2010 07:13:01 -0000 Received: (qmail 2211 invoked by uid 22791); 30 Oct 2010 07:12:59 -0000 X-SWARE-Spam-Status: No, hits=-0.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_SOFTFAIL X-Spam-Check-By: sourceware.org Received: from mtaout21.012.net.il (HELO mtaout21.012.net.il) (80.179.55.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 30 Oct 2010 07:12:53 +0000 Received: from conversion-daemon.a-mtaout21.012.net.il by a-mtaout21.012.net.il (HyperSendmail v2007.08) id <0LB300E00D6MJN00@a-mtaout21.012.net.il> for gdb-patches@sourceware.org; Sat, 30 Oct 2010 09:12:50 +0200 (IST) Received: from HOME-C4E4A596F7 ([77.126.229.202]) by a-mtaout21.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0LB300EZPDDDCK80@a-mtaout21.012.net.il>; Sat, 30 Oct 2010 09:12:50 +0200 (IST) Date: Sat, 30 Oct 2010 07:13:00 -0000 From: Eli Zaretskii Subject: Re: [patch 2/2] Implement support for PowerPC BookE masked and ranged watchpoints In-reply-to: <1288403952.2598.58.camel@hactar> To: Thiago Jung Bauermann Cc: gdb-patches@sourceware.org Reply-to: Eli Zaretskii Message-id: <83wrp05f2f.fsf@gnu.org> References: <1282074110.2606.703.camel@hactar> <1287807761.10521.423.camel@hactar> <838w1p8egs.fsf@gnu.org> <1288403952.2598.58.camel@hactar> 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-10/txt/msg00400.txt.bz2 > From: Thiago Jung Bauermann > Cc: gdb-patches@sourceware.org > Date: Fri, 29 Oct 2010 23:59:12 -0200 > > The version of the patch you reviewed wouldn't allow other architectures > to create ranged software watchpoints (it asked the hardware whether the > capability was available), though that was an unnecessary restriction. > This version allows ranged software watchpoints for all architectures. > (The change was made in the watch_range_command_1 function). That's good to hear. I think this is NEWS worthy, btw. > > And what about masked watchpoints -- can they also be used in software > > mode? > > AFAIK, to implement masked watchpoints in software GDB would need to > decode each instruction to see which address it was trying to access. > This patch doesn't implement that, it would be hard work. If the target > doesn't allow masked hardware watchpoints, the watch command will error > out. That's fine with me, I just wanted to make sure that documentation faithfully reflects the implementation. > > > + case bp_hardware_watchpoint: > > > + ui_out_text (uiout, "Masked hardware watchpoint "); > > > > Isn't it better to say "Masked hardware (write) watchpoint"? > > Here I'm following the current GDB behaviour. It says just "Hardware > watchpoint" for a bp_hardware_watchpoint. OK. > > > + /* Display one line of extra information about this breakpoint, > > > + for "info breakpoints". */ > > > + void (*print_one_detail) (const struct breakpoint *, struct ui_out *); > > > > Examples of such "extras" would be beneficial here. Also, are there > > any limitations on this extra information, like maximum length? > > I changed this part to: > > /* Display extra information about this breakpoint, below the normal > breakpoint description in "info breakpoints". In the example below, > the line with "memory range: [0x10094354, 0x100943a2]" was printed > by print_one_detail_ranged_watchpoint. > > (gdb) info breakpoints > Num Type Disp Enb Address What > 2 hw watchpoint keep y b > memory range: [0x10094354, 0x100943a2] > > */ > void (*print_one_detail) (const struct breakpoint *, struct ui_out *); > > What do you think? I'm happy now. Thanks. > > > + /* The watchpoint will trigger if the address of the memory access is > > > + within the defined range, as follows: p.addr <= address < p.addr2. */ > > > + p.addr = (uint64_t) addr; > > > + p.addr2 = (uint64_t) addr + len; > > > > But the documentation says that the end address is included, not > > excluded. Can we eliminate this source of confusion? > > This code is in ppc-linux-nat.c right before calling ptrace to set the > watchpoint, so it just documents how the kernel/processor interprets the > parameters. > > The user-facing code respects the semantics explained in the > documentation. Also, target_insert_ranged_watchpoint takes a start > address and a length as arguments and IMHO doesn't have this confusion. Well, maybe this is worth a comment in this place. Something like "Note that this just documents how ptrace interprets its arguments; the watchpoint is set to watch the defined range _inclusively_, as specified by the user interface." > gdb/doc/ > * gdb.texinfo (Set Watchpoints): Document mask parameter. > (PowerPC Embedded): Document masked watchpoints and ranged watchpoints. This part is approved. Thanks.