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

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

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.

> > > It seems to me just about every current user of stop_pc *really* wants
> > > to look at regcache_read_pc (get_current_regcache ()) ...
> 
> Is using read_pc instead OK with you?  It's what I had written already.

I guess so; longer term read_pc should probably die, but for now it's
not really worse than the alternative.

> @@ -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.

> @@ -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.

> @@ -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

> @@ -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 ...).

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com