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

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

[Doug Evans]

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);
 }
 

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

[Doug Evans]

[-- 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

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

[Pedro Alves]

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

Looks OK to me.  Small nit below.

On Thursday 07 May 2009 04:11:08, Doug Evans wrote:
>    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;
>      }

Can we rename that `stop_pc' variable to, say, `pc', so we're
consistent throughout?  We can think of the `stop_pc' as having
always the decr_pc_after_break adjustment applied.

-- 
Pedro Alves

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

[Doug Evans]

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

On Sun, May 24, 2009 at 9:47 AM, Pedro Alves <pedro@codesourcery.com> wrote:
>
>> 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.
>
> Looks OK to me.  Small nit below.
>
> On Thursday 07 May 2009 04:11:08, Doug Evans wrote:
>>    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;
>>      }
>
> Can we rename that `stop_pc' variable to, say, `pc', so we're
> consistent throughout?  We can think of the `stop_pc' as having
> always the decr_pc_after_break adjustment applied.

Thanks.

Attached is what I checked in.

[-- Attachment #2: gdb-090524-gdbserver-get-pc-3.patch.txt --]
[-- Type: text/plain, Size: 2486 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.103
diff -u -p -r1.103 linux-low.c
--- linux-low.c	24 May 2009 01:09:22 -0000	1.103
+++ linux-low.c	24 May 2009 17:27:10 -0000
@@ -354,10 +354,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 *
@@ -814,7 +817,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;
@@ -942,14 +949,16 @@ retry:
     }
 
   if (debug_threads
-      && WIFSTOPPED (*wstatp))
+      && WIFSTOPPED (*wstatp)
+      && the_low_target.get_pc != NULL)
     {
       struct thread_info *saved_inferior = current_inferior;
+      CORE_ADDR 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 ();
+      pc = (*the_low_target.get_pc) ();
+      fprintf (stderr, "linux_wait_for_lwp: pc is 0x%lx\n", (long) pc);
       current_inferior = saved_inferior;
     }
 
@@ -1706,8 +1715,8 @@ linux_resume_one_lwp (struct lwp_info *l
 
   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