PR cli/13110

PR cli/13110

[Pedro Alves]

<http://sourceware.org/ml/gdb/2011-09/msg00019.html>

Tested on x86_64-linux and applied.

Repeating what I said on gdb@, I'm now wondering if we actually ever
need the registers_changed call here or in wait_for_inferior
nowadays --- we flush threads' register caches when we resume
them (target_resume).

-- 
Pedro Alves

2011-09-05  Pedro Alves  <pedro@codesourcery.com>

	PR cli/13110

	gdb/
	* infrun.c (fetch_inferior_event): Check if there's a selected
	thread before checking if the selected thread is executing.

---
 gdb/infrun.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2011-09-05 15:50:49.000000000 +0100
+++ src/gdb/infrun.c	2011-09-05 15:57:10.693964411 +0100
@@ -2749,7 +2749,9 @@ fetch_inferior_event (void *client_data)
      switches threads anyway).  If we didn't do this, a spurious
      delayed event in all-stop mode would make the user lose the
      selected frame.  */
-  if (non_stop || is_executing (inferior_ptid))
+  if (non_stop
+      || (!ptid_equal (inferior_ptid, null_ptid)
+	  && is_executing (inferior_ptid)))
     registers_changed ();
 
   make_cleanup_restore_integer (&execution_direction);

Re: PR cli/13110

[Doug Evans]

On Mon, Sep 5, 2011 at 8:56 AM, Pedro Alves <pedro@codesourcery.com> wrote:
> <http://sourceware.org/ml/gdb/2011-09/msg00019.html>
>
> Tested on x86_64-linux and applied.
>
> Repeating what I said on gdb@, I'm now wondering if we actually ever
> need the registers_changed call here or in wait_for_inferior
> nowadays --- we flush threads' register caches when we resume
> them (target_resume).
>
> --
> Pedro Alves
>
> 2011-09-05  Pedro Alves  <pedro@codesourcery.com>
>
>        PR cli/13110
>
>        gdb/
>        * infrun.c (fetch_inferior_event): Check if there's a selected
>        thread before checking if the selected thread is executing.
>
> ---
>  gdb/infrun.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> Index: src/gdb/infrun.c
> ===================================================================
> --- src.orig/gdb/infrun.c       2011-09-05 15:50:49.000000000 +0100
> +++ src/gdb/infrun.c    2011-09-05 15:57:10.693964411 +0100
> @@ -2749,7 +2749,9 @@ fetch_inferior_event (void *client_data)
>      switches threads anyway).  If we didn't do this, a spurious
>      delayed event in all-stop mode would make the user lose the
>      selected frame.  */
> -  if (non_stop || is_executing (inferior_ptid))
> +  if (non_stop
> +      || (!ptid_equal (inferior_ptid, null_ptid)
> +         && is_executing (inferior_ptid)))
>     registers_changed ();
>
>   make_cleanup_restore_integer (&execution_direction);
>

The code may be to additionally check if there's a selected thread,
but when I read it I think "When is null_ptid ever executing?".

Comment in the code is required IMO.

Re: PR cli/13110

[Pedro Alves]

On Wednesday 07 September 2011 01:45:36, Doug Evans wrote:
> On Mon, Sep 5, 2011 at 8:56 AM, Pedro Alves <pedro@codesourcery.com> wrote:
> > <http://sourceware.org/ml/gdb/2011-09/msg00019.html>
> >
> > Tested on x86_64-linux and applied.
> >
> > Repeating what I said on gdb@, I'm now wondering if we actually ever
> > need the registers_changed call here or in wait_for_inferior
> > nowadays --- we flush threads' register caches when we resume
> > them (target_resume).

> > Index: src/gdb/infrun.c
> > ===================================================================
> > --- src.orig/gdb/infrun.c       2011-09-05 15:50:49.000000000 +0100
> > +++ src/gdb/infrun.c    2011-09-05 15:57:10.693964411 +0100
> > @@ -2749,7 +2749,9 @@ fetch_inferior_event (void *client_data)
> >      switches threads anyway).  If we didn't do this, a spurious
> >      delayed event in all-stop mode would make the user lose the
> >      selected frame.  */
> > -  if (non_stop || is_executing (inferior_ptid))
> > +  if (non_stop
> > +      || (!ptid_equal (inferior_ptid, null_ptid)
> > +         && is_executing (inferior_ptid)))
> >     registers_changed ();
> >
> >   make_cleanup_restore_integer (&execution_direction);
> >
> 
> The code may be to additionally check if there's a selected thread,
> but when I read it I think "When is null_ptid ever executing?".
> 
> Comment in the code is required IMO.

