Re: Get rid of stop_pc (was: [RFA] dummy frame handling cleanup, plus inferior fun call signal handling improvement)

[Pedro Alves <pedro@codesourcery.com>]

On Friday 05 December 2008 18:42:45, Ulrich Weigand wrote:
> Pedro Alves wrote:
> 
> > > > > <stopped at 0x1234, thread 1>
> > > > >  (gdb) set $pc = 0xf00
> > > > >  (gdb) call func()
> > > > 
> > > > Huh.  But that case is in fact *broken*, because GDB will use stop_pc
> > > > incorrectly: for example, the check whether we are about to continue
> > > > at a breakpoint will look at stop_pc, but then continue at $pc.  
> > > 
> > > This one I believe was the original intention.  The rationale being
> > > that you'd not want to hit a breakpoint again at stop_pc (0x1234),
> > > because there's where you stopped; but, you'd want to hit a a breakpoint
> > > at 0xf00, sort of like jump *$pc hits a breakpoint at $pc.
> > > 
> > > Note, I'm not saying I agree with this.  I did say that probably nobody
> > > would notice if we got rid of stop_pc.
> 
> OK, I see.  This is a valid use case, and it may make sense to keep it.
> However, as you point out, to make this really work as intended, we'd
> have make stop_pc a per-thread variable.
> 
> And even in that case, the uses of stop_pc in step_1 and step_once seem
> invalid to me.
> 

100% Agreed.  I'll take care of it.

> > @@ -3705,6 +3706,7 @@ handle_step_into_function (struct execut
> >  {
> >    struct symtab *s;
> >    struct symtab_and_line stop_func_sal, sr_sal;
> > +  CORE_ADDR stop_pc = read_pc ();
> >  
> >    s = find_pc_symtab (stop_pc);
> >    if (s && s->language != language_asm)
> > @@ -3781,6 +3783,7 @@ handle_step_into_function_backward (stru
> >  {
> >    struct symtab *s;
> >    struct symtab_and_line stop_func_sal, sr_sal;
> > +  CORE_ADDR stop_pc = read_pc ();
> >  
> >    s = find_pc_symtab (stop_pc);
> >    if (s && s->language != language_asm)
> 
> These could probably receive the stop_pc from handle_inferior_event
> instead of recomputing it.

Right.  It would hit the cache, but, then again, if/when we have
a stop_pc per-thread, we'd use that.

> 
> > @@ -4283,7 +4286,7 @@ Further execution is probably impossible
> >  	      if (tp->stop_step
> >  		  && frame_id_eq (tp->step_frame_id,
> >  				  get_frame_id (get_current_frame ()))
> > -		  && step_start_function == find_pc_function (stop_pc))
> > +		  && step_start_function == find_pc_function (read_pc ()))
> >  		source_flag = SRC_LINE;	/* finished step, just print source line */
> >  	      else
> >  		source_flag = SRC_AND_LOC;	/* print location and source line */
> 
> As Andrew's comment notes, the function comparison should be redundant
> these days as it is already implied in the frame-ID comparison.
> 

Oh, that's what that comment means?  I always had trouble parsing the
English in it.  Makes sense.

> > @@ -1149,7 +1149,7 @@ signal_command (char *signum_exp, int fr
> >       FIXME: Neither should "signal foo" but when I tried passing
> >       (CORE_ADDR)-1 unconditionally I got a testsuite failure which I haven't
> >       tried to track down yet.  */
> > -  proceed (oursig == TARGET_SIGNAL_0 ? (CORE_ADDR) -1 : stop_pc, oursig, 0);
> > +  proceed (oursig == TARGET_SIGNAL_0 ? (CORE_ADDR) -1 : read_pc (), oursig, 0);
> >  }
> >  
> >  /* Proceed until we reach a different source line with pc greater than
> 
> Dan wanted to get rid of this use of stop_pc anyway, see:
> http://sourceware.org/ml/gdb-patches/2008-08/msg00651.html

Yep.  I think his patch makes sense:
 http://sourceware.org/ml/gdb-patches/2008-11/msg00439.html

> 
> > @@ -1585,8 +1585,7 @@ program_info (char *args, int from_tty)
> >    stat = bpstat_num (&bs, &num);
> >  
> >    target_files_info ();
> > -  printf_filtered (_("Program stopped at %s.\n"),
> > -		   hex_string ((unsigned long) stop_pc));
> > +  printf_filtered (_("Program stopped at %s.\n"), paddr_nz (read_pc ()));
> >    if (tp->stop_step)
> >      printf_filtered (_("It stopped after being stepped.\n"));
> >    else if (stat != 0)
> 
> If we keep a tp->stop_pc, this place should also make use of it;
> otherwise the message isn't really valid (and not very useful:
> if it always just prints $pc, it would be redundant with the
> other commands to do so ...).

Right you are.

I'll fix the step_1/step_once bit for now.

-- 
Pedro Alves