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

Ulrich Weigand
2008-12-05T18:43:41Z

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