[rfc] Fix register cache invalidation

[rfc] Fix register cache invalidation

[Ulrich Weigand]

Hello,

when testing the Cell/B.E. combined debugger patches I ran into a situation
where registers_changed was not called soon enough.

A while ago I proposed a patch to simplify register invalidation (as part of
a infrun cleanup series):
http://sourceware.org/ml/gdb-patches/2008-12/msg00120.html
This cleanup actually fixes the combined debugger problem as well.

At the time, Pedro pointed out that parts of that patch weren't safe in the
non-stop case.  The version below omits those parts, and only changes code
to simplify calling registers_changed and resetting waiton_ptid.

Tested on powerpc64-linux with no regressions.
Any comments?  I'd like to commit this in a couple of days.

Bye,
Ulrich


ChangeLog:

	* infrun.c (wait_for_inferior): Invalidate registers and overlay
	cache every time before calling target_wait.
	(handle_inferior_event): Make static. Always reset waiton_ptid.
	Never call registers_changed.
	(prepare_to_wait): Do not invaliate registers or overlay cache
	(moved to wait_for_inferior).  Do not reset waiton_ptid (moved
	to handle_inferior_event).
	(fetch_inferior_event): Remove check for always-true condition.  


Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c
+++ src/gdb/infrun.c
@@ -1756,7 +1756,7 @@ struct execution_control_state
 
 static void init_execution_control_state (struct execution_control_state *ecs);
 
-void handle_inferior_event (struct execution_control_state *ecs);
+static void handle_inferior_event (struct execution_control_state *ecs);
 
 static void handle_step_into_function (struct gdbarch *gdbarch,
 				       struct execution_control_state *ecs);
