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

Pedro Alves
2008-12-05T01:16:35Z

[-- Attachment #1: Type: text/plain, Size: 1933 bytes --]

On Friday 05 December 2008 00:36:56, Pedro Alves wrote:
> On Friday 05 December 2008 00:18:00, Ulrich Weigand wrote:
> > Pedro Alves wrote:
> > > On Thursday 04 December 2008 22:32:12, Doug Evans wrote:
> > > > In the original code, is there a case when stop_pc != registers.pc?
> > >
> > > Here,
> > >
> > > <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.
>
> > 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've been sneaking the idea of getting rid of stop_pc for a while now:
> http://sourceware.org/ml/gdb-patches/2008-06/msg00450.html
>
> In fact, I have a months old patch here that completelly removes stop_pc.
> IIRC, there were no visible changes in the testsuite. Say the word,
> and I'll brush it up, regtest, submit it.

Here it is, it still applied cleanly. It's smallish. Regtested on
x86-64-unknown-linux-gnu.

My original motivation was to get rid of the ugly checks
in switch_to_thread, and to try to minimize the extra thread
switching and register reads in non-stop mode.

I had held posting this when I wrote it, since I was not sure we'd not
miss stop_pc in some case.

--
Pedro Alves

[-- Attachment #2: stop_pc.diff --]
[-- Type: text/x-diff, Size: 11578 bytes --]

2008-12-05 Pedro Alves <pedro@codesourcery.com>

Remove stop_pc.

* infrun.c (proceed): Don't check stop_pc against the current pc.
(handle_inferior_event): Add a stop_pc local.
(handle_step_into_function): Likewise.
(handle_step_into_function_backward): Likewise.
(normal_stop): Likewise.
(struct inferior_status) <stop_pc>: Remove it.
(save_inferior_status): Don't store the stop_pc.
(restore_inferior_status): Don't restore it.
* linux-fork.c (fork_load_infrun_state): Don't set it.
* m32c-tdep.c (m32c_skip_trampoline_code): Adjust comment.
* remote-mips.c (common_open): Remove reading the stop_pc.
* solib-sunos.c (disable_break): Replace stop_pc reference by a
read_pc call.
(sunos_solib_create_inferior_hook): Add a stop_pc local.
* target.h (target_wait): Adjust comment.
* thread.c (switch_to_thread): Don't read the stop_pc.
* infcmd.c (stop_pc): Delete.
(step_1, step_once, signal_command, program_info): Adjust.
* inferior.h (stop_pc): Delete declaration.

---
gdb/fork-child.c | 2 --
gdb/infcmd.c | 19 +++++++++----------
gdb/inferior.h | 4 ----
gdb/infrun.c | 14 +++++---------
gdb/linux-fork.c | 1 -
gdb/m32c-tdep.c | 2 +-
gdb/remote-mips.c | 1 -
gdb/solib-sunos.c | 3 ++-
gdb/target.h | 2 +-
gdb/thread.c | 8 --------
10 files changed, 18 insertions(+), 38 deletions(-)

Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c 2008-12-05 00:49:28.000000000 +0000
+++ src/gdb/infrun.c 2008-12-05 01:10:15.000000000 +0000
@@ -1311,7 +1311,7 @@ proceed (CORE_ADDR addr, enum target_sig

if (addr == (CORE_ADDR) -1)
{
- if (pc == stop_pc && breakpoint_here_p (pc)
+ if (breakpoint_here_p (pc)
&& execution_direction != EXEC_REVERSE)
/* There is a breakpoint at the address we will resume at,
step one instruction before inserting breakpoints so that
@@ -2100,6 +2100,7 @@ handle_inferior_event (struct execution_
int stepped_after_stopped_by_watchpoint = 0;
struct symtab_and_line stop_pc_sal;
enum stop_kind stop_soon;
+ CORE_ADDR stop_pc;

if (ecs->ws.kind != TARGET_WAITKIND_EXITED
&& ecs->ws.kind != TARGET_WAITKIND_SIGNALLED
@@ -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)
@@ -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 */
@@ -4350,10 +4353,6 @@ Further execution is probably impossible
ends with a setting of the current frame, so we can use that
next. */
frame_pop (get_current_frame ());
- /* Set stop_pc to what it was before we called the function.
- Can't rely on restore_inferior_status because that only gets
- called if we don't stop in the called function. */
- stop_pc = read_pc ();
select_frame (get_current_frame ());
}

@@ -4753,7 +4752,6 @@ signals_info (char *signum_exp, int from
struct inferior_status
{
enum target_signal stop_signal;
- CORE_ADDR stop_pc;
bpstat stop_bpstat;
int stop_step;
int stop_stack_dummy;
@@ -4792,7 +4790,6 @@ save_inferior_status (int restore_stack_
struct inferior *inf = current_inferior ();

inf_status->stop_signal = tp->stop_signal;
- inf_status->stop_pc = stop_pc;
inf_status->stop_step = tp->stop_step;
inf_status->stop_stack_dummy = stop_stack_dummy;
inf_status->stopped_by_random_signal = stopped_by_random_signal;
@@ -4847,7 +4844,6 @@ restore_inferior_status (struct inferior
struct inferior *inf = current_inferior ();

tp->stop_signal = inf_status->stop_signal;
- stop_pc = inf_status->stop_pc;
tp->stop_step = inf_status->stop_step;
stop_stack_dummy = inf_status->stop_stack_dummy;
stopped_by_random_signal = inf_status->stopped_by_random_signal;
Index: src/gdb/linux-fork.c
===================================================================
--- src.orig/gdb/linux-fork.c 2008-12-05 00:47:34.000000000 +0000
+++ src/gdb/linux-fork.c 2008-12-05 00:49:53.000000000 +0000
@@ -250,7 +250,6 @@ fork_load_infrun_state (struct fork_info
registers_changed ();
reinit_frame_cache ();

- stop_pc = read_pc ();
nullify_last_target_wait_ptid ();

/* Now restore the file positions of open file descriptors. */
Index: src/gdb/m32c-tdep.c
===================================================================
--- src.orig/gdb/m32c-tdep.c 2008-10-21 15:45:49.000000000 +0100
+++ src/gdb/m32c-tdep.c 2008-12-05 00:49:53.000000000 +0000
@@ -2306,7 +2306,7 @@ m32c_skip_trampoline_code (struct frame_
struct gdbarch_tdep *tdep = gdbarch_tdep (get_frame_arch (frame));

/* It would be nicer to simply look up the addresses of known
- trampolines once, and then compare stop_pc with them. However,
+ trampolines once, and then compare the stop pc with them. However,
we'd need to ensure that that cached address got invalidated when
someone loaded a new executable, and I'm not quite sure of the
best way to do that. find_pc_partial_function does do some
Index: src/gdb/remote-mips.c
===================================================================
--- src.orig/gdb/remote-mips.c 2008-12-02 12:19:23.000000000 +0000
+++ src/gdb/remote-mips.c 2008-12-05 00:49:53.000000000 +0000
@@ -1583,7 +1583,6 @@ device is attached to the target board (

reinit_frame_cache ();
registers_changed ();
- stop_pc = read_pc ();
print_stack_frame (get_selected_frame (NULL), 0, SRC_AND_LOC);
xfree (serial_port_name);
}
Index: src/gdb/solib-sunos.c
===================================================================
--- src.orig/gdb/solib-sunos.c 2008-10-21 15:45:49.000000000 +0100
+++ src/gdb/solib-sunos.c 2008-12-05 00:49:53.000000000 +0000
@@ -533,7 +533,7 @@ disable_break (void)
SunOS version we don't know it until the above code is executed.
Grumble if we are stopped anywhere besides the breakpoint address. */

- if (stop_pc != breakpoint_addr)
+ if (read_pc () != breakpoint_addr)
{
warning (_("stopped at unknown breakpoint while handling shared libraries"));
}
@@ -790,6 +790,7 @@ sunos_solib_create_inferior_hook (void)

if (gdbarch_decr_pc_after_break (target_gdbarch))
{
+ CORE_ADDR stop_pc = read_pc ();
stop_pc -= gdbarch_decr_pc_after_break (target_gdbarch);
write_pc (stop_pc);
}
Index: src/gdb/target.h
===================================================================
--- src.orig/gdb/target.h 2008-12-05 00:47:34.000000000 +0000
+++ src/gdb/target.h 2008-12-05 00:49:53.000000000 +0000
@@ -628,7 +628,7 @@ extern void target_resume (ptid_t ptid,
_NOT_ OK to throw_exception() out of target_wait() without popping
the debugging target from the stack; GDB isn't prepared to get back
to the prompt with a debugging target but without the frame cache,
- stop_pc, etc., set up. */
+ etc., set up. */

#define target_wait(ptid, status) \
(*current_target.to_wait) (ptid, status)
Index: src/gdb/thread.c
===================================================================
--- src.orig/gdb/thread.c 2008-12-05 00:47:41.000000000 +0000
+++ src/gdb/thread.c 2008-12-05 00:49:53.000000000 +0000
@@ -766,14 +766,6 @@ switch_to_thread (ptid_t ptid)
inferior_ptid = ptid;
reinit_frame_cache ();
registers_changed ();
-
- /* We don't check for is_stopped, because we're called at times
- while in the TARGET_RUNNING state, e.g., while handling an
- internal event. */
- if (!is_exited (ptid) && !is_executing (ptid))
- stop_pc = read_pc ();
- else
- stop_pc = ~(CORE_ADDR) 0;
}

static void
Index: src/gdb/infcmd.c
===================================================================
--- src.orig/gdb/infcmd.c 2008-12-05 00:47:41.000000000 +0000
+++ src/gdb/infcmd.c 2008-12-05 00:49:53.000000000 +0000
@@ -144,10 +144,6 @@ static char *inferior_io_terminal;

ptid_t inferior_ptid;

-/* Address at which inferior stopped. */
-
-CORE_ADDR stop_pc;
-
/* Flag indicating that a command has proceeded the inferior past the
current breakpoint. */

@@ -805,6 +801,7 @@ step_1 (int skip_subroutines, int single
for (; count > 0; count--)
{
struct thread_info *tp = inferior_thread ();
+ CORE_ADDR stop_pc = read_pc ();
clear_proceed_status ();

frame = get_current_frame ();
@@ -924,14 +921,17 @@ step_once (int skip_subroutines, int sin
the longjmp breakpoint was not required. Use the
INFERIOR_PTID thread instead, which is the same thread when
THREAD is set. */
- struct thread_info *tp = inferior_thread ();
+ struct thread_info *tp;
+ CORE_ADDR stop_pc;
+
+ tp = inferior_thread ();
clear_proceed_status ();

frame = get_current_frame ();
- if (!frame) /* Avoid coredump here. Why tho? */
- error (_("No current frame"));
tp->step_frame_id = get_frame_id (frame);

+ stop_pc = read_pc ();
+
if (!single_inst)
{
find_pc_line_pc_range (stop_pc,
@@ -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
@@ -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)
Index: src/gdb/inferior.h
===================================================================
--- src.orig/gdb/inferior.h 2008-12-05 00:47:40.000000000 +0000
+++ src/gdb/inferior.h 2008-12-05 00:49:53.000000000 +0000
@@ -271,10 +271,6 @@ extern void interrupt_target_1 (int all_

extern void detach_command (char *, int);

-/* Address at which inferior stopped. */
-
-extern CORE_ADDR stop_pc;
-
/* Flag indicating that a command has proceeded the inferior past the
current breakpoint. */

Index: src/gdb/fork-child.c
===================================================================
--- src.orig/gdb/fork-child.c 2008-12-05 00:51:40.000000000 +0000
+++ src/gdb/fork-child.c 2008-12-05 00:52:00.000000000 +0000
@@ -530,8 +530,6 @@ startup_inferior (int ntraps)

/* Mark all threads non-executing. */
set_executing (pid_to_ptid (-1), 0);
-
- stop_pc = read_pc ();
}

/* Implement the "unset exec-wrapper" command. */