[RFA/PATCH] breakpoint.c: fix until command

[Michael Snyder]

Elena Zannoni wrote:
> 
> Daniel Jacobowitz writes:
>  > On Thu, Jan 02, 2003 at 05:44:42PM -0800, Michael Snyder wrote:
>  > > Elena Zannoni wrote:
>  > > >
>  > > > Elena Zannoni writes:
>  > > > >  > Nevertheless, that is and has always been the intent.
>  > > >  >  > If you're in factorial(5), and you say "until 100",
>  > > >  >  > you don't stop until line 100 is hit by factorial(5).
>  > > >  >
>  > > >  >
>  > > >  > I am tracking down this to something that changed between (ahem...)
>  > > >  > 4.18 and 5.0. The code in breakpoint.c didn't change. Right now,
>  > > >  > stepping the two gdb's side to side, I can see a difference in
>  > > >  > get_prev_frame, because of a different value returned by
>  > > >  > FRAME_CHAIN_VALID. :-( (i have not still stepped past that to see how
>  > > >  > that could influence the until foo behavior, maybe it doesn't).
>  > > >  >
>  > > >  > The behavior you specify above is in 5.0 and not in 4.18, while the
>  > > >  > 'until foo' works in 4.18 and is broken in 5.0.
>  > > >  >
>  > > >  > More digging.
>  > > >  >
>  > > >  > Elena
>  > > >
>  > > > OK. The reason for which 'until foo' worked at all in 4.18 is totally
>  > > > fortuitous.  It is because of this patch in breakpoint.c:
>  > > >
>  > > > 1998-09-08  Jason Molenda  (jsm@bugshack.cygnus.com)
>  > > >
>  > > >         * breakpoint.c (bpstat_stop_status):  Declare a bp match if the
>  > > >         current fp matches the bp->fp OR if the current fp is less than
>  > > >         the bp->fp if we're looking at a bp_step_resume breakpoint.
>  > > >
>  > > > Index: breakpoint.c
>  > > > ===================================================================
>  > > > RCS file: /cvs/cvsfiles/src/gdb/breakpoint.c,v
>  > > > retrieving revision 1.190
>  > > > retrieving revision 1.191
>  > > > diff -u -p -p -r1.190 -r1.191
>  > > > --- breakpoint.c        1998/07/17 15:29:10     1.190
>  > > > +++ breakpoint.c        1998/09/09 04:16:57     1.191
>  > > > @@ -1506,7 +1506,9 @@ bpstat_stop_status (pc, not_a_breakpoint
>  > > >        else if (DECR_PC_AFTER_BREAK != 0 || must_shift_inst_regs)
>  > > >         real_breakpoint = 1;
>  > > >
>  > > > -      if (b->frame && b->frame != (get_current_frame ())->frame)
>  > > > +      if (b->frame && b->frame != (get_current_frame ())->frame &&
>  > > > +          (b->type == bp_step_resume &&
>  > > > +           (get_current_frame ())->frame INNER_THAN b->frame))
>  > > >         bs->stop = 0;
>  > > >        else
>  > > >         {
>  > > >
>  > > > Note that this added condition is always false for a bp_until type
>  > > > breakpoint.  So, effectively we were invalidating the check of the
>  > > > current frame vs. bp->frame. And we always stopped.
>  > > >
>  > > > However, since we were not checking the frames, the case Michael wants
>  > > > didn't work.
>  > > >
>  > > > The patch above was reverted in 1999:
>  > > >
>  > > > 1999-08-13  Jim Kingdon  <http://developer.redhat.com/>
>  > > >
>  > > >         * breakpoint.c (bpstat_stop_status): Revert 1998-09-08 change
>  > > >         to ->frame matching.  The change did not match the ChangeLog
>  > > >         entry, looked fishy, and caused infinite stepping when running
>  > > >         "next" from main on sparc w/ RH Linux.  Thanks to Jakub for the
>  > > >         report.
>  > > >
>  > > > the effect was that the frame matching check was re-enabled, and so
>  > > > 'until foo' stopped working.
>  > > >
>  > > > I don't think there is a way to have both behaviors work correctly.  I
>  > > > thought of checking that the pc which you want to run until is in
>  > > > the same function as the one of the selected frame, and in that case
>  > > > enforce the check (by using a non-null frame for the bp_until),
>  > > > otherwise use the null frame (which disables the check). But what would
>  > > > be the correct behavior if you say:
>  > > >
>  > > > "until bar" where bar is recursive, and you are in "bar" at the
>  > > > moment?  This doesn't work currently. It seems intuitive that you
>  > > > would stop the next time you enter "bar". Right now you end up at the
>  > > > caller of "bar".
>  > > >
>  > > > I think it is a matter of deciding which behavior is more useful.
>  > > >
>  > > > (note that I tried to revert Jason's patch in stock 4.18 and 'until
>  > > > foo' stopped working, i.e. it wasn't something else that broke between
>  > > > 4.18 and 5.0)
>  > >
>  > > You raise a good point.  The commands "until <line>" and "until <func>"
>  > > are inconsistant.  Moreover the docs do not seem to describe this
>  > > recursion behavior.  Maybe a conversation with a wider audience is
>  > > in order (the gdb list)?  I'm sure I can't be the only one who
>  > > remembers that "until" behaved this way, and we shouldn't change
>  > > the behavior precipitously.
>  >
>  > Am I the only one getting the feeling that we have two useful behaviors
>  > here; and that we should pick one for "until" but expose the other
>  > under some other name or with some option?
>  >
> 
> yes, I don't think there is any way we can distinguish the recursive
> function cases from the non recursive ones, and the 'line'
> vs. 'funcname' argument.

I agree, but maybe we can distinguish between "a location inside the
current function" and "a location outside the current function".
After all, a line is not a meaningful distinction -- it could be a
line inside another function.

So I suggest using find_pc_partial_function to find the bounds
of the current function.  If <location> is inside, we use a 
frame-relative breakpoint.  If it's outside, we don't.

Wouldn't that satisfy the issue that you're working on?

Michael