How about removing the registers_changed calls once and
for all?

Tested on x86_64-linux native|gdbserver x sync|async.

-- 
Pedro Alves

2011-09-07  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* infrun.c (prepare_for_detach, wait_for_inferior)
	(fetch_inferior_event): Don't flush the register cache.
	* remote.c (struct stop_reply) <regcache>: Add comment.

---
 gdb/infrun.c |   33 ---------------------------------
 gdb/remote.c |    4 ++++
 2 files changed, 4 insertions(+), 33 deletions(-)

Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2011-09-07 13:21:46.877976256 +0100
+++ src/gdb/infrun.c	2011-09-07 14:41:52.954338077 +0100
@@ -2580,14 +2580,6 @@ prepare_for_detach (void)
 
       overlay_cache_invalid = 1;
 
-      /* We have to invalidate the registers BEFORE calling
-	 target_wait because they can be loaded from the target while
-	 in target_wait.  This makes remote debugging a bit more
-	 efficient for those targets that provide critical registers
-	 as part of their normal status mechanism.  */
-
-      registers_changed ();
-
       if (deprecated_target_wait_hook)
 	ecs->ptid = deprecated_target_wait_hook (pid_ptid, &ecs->ws, 0);
       else
@@ -2657,14 +2649,7 @@ wait_for_inferior (void)
     {
       struct cleanup *old_chain;
 
-      /* We have to invalidate the registers BEFORE calling target_wait
-	 because they can be loaded from the target while in target_wait.
-	 This makes remote debugging a bit more efficient for those
-	 targets that provide critical registers as part of their normal
-	 status mechanism.  */
-
       overlay_cache_invalid = 1;
-      registers_changed ();
 
       if (deprecated_target_wait_hook)
 	ecs->ptid = deprecated_target_wait_hook (waiton_ptid, &ecs->ws, 0);
@@ -2734,26 +2719,8 @@ fetch_inferior_event (void *client_data)
        running any breakpoint commands.  */
     make_cleanup_restore_current_thread ();
 
-  /* We have to invalidate the registers BEFORE calling target_wait
-     because they can be loaded from the target while in target_wait.
-     This makes remote debugging a bit more efficient for those
-     targets that provide critical registers as part of their normal
-     status mechanism.  */
-
   overlay_cache_invalid = 1;
 
-  /* But don't do it if the current thread is already stopped (hence
-     this is either a delayed event that will result in
-     TARGET_WAITKIND_IGNORE, or it's an event for another thread (and
-     we always clear the register and frame caches when the user
-     switches threads anyway).  If we didn't do this, a spurious
-     delayed event in all-stop mode would make the user lose the
-     selected frame.  */
-  if (non_stop
-      || (!ptid_equal (inferior_ptid, null_ptid)
-	  && is_executing (inferior_ptid)))
-    registers_changed ();
-
   make_cleanup_restore_integer (&execution_direction);
   execution_direction = target_execution_direction ();
 
Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2011-09-07 13:21:46.877976256 +0100
+++ src/gdb/remote.c	2011-09-07 14:41:52.954338077 +0100
@@ -4898,6 +4898,10 @@ struct stop_reply
 
   struct target_waitstatus ws;
 
+  /* Expedited registers.  This makes remote debugging a bit more
+     efficient for those targets that provide critical registers as
+     part of their normal status mechanism (as another roundtrip to
+     fetch them is avoided).  */
   VEC(cached_reg_t) *regcache;
 
   int stopped_by_watchpoint_p;

Re: PR cli/13110

[Doug Evans]

On Wed, Sep 7, 2011 at 7:12 AM, Pedro Alves <pedro@codesourcery.com> wrote:
> How about removing the registers_changed calls once and
> for all?

Having/requiring them to be littered throughout the code always seemed
a tad fragile ...

Re: PR cli/13110

[Pedro Alves]

On Wednesday 07 September 2011 15:12:01, Pedro Alves wrote:
> 2011-09-07  Pedro Alves  <pedro@codesourcery.com>
> 
>         gdb/
>         * infrun.c (prepare_for_detach, wait_for_inferior)
>         (fetch_inferior_event): Don't flush the register cache.
>         * remote.c (struct stop_reply) <regcache>: Add comment.
> 

Now applied, thanks.

-- 
Pedro Alves