This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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
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.  */

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]