Re: [RFA]: Clean up debug printing of pc in gdbserver

[Doug Evans <dje@google.com>]

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

Ping.
Attached is an updated patch (now that x86 gdbserver is biarch).

On Wed, May 6, 2009 at 8:11 PM, Doug Evans <dje@google.com> wrote:
> Hi.
>
> The debug printing of pc in linux-i386-low.c/linux-x86-64-low.c can
> use some clean up.
>
> - why restrict the printing to just x86?
> - the text that is printed for linux_resume_one_lwp is confusing
>
> Ok to check in?
>
> 2009-05-06  Doug Evans  <dje@google.com>
>
>        * linux-x86-64-low.c (debug_threads): Remove declaration.
>        (x86_64_get_pc,x86_64_set_pc): Remove debug printing of pc.
>        * linux-i386-low.c (debug_threads): Remove declaration.
>        (i386_get_pc,i386_set_pc): Remove debug printing of pc.
>        * linux-low.c (get_stop_pc): Print pc if debug_threads.
>        (check_removed_breakpoint, linux_wait_for_lwp): Ditto.
>        (linux_resume_one_lwp): Ditto.
>
> Index: linux-low.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
> retrieving revision 1.99
> diff -u -p -r1.99 linux-low.c
> --- linux-low.c 6 May 2009 17:32:59 -0000       1.99
> +++ linux-low.c 7 May 2009 02:10:55 -0000
> @@ -290,10 +290,14 @@ get_stop_pc (void)
>  {
>   CORE_ADDR stop_pc = (*the_low_target.get_pc) ();
>
> -  if (get_thread_lwp (current_inferior)->stepping)
> -    return stop_pc;
> -  else
> -    return stop_pc - the_low_target.decr_pc_after_break;
> +  if (! get_thread_lwp (current_inferior)->stepping)
> +    stop_pc -= the_low_target.decr_pc_after_break;
> +
> +  if (debug_threads)
> +    fprintf (stderr, "stop pc is %08lx\n",
> +            (long) stop_pc);
> +
> +  return stop_pc;
>  }
>
>  static void *
> @@ -750,7 +754,11 @@ check_removed_breakpoint (struct lwp_inf
>      decrement.  We go immediately from this function to resuming,
>      and can not safely call get_stop_pc () again.  */
>   if (the_low_target.set_pc != NULL)
> -    (*the_low_target.set_pc) (stop_pc);
> +    {
> +      if (debug_threads)
> +       fprintf (stderr, "Set pc to %08lx.\n", (long) stop_pc);
> +      (*the_low_target.set_pc) (stop_pc);
> +    }
>
>   /* We consumed the pending SIGTRAP.  */
>   event_child->pending_is_breakpoint = 0;
> @@ -878,14 +886,14 @@ retry:
>     }
>
>   if (debug_threads
> -      && WIFSTOPPED (*wstatp))
> +      && WIFSTOPPED (*wstatp)
> +      && the_low_target.get_pc != NULL)
>     {
>       struct thread_info *saved_inferior = current_inferior;
> +      CORE_ADDR stop_pc = (*the_low_target.get_pc) ();
>       current_inferior = (struct thread_info *)
>        find_inferior_id (&all_threads, child->head.id);
> -      /* For testing only; i386_stop_pc prints out a diagnostic.  */
> -      if (the_low_target.get_pc != NULL)
> -       get_stop_pc ();
> +      fprintf (stderr, "linux_wait_for_lwp: pc is %08lx\n", (long) stop_pc);
>       current_inferior = saved_inferior;
>     }
>
> @@ -1644,8 +1652,8 @@ linux_resume_one_lwp (struct inferior_li
>
>   if (debug_threads && the_low_target.get_pc != NULL)
>     {
> -      fprintf (stderr, "  ");
> -      (*the_low_target.get_pc) ();
> +      CORE_ADDR pc = (*the_low_target.get_pc) ();
> +      fprintf (stderr, "  resuming from pc 0x%lx\n", (long) pc);
>     }
>
>   /* If we have pending signals, consume one unless we are trying to reinsert
> Index: linux-x86-64-low.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/linux-x86-64-low.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 linux-x86-64-low.c
> --- linux-x86-64-low.c  22 Mar 2009 23:57:10 -0000      1.22
> +++ linux-x86-64-low.c  7 May 2009 03:03:09 -0000
> @@ -128,8 +128,6 @@ struct regset_info target_regsets[] = {
>  static const unsigned char x86_64_breakpoint[] = { 0xCC };
>  #define x86_64_breakpoint_len 1
>
> -extern int debug_threads;
> -
>  static CORE_ADDR
>  x86_64_get_pc ()
>  {
> @@ -137,16 +135,12 @@ x86_64_get_pc ()
>
>   collect_register_by_name ("rip", &pc);
>
> -  if (debug_threads)
> -    fprintf (stderr, "stop pc (before any decrement) is %08lx\n", pc);
>   return pc;
>  }
>
>  static void
>  x86_64_set_pc (CORE_ADDR newpc)
>  {
> -  if (debug_threads)
> -    fprintf (stderr, "set pc to %08lx\n", (long) newpc);
>   supply_register_by_name ("rip", &newpc);
>  }
>
> Index: linux-i386-low.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/linux-i386-low.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 linux-i386-low.c
> --- linux-i386-low.c    22 Mar 2009 23:57:10 -0000      1.19
> +++ linux-i386-low.c    7 May 2009 03:03:09 -0000
> @@ -154,8 +154,6 @@ struct regset_info target_regsets[] = {
>  static const unsigned char i386_breakpoint[] = { 0xCC };
>  #define i386_breakpoint_len 1
>
> -extern int debug_threads;
> -
>  static CORE_ADDR
>  i386_get_pc ()
>  {
> @@ -163,16 +161,12 @@ i386_get_pc ()
>
>   collect_register_by_name ("eip", &pc);
>
> -  if (debug_threads)
> -    fprintf (stderr, "stop pc (before any decrement) is %08lx\n", pc);
>   return pc;
>  }
>
>  static void
>  i386_set_pc (CORE_ADDR newpc)
>  {
> -  if (debug_threads)
> -    fprintf (stderr, "set pc to %08lx\n", (long) newpc);
>   supply_register_by_name ("eip", &newpc);
>  }
>
>

[-- Attachment #2: gdb-090523-gdbserver-get-pc-2.patch.txt --]
[-- Type: text/plain, Size: 2502 bytes --]

2009-05-06  Doug Evans  <dje@google.com>

	* linux-low.c (get_stop_pc): Print pc if debug_threads.
	(check_removed_breakpoint, linux_wait_for_lwp): Ditto.
	(linux_resume_one_lwp): Ditto.

Index: linux-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
retrieving revision 1.102
diff -u -p -r1.102 linux-low.c
--- linux-low.c	13 May 2009 17:17:22 -0000	1.102
+++ linux-low.c	24 May 2009 00:21:58 -0000
@@ -355,10 +355,13 @@ get_stop_pc (void)
 {
   CORE_ADDR stop_pc = (*the_low_target.get_pc) ();
 
-  if (get_thread_lwp (current_inferior)->stepping)
-    return stop_pc;
-  else
-    return stop_pc - the_low_target.decr_pc_after_break;
+  if (! get_thread_lwp (current_inferior)->stepping)
+    stop_pc -= the_low_target.decr_pc_after_break;
+
+  if (debug_threads)
+    fprintf (stderr, "stop pc is 0x%lx\n", (long) stop_pc);
+
+  return stop_pc;
 }
 
 static void *
@@ -815,7 +818,11 @@ check_removed_breakpoint (struct lwp_inf
      decrement.  We go immediately from this function to resuming,
      and can not safely call get_stop_pc () again.  */
   if (the_low_target.set_pc != NULL)
-    (*the_low_target.set_pc) (stop_pc);
+    {
+      if (debug_threads)
+	fprintf (stderr, "Set pc to 0x%lx.\n", (long) stop_pc);
+      (*the_low_target.set_pc) (stop_pc);
+    }
 
   /* We consumed the pending SIGTRAP.  */
   event_child->pending_is_breakpoint = 0;
@@ -943,14 +950,16 @@ retry:
     }
 
   if (debug_threads
-      && WIFSTOPPED (*wstatp))
+      && WIFSTOPPED (*wstatp)
+      && the_low_target.get_pc != NULL)
     {
       struct thread_info *saved_inferior = current_inferior;
+      CORE_ADDR stop_pc;
+
       current_inferior = (struct thread_info *)
 	find_inferior_id (&all_threads, child->head.id);
-      /* For testing only; i386_stop_pc prints out a diagnostic.  */
-      if (the_low_target.get_pc != NULL)
-	get_stop_pc ();
+      stop_pc = (*the_low_target.get_pc) ();
+      fprintf (stderr, "linux_wait_for_lwp: pc is 0x%lx\n", (long) stop_pc);
       current_inferior = saved_inferior;
     }
 
@@ -1709,8 +1718,8 @@ linux_resume_one_lwp (struct inferior_li
 
   if (debug_threads && the_low_target.get_pc != NULL)
     {
-      fprintf (stderr, "  ");
-      (*the_low_target.get_pc) ();
+      CORE_ADDR pc = (*the_low_target.get_pc) ();
+      fprintf (stderr, "  resuming from pc 0x%lx\n", (long) pc);
     }
 
   /* If we have pending signals, consume one unless we are trying to reinsert