@@ -1993,23 +1993,22 @@ wait_for_inferior (int treat_exec_as_sig
   ecs = &ecss;
   memset (ecs, 0, sizeof (*ecs));
 
-  overlay_cache_invalid = 1;
-
   /* We'll update this if & when we switch to a new thread.  */
   previous_inferior_ptid = inferior_ptid;
 
-  /* 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 ();
-
   while (1)
     {
       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);
       else
@@ -2063,14 +2062,8 @@ fetch_inferior_event (void *client_data)
 
   memset (ecs, 0, sizeof (*ecs));
 
-  overlay_cache_invalid = 1;
-
-  /* We can only rely on wait_for_more being correct before handling
-     the event in all-stop, but previous_inferior_ptid isn't used in
-     non-stop.  */
-  if (!ecs->wait_some_more)
-    /* We'll update this if & when we switch to a new thread.  */
-    previous_inferior_ptid = inferior_ptid;
+  /* We'll update this if & when we switch to a new thread.  */
+  previous_inferior_ptid = inferior_ptid;
 
   if (non_stop)
     /* In non-stop mode, the user/frontend should not notice a thread
@@ -2085,6 +2078,7 @@ fetch_inferior_event (void *client_data)
      targets that provide critical registers as part of their normal
      status mechanism. */
 
+  overlay_cache_invalid = 1;
   registers_changed ();
 
   if (deprecated_target_wait_hook)
@@ -2375,7 +2369,7 @@ stepped_in_from (struct frame_info *fram
    by an event from the inferior, figure out what it means and take
    appropriate action.  */
 
-void
+static void
 handle_inferior_event (struct execution_control_state *ecs)
 {
   struct frame_info *frame;
@@ -2444,8 +2438,6 @@ handle_inferior_event (struct execution_
     case infwait_thread_hop_state:
       if (debug_infrun)
         fprintf_unfiltered (gdb_stdlog, "infrun: infwait_thread_hop_state\n");
-      /* Cancel the waiton_ptid. */
-      waiton_ptid = pid_to_ptid (-1);
       break;
 
     case infwait_normal_state:
@@ -2476,7 +2468,9 @@ handle_inferior_event (struct execution_
     default:
       internal_error (__FILE__, __LINE__, _("bad switch"));
     }
+
   infwait_state = infwait_normal_state;
+  waiton_ptid = pid_to_ptid (-1);
 
   switch (ecs->ws.kind)
     {
@@ -3001,7 +2995,6 @@ targets should add new threads to the th
 
 	      ecs->event_thread->stepping_over_breakpoint = 1;
 	      keep_going (ecs);
-	      registers_changed ();
 	      return;
 	    }
 	}
@@ -3076,7 +3069,6 @@ targets should add new threads to the th
 	/* Single step */
       hw_step = maybe_software_singlestep (gdbarch, stop_pc);
       target_resume (ecs->ptid, hw_step, TARGET_SIGNAL_0);
-      registers_changed ();
       waiton_ptid = ecs->ptid;
       if (target_have_steppable_watchpoint)
 	infwait_state = infwait_step_watch_state;
@@ -4482,19 +4474,7 @@ prepare_to_wait (struct execution_contro
 {
   if (debug_infrun)
     fprintf_unfiltered (gdb_stdlog, "infrun: prepare_to_wait\n");
-  if (infwait_state == infwait_normal_state)
-    {
-      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 ();
-      waiton_ptid = pid_to_ptid (-1);
-    }
   /* This is the old end of the while loop.  Let everybody know we
      want to wait for the inferior some more and get called again
      soon.  */
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Fix register cache invalidation

[Pedro Alves]

On Tuesday 14 July 2009 15:59:28, Ulrich Weigand wrote:

> when testing the Cell/B.E. combined debugger patches I ran into a situation
> where registers_changed was not called soon enough.
> 
> A while ago I proposed a patch to simplify register invalidation (as part of
> a infrun cleanup series):
> http://sourceware.org/ml/gdb-patches/2008-12/msg00120.html
> This cleanup actually fixes the combined debugger problem as well.

Curious.  Are we sure we're not hidding some other bug then?

> At the time, Pedro pointed out that parts of that patch weren't safe in the
> non-stop case.  The version below omits those parts, and only changes code
> to simplify calling registers_changed and resetting waiton_ptid.

> Tested on powerpc64-linux with no regressions.
> Any comments?  I'd like to commit this in a couple of days.

I like it!

> 	> 	(fetch_inferior_event): Remove check for always-true condition.   
> 

> -  /* We can only rely on wait_for_more being correct before handling
> -     the event in all-stop, but previous_inferior_ptid isn't used in
> -     non-stop.  */
> -  if (!ecs->wait_some_more)
> -    /* We'll update this if & when we switch to a new thread.  */
> -    previous_inferior_ptid = inferior_ptid;
> +  /* We'll update this if & when we switch to a new thread.  */
> +  previous_inferior_ptid = inferior_ptid;

This is actually a bug in the async/all-stop (you're not introducing
it, it's already there).  We shouldn't be clobbering previous_inferior_ptid
on every internaly handled event.  Just noting it --- don't worry about it.

-- 
Pedro Alves

Re: [rfc] Fix register cache invalidation

[Ulrich Weigand]

Pedro Alves wrote:
> On Tuesday 14 July 2009 15:59:28, Ulrich Weigand wrote:
> 
> > when testing the Cell/B.E. combined debugger patches I ran into a situation
> > where registers_changed was not called soon enough.
> > 
> > A while ago I proposed a patch to simplify register invalidation (as part of
> > a infrun cleanup series):
> > http://sourceware.org/ml/gdb-patches/2008-12/msg00120.html
> > This cleanup actually fixes the combined debugger problem as well.
> 
> Curious.  Are we sure we're not hidding some other bug then?

Hmm, interestingly enough I cannot reproduce the problem I was seeing with
the current combined debugger patches ...

The problem I had was basically that during adjust_pc_after_break I was
getting the wrong architecture (and thus the wrong decr_pc_after_break
value) because the old architecture had been cached, and no registers_changed
call happened in between.

As the problem disappeared when applying the cleanup patch (which I'd
intended to do anyway), I didn't investigate in more detail.  Unfortunately,
the underlying problem seems to have gone away anyway in the meantime ...

> > At the time, Pedro pointed out that parts of that patch weren't safe in the
> > non-stop case.  The version below omits those parts, and only changes code
> > to simplify calling registers_changed and resetting waiton_ptid.
> 
> > Tested on powerpc64-linux with no regressions.
> > Any comments?  I'd like to commit this in a couple of days.
> 
> I like it!

OK, thanks for looking at the patch!

I've checked this in now.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com