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

Pedro Alves
2008-12-05T19:07:14Z

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