From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31239 invoked by alias); 13 Nov 2005 17:09:23 -0000 Mailing-List: contact gdb-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-owner@sourceware.org Received: (qmail 31232 invoked by uid 22791); 13 Nov 2005 17:09:20 -0000 Received: from nevyn.them.org (HELO nevyn.them.org) (66.93.172.17) by sourceware.org (qpsmtpd/0.30-dev) with ESMTP; Sun, 13 Nov 2005 17:09:20 +0000 Received: from drow by nevyn.them.org with local (Exim 4.54) id 1EbLLx-0000VI-JK; Sun, 13 Nov 2005 12:09:17 -0500 Date: Sun, 13 Nov 2005 17:09:00 -0000 From: Daniel Jacobowitz To: Eli Zaretskii Cc: Vladimir Prus , gdb@sources.redhat.com Subject: Re: read watchpoints broken with remote targets? Message-ID: <20051113170917.GC465@nevyn.them.org> Mail-Followup-To: Eli Zaretskii , Vladimir Prus , gdb@sources.redhat.com References: <200511111621.03372.ghost@cs.msu.su> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.8i X-SW-Source: 2005-11/txt/msg00244.txt.bz2 On Fri, Nov 11, 2005 at 07:39:08PM +0200, Eli Zaretskii wrote: > > 1. handle_inferior_event (infrun.c) is called. > > > > 2. It contains: > > int stopped_by_watchpoint = -1; > > > > 3. The following code is executed: > > > > if (HAVE_CONTINUABLE_WATCHPOINT) > > stopped_by_watchpoint = STOPPED_BY_WATCHPOINT (ecs->ws); > > > > For remote targets, and for pretty much all other targets, > > HAVE_CONTINUABLE_WATCHPOINT is 0, so value of stopped_by_watchpoint > > is still -1 > > > > 4. Function bpstat_stop_status (breakpoint.c) is called, and > > stopped_by_watchpoint is passed to it (the value is still -1). > > > > 5. bpstat_stop_status tries to create a list of stop reasons, by iterating > > over all breakpoints and trying to check if that's breakpoint that's > > fired. For read wathcpoints we arrive at this: > > > > if ((b->type == bp_hardware_watchpoint > > || b->type == bp_read_watchpoint > > || b->type == bp_access_watchpoint) > > && !stopped_by_watchpoint) > > continue; > > > > since stepped_by_watchpoint is -1 we continue with the loop body, and > > arrive at this: > > > > bs = bpstat_alloc (b, bs); /* Alloc a bpstat to explain stop */ > > > > this adds a new element to the list of stop reasons. > > > > 6. We execute this code: > > > > if (!target_stopped_data_address (¤t_target, &addr)) > > continue; > > > > since there were no watchpoint, "continue" is executed. But the stop > > reasons list still has a new element corresponding to read > > watchpoint. > > > > 7. We return to handle_inhefiour_event, which notices the stop reasons list > > and stops stepping. > > Thanks for the detailed report. > > I think this should be fixed by explicitly testing for > stopped_by_watchpoint being -1, and treating it as if the value were > zero. Or perhaps bpstat_stop_status should change its value to zero > if it's -1. > > Can you see if one of these methods solves the problem? A little archeology is in order here. The stopped_by_watchpoint check was added only last year, by Ulrich Weigand. At that point it was 0 or 1. date: 2004/05/13 16:39:10; author: uweigand; state: Exp; lines: +4 -3 * breakpoint.c (bpstat_stop_status): Add new argument STOPPED_BY_WATCHPOINT. Use it instead of testing target_stopped_data_address agaist 0 to check whether or not we stopped due to a hardware watchpoint. * breakpoint.h (bpstat_stop_status): Adapt prototype. * infrun.c (handle_inferior_event): Call bpstat_stop_status with new argument. The initialization to -1 was added later: 2004-06-22 Jeff Johnston * infrun.c (handle_inferior_event): Initialize stopped_by_watchpoint to -1. * breakpoint.c (bpstat_stop_status): Move check for ignoring untriggered watchpoints to a separate if clause. Update function comment regarding STOPPED_BY_WATCHPOINT argument. + /* Continuable hardware watchpoints are treated as non-existent if the + reason we stopped wasn't a hardware watchpoint (we didn't stop on + some data address). Otherwise gdb won't stop on a break instruction + in the code (not from a breakpoint) when a hardware watchpoint has + been defined. */ So at the time, Jeff wanted to change this for only continuable watchpoints. Which are actually supported for i386, sparc-solaris, s390, and QNX, not just i386. He was trying to fix a bug on a platform without continuable watchpoints, ia64: http://sourceware.org/ml/gdb-patches/2004-06/msg00481.html It looks like this happens because we step over nonsteppable watchpoints before reporting them, thus preventing us from using STOPPED_BY_WATCHPOINT to know that we hit a watchpoint. Which doesn't really make any sense to me. Since i386 and sparc-solaris are probably the oldest supported targets with watchpoints, this may have been the path of least resistance when !HAVE_CONTINUABLE_WATCHPOINT support was added. I don't want to try to rearchitect those bits right now, though. So for the moment... Jeff fixed this problem for non-read watchpoints, but read watchpoints are still broken. > > I've fixed this by replacing code in (6) with: > > > > if (!target_stopped_data_address (¤t_target, &addr)) > > { > > bs->print_it = print_it_noop; > > bs->stop = 0; > > continue; > > } > > > > Could somebody comment if this is the right fix? > > I don't think this is the right fix. In effect, you say that if > stopped_by_watchpoint is non-zero, but target_stopped_data_address > says there was no watchpoint, let's ignore stopped_by_watchpoint. > That's not clean, IMHO. Actually, I think it is the right solution. This is a read or access watchpoint; we can't report it if we don't have target_stopped_data_address. The code already tried to do that. Here's what was there: /* Come here if it's a watchpoint, or if the break address matches */ bs = bpstat_alloc (b, bs); /* Alloc a bpstat to explain stop */ /* Watchpoints may change this, if not found to have triggered. */ bs->stop = 1; bs->print = 1; if (b->type == bp_watchpoint || b->type == bp_hardware_watchpoint) { .... else if (b->type == bp_read_watchpoint || b->type == bp_access_watchpoint) { CORE_ADDR addr; struct value *v; int found = 0; if (!target_stopped_data_address (¤t_target, &addr)) continue; So a watchpoint was found not to have triggered, but failed to change bs->stop, so it was bogusly reported. -- Daniel Jacobowitz CodeSourcery, LLC