[rfc] [2/7] infrun cleanup: miscellaneous cleanups

[rfc] [2/7] infrun cleanup: miscellaneous cleanups

[Ulrich Weigand]

Hello,

this patch performs a number of cleanups that the follwoing patches
depend upon.  While I think that this patch should have no effect on
GDB behaviour, this is less obviously the case that for the follow-on
patches (which are clearly mechanical changes).  Therefore, I'd 
definitely appreciate feedback in particular on this patch.

The changes are in detail:

* handle_inferior_event is made static.

* prepare_to_wait performs some cleanup in preparation for another
  target_wait call, most notably calling registers_changed.  It does
  this only in infwait_normal_state -- but in those places where
  another infwait_state is chosen, handle_inferior_event actually
  calls registers_changed itself.

  This patch changes wait_for_inferior to *always* invalidate
  registers (and overlay cache state) before every call to target_wait,
  and removes this from prepare_to_wait and handle_inferior_event.

* prepare_to_wait also resets waiton_ptid.  This is moved into
  handle_inferior_event at the place where infwait_state is reset
  (those two globals really should always be set at the same time).

* fetch_inferior_event uses the contents of ecs->ptid and
  ecs->event_thread after handle_inferior_event returns.  As later
  patches will remove those fields, I've replaced them with accessing
  the current inferior -- just like normal_stop, which is called
  by fetch_inferior_event anyway, does as well.

* Finally, some dead code is removed: infrun_thread_stop_requested_callback
  sets ecs->event_thread before calling handle_inferior_event -- which is
  always ignored and re-computed by that routine.  fetch_inferior_event
  performs a check for !ecs->wait_some_more -- immediately after memsetting
  the ecs structure to zero.  And finally handle_step_into_function_backward
  computes the function end-of-prologue address  -- without ever using it.

Bye,
Ulrich


ChangeLog:

	* infrun.c (infrun_thread_stop_requested_callback): Do not set
	ecs->event_thread before calling handle_inferior_event.

	(fetch_inferior_event): Remove check for always-true condition.
	Do not rely on contents of ecs->ptid or ecs->event_thread
	after calling handle_inferior_event.

	(handle_step_into_function_backward): Do not modify (unused)
	ecs->stop_func_start.

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


Index: gdb-head/gdb/infrun.c
===================================================================
--- gdb-head.orig/gdb/infrun.c
+++ gdb-head/gdb/infrun.c
@@ -1581,7 +1581,7 @@ struct execution_control_state
 
 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 execution_control_state *ecs);
 static void handle_step_into_function_backward (struct execution_control_state *ecs);
@@ -1631,7 +1631,6 @@ infrun_thread_stop_requested_callback (s
 	 have consistent output as if the stop event had been
 	 reported.  */
       ecs->ptid = info->ptid;
-      ecs->event_thread = find_thread_pid (info->ptid);
       ecs->ws.kind = TARGET_WAITKIND_STOPPED;
       ecs->ws.value.sig = TARGET_SIGNAL_0;
 
@@ -1761,21 +1760,20 @@ wait_for_inferior (void)
   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)
     {
+      /* 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);
       else
@@ -1810,14 +1808,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
@@ -1832,6 +1824,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)
@@ -1854,7 +1847,7 @@ fetch_inferior_event (void *client_data)
 
   if (!ecs->wait_some_more)
     {
-      struct inferior *inf = find_inferior_pid (ptid_get_pid (ecs->ptid));
+      struct inferior *inf = current_inferior ();
 
       delete_step_thread_step_resume_breakpoint ();
 
@@ -1865,8 +1858,8 @@ fetch_inferior_event (void *client_data)
       if (target_has_execution
 	  && ecs->ws.kind != TARGET_WAITKIND_EXITED
 	  && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED
-	  && ecs->event_thread->step_multi
-	  && ecs->event_thread->stop_step)
+	  && inferior_thread ()->step_multi
+	  && inferior_thread ()->stop_step)
 	inferior_event_handler (INF_EXEC_CONTINUE, NULL);
       else
 	inferior_event_handler (INF_EXEC_COMPLETE, NULL);
@@ -2078,7 +2071,7 @@ ensure_not_running (void)
    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)
 {
   int sw_single_step_trap_p = 0;
@@ -2141,8 +2134,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:
@@ -2173,7 +2164,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)
     {
@@ -2665,7 +2658,6 @@ targets should add new threads to the th
 
 	      ecs->event_thread->stepping_over_breakpoint = 1;
 	      keep_going (ecs);
-	      registers_changed ();
 	      return;
 	    }
 	}
@@ -2732,7 +2724,6 @@ targets should add new threads to the th
 	 
       if (!HAVE_STEPPABLE_WATCHPOINT)
 	remove_breakpoints ();
-      registers_changed ();
       target_resume (ecs->ptid, 1, TARGET_SIGNAL_0);	/* Single step */
       waiton_ptid = ecs->ptid;
       if (HAVE_STEPPABLE_WATCHPOINT)
@@ -3765,14 +3756,7 @@ handle_step_into_function (struct execut
 static void
 handle_step_into_function_backward (struct execution_control_state *ecs)
 {
-  struct symtab *s;
-  struct symtab_and_line stop_func_sal, sr_sal;
-
-  s = find_pc_symtab (stop_pc);
-  if (s && s->language != language_asm)
-    ecs->stop_func_start = gdbarch_skip_prologue (current_gdbarch, 
-						  ecs->stop_func_start);
-
+  struct symtab_and_line stop_func_sal;
   stop_func_sal = find_pc_line (stop_pc, 0);
 
   /* OK, we're just going to keep stepping here.  */
@@ -3998,19 +3982,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] [2/7] infrun cleanup: miscellaneous cleanups

[Pedro Alves]

On Sunday 07 December 2008 00:17:36, Ulrich Weigand wrote:
>   This patch changes wait_for_inferior to *always* invalidate
>   registers (and overlay cache state) before every call to target_wait,
>   and removes this from prepare_to_wait and handle_inferior_event.

I welcome this bit in particular very much.  I know I was confused
by this when I first learnt about expedite registers.

> * fetch_inferior_event uses the contents of ecs->ptid and
>   ecs->event_thread after handle_inferior_event returns.  As later
>   patches will remove those fields, I've replaced them with accessing
>   the current inferior -- just like normal_stop, which is called
>   by fetch_inferior_event anyway, does as well.

Yes, but not through current_inferior.

On Sunday 07 December 2008 00:17:36, Ulrich Weigand wrote:
>    if (!ecs->wait_some_more)
>      {
> -      struct inferior *inf = find_inferior_pid (ptid_get_pid (ecs->ptid));
> +      struct inferior *inf = current_inferior ();
>  

Please change this bit to:

-      struct inferior *inf = find_inferior_pid (ptid_get_pid (ecs->ptid));
+      struct inferior *inf = find_inferior_pid (ptid_get_pid (inferior_ptid));

Although inferior_ptid is pointing ecs->ptid at this point,
current_inferior asserts if the inferior is not listed, which does happen today.
See this just below:

     /* We may not find an inferior if this was a process exit.  */
      if (inf == NULL || inf->stop_soon == NO_STOP_QUIETLY)
	normal_stop ();

I've got  patches that fix the targets to not do that, so I can
revisit this bit by then.

-- 
Pedro Alves

Re: [rfc] [2/7] infrun cleanup: miscellaneous cleanups

[Pedro Alves]

On Sunday 07 December 2008 01:23:40, Pedro Alves wrote:
> On Sunday 07 December 2008 00:17:36, Ulrich Weigand wrote:
> >    if (!ecs->wait_some_more)
> >      {
> > -      struct inferior *inf = find_inferior_pid (ptid_get_pid (ecs->ptid));
> > +      
> Please change this bit to:
> 
> -      struct inferior *inf = find_inferior_pid (ptid_get_pid (ecs->ptid));
> +      struct inferior *inf = find_inferior_pid (ptid_get_pid (inferior_ptid));

Looking again, it would be more correct (and safest as it doesn't change current
behaviour) to leave it as ptid_get_pid(ecs->ptid) in this patch, and change it
to ptid_get_pid (ptid) in the last patch (the event ptid).

-- 
Pedro Alves

Re: [rfc] [2/7] infrun cleanup: miscellaneous cleanups

[Ulrich Weigand]

Pedro Alves:
> On Sunday 07 December 2008 01:23:40, Pedro Alves wrote:
> > On Sunday 07 December 2008 00:17:36, Ulrich Weigand wrote:
> > >    if (!ecs->wait_some_more)
> > >      {
> > > -      struct inferior *inf =3D find_inferior_pid (ptid_get_pid (ecs->p=
> tid));
> > > +=20=20=20=20=20=20
> > Please change this bit to:
> >=20
> > - =A0 =A0 =A0struct inferior *inf =3D find_inferior_pid (ptid_get_pid (ec=
> s->ptid));
> > + =A0 =A0 =A0struct inferior *inf =3D find_inferior_pid (ptid_get_pid (in=
> ferior_ptid));
> 
> Looking again, it would be more correct (and safest as it doesn't change cu=
> rrent
> behaviour) to leave it as ptid_get_pid(ecs->ptid) in this patch, and change=
>  it
> to ptid_get_pid (ptid) in the last patch (the event ptid).

Well, I was concerned about the cases where today handle_inferior_event
*modifies* the ecs->ptid it gets as input (e.g. thread-hopping or switching
back to the single-stepped thread).  I'm not sure if that can today actually
ever cause it switch to a different *inferior*, but it appears it could.

Using simply the caller's ptid means we will miss that switch.  Using
inferior_ptid should catch it, however, as handle_inferior_event will
have done a switch_to_thread in addition to changing ecs->ptid.

Bye,
Ulrich

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

Re: [rfc] [2/7] infrun cleanup: miscellaneous cleanups

[Pedro Alves]

On Sunday 07 December 2008 18:23:06, Ulrich Weigand wrote:

> Well, I was concerned about the cases where today handle_inferior_event
> *modifies* the ecs->ptid it gets as input (e.g. thread-hopping or switching
> back to the single-stepped thread).  I'm not sure if that can today actually
> ever cause it switch to a different *inferior*, but it appears it could.
> Using simply the caller's ptid means we will miss that switch.  Using
> inferior_ptid should catch it, however, as handle_inferior_event will
> have done a switch_to_thread in addition to changing ecs->ptid.

Do those cases cause handle_inferior_event to return 0?  Notice
that normal_stop uses the last target event (get_last_target_status)
to check if this was an inferior exit, because inferior_ptid will
usually be set to null_ptid on that case, at that point.  But
I do see what you mean.
There are several cases where we don't switch inferior_ptid
to the event ptid, which are wrong in multiprocess.  I just haven't
been afected by them yet (e.g., TARGET_WAITKIND_LOADED,
TARGET_WAITKIND_SYSCALL_ENTRY, new_thread_event), so I didn't
address it.

The issues I was mentioning here in particular were related to
inferior exits.

1) we are not switching inferior_ptid to the event ptid on an inferior
   exit (before calling target_mourn_inferior).  This is of course
   bad in multi-process, as we mourn the wrong inferior.  Furthermore,
   some targets return pid_to_ptid (-1) instead of the pid of the process
   that exited, when returning a TARGET_WAITKING_EXITED / TARGET_WAITKING_SIGNALLED.

2) On an inferior exit, the remote target is deleting the inferior from
   gdb's list even before target_mourn_inferior is called.  Calling
   current_inferior here will assert.

3) most targets' target_mourn_inferior calls generic_mourn_inferior,
   which deletes the dead inferior from gdb's list, and sets inferior_ptid
   to null_ptid.  This is done inside handle_inferior_event, so, on an inferior exit,
   inferior_ptid is most of the times null_ptid in the hunk we're talking about
   in fetch_inferior_event.

I'm going to post patches that address #1 and #2.  Changing
#3 involves further design decisions, so best to leave that out of
this series, IMO.

The 3 points above mean that unconditionally calling
current_inferior in fetch_inferior_event like you were doing asserts.

We could make fetch_inferior_event call normal_stop like:

if (ecs->ws.kind == TARGET_WAITKIND_EXITED
   || ecs->ws.kind == TARGET_WAITKIND_SIGNALLED
   || current_inferior ()->stop_soon == NO_STOP_QUIETLY)
  normal_stop ();

Maybe also add a:
   || ptid_equal (inferior_ptid, null_ptid))

... but that will still not be 100% correct due to the
missing inferior_ptid switches...

-- 
Pedro Alves