This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun
- From: Daniel Jacobowitz <drow at mvista dot com>
- To: Eli Zaretskii <eliz at elta dot co dot il>
- Cc: gdb-patches at sources dot redhat dot com
- Date: Sun, 18 Jan 2004 10:19:10 -0500
- Subject: Re: RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun
- References: <20040117222007.GA23563@nevyn.them.org> <uy8s5ka50.fsf@elta.co.il>
On Sun, Jan 18, 2004 at 09:07:23AM +0200, Eli Zaretskii wrote:
> > Date: Sat, 17 Jan 2004 17:20:07 -0500
> > From: Daniel Jacobowitz <drow@mvista.com>
> >
> > One case, HANDLE_NONSTEPPABLE_WATCHPOINTS and DECR_PC_AFTER_BREAK, is simply
> > removed. There are no targets using this combination, and if one is added,
> > it's non-obvious whether a nonsteppable watchpoint really should be affected
> > by DECR_PC_AFTER_BREAK.
>
> Right, but since we don't really know what that feature was about, I'd
> suggest to leave a comment in adjust_pc_after_break that mentions
> HANDLE_NONSTEPPABLE_WATCHPOINTS and that its support, if needed,
> should be added.
Well, we know what HANDLE_NONSTEPPABLE_WATCHPOINTS was about. I'd be
curious to see whether any target ever used these two together, or if
the decrement was just added for consistency. I'll add a comment.
> > * breakpoint.c (software_breakpoint_inserted_here_p): New function.
> > (bpstat_stop_status): Don't decrement PC.
> > * breakpoint.h (software_breakpoint_inserted_here_p): Add
> > prototype.
> > * infrun.c (adjust_pc_after_break): New function.
> > (handle_inferior_event): Call it, early. Remove later references
> > to DECR_PC_AFTER_BREAK.
> > (normal_stop): Add commentary.
>
> What happens if a location has both software and hardware
> breakpoints? Does the code still DTRT?
Hmm, I am not sure. What _is_ the right thing? Decrement if the
software breakpoint was inserted, and do nothing if the hardware
breakpoint was inserted, and assume that both will not be inserted?
> > + /* If we've hit a breakpoint, we'll be stopped with SIGTRAP. */
> > + if (ecs->ws.kind != TARGET_WAITKIND_STOPPED)
> > + return;
> > +
> > + if (ecs->ws.value.sig != TARGET_SIGNAL_TRAP)
> > + return;
>
> The original code didn't check these conditions, right? So why add
> them here? (Also, the comment doesn't seem to describe the two
> tests, only the second one.)
The comment does describe both tests; if != TARGET_WAITKIND_STOPPED,
then we aren't stopped by a signal. The other waitkinds correspond to
things like exiting and catchpoints, and with the exception of some
complications in the FORKED/EXECD cases, stop_signal will not get set
to SIGTRAP. Also, the original code did check these conditions, though
somewhat indirectly:
if (stop_signal == TARGET_SIGNAL_TRAP)
{
/* Check if a regular breakpoint has been hit before checking
for a potential single step breakpoint. Otherwise, GDB will
not see this breakpoint hit when stepping onto breakpoints. */
Oh, hum, that's for the first set (thread hit thread-specific BP for a
different thread). The second thread does this:
if (stop_signal == TARGET_SIGNAL_TRAP
|| (breakpoints_inserted &&
(stop_signal == TARGET_SIGNAL_ILL
|| stop_signal == TARGET_SIGNAL_EMT))
|| stop_soon == STOP_QUIETLY
|| stop_soon == STOP_QUIETLY_NO_SIGSTOP)
The stop_soon's aren't relevant here, since they're handled before
DECR_PC_AFTER_BREAK, but the ILL/EMT are relevant. They should be
added to adjust_pc_after_break - thanks!
--
Daniel Jacobowitz
MontaVista Software Debian GNU/Linux Developer