This is the mail archive of the
gdb@sourceware.org
mailing list for the GDB project.
Re: read watchpoints broken with remote targets?
- From: Daniel Jacobowitz <drow at false dot org>
- To: Eli Zaretskii <eliz at gnu dot org>
- Cc: Vladimir Prus <ghost at cs dot msu dot su>, gdb at sources dot redhat dot com
- Date: Sun, 13 Nov 2005 12:09:17 -0500
- Subject: Re: read watchpoints broken with remote targets?
- References: <200511111621.03372.ghost@cs.msu.su> <ulkzv2if7.fsf@gnu.org>
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 <jjohnstn@redhat.com>
* 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