Re: RFC: Centralize DECR_PC_AFTER_BREAK handling from infrun

[Daniel Jacobowitz <drow@mvista.com>]

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