[1/3] broken -thread-info output in non-stop mode, suppress_stop_observer

[1/3] broken -thread-info output in non-stop mode, suppress_stop_observer

[Pedro Alves]

This patch removes the suppress_stop_observer global.  As indicated
in the comments I'm removing, this was problematic for non-stop
mode.

To fix the broken -thread-info output mentioned here:

 http://sourceware.org/ml/gdb-patches/2009-03/msg00236.html

... I wanted to add a thread_info->in_infcall flag to use in
the 3rd patch in the series.  That meant I needed to get rid of
the suppress_resume_observer global, which then brings
us to this.

Vladimir, et al, how does this look?

The whole series was tested on x86_64-linux, sync/async.

-- 
Pedro Alves

2009-03-17  Pedro Alves  <pedro@codesourcery.com>

	* infcall.c (run_inferior_call): Remove references to
	suppress_stop_observer.
	* infcmd.c (suppress_stop_observer): Delete.
	(finish_command_continuation): Remove NOTE.  Don't clear
	suppress_stop_observer anymore.
	(finish_command_continuation_free_arg): Likewise.
	(finish_forward): Remove references to suppress_stop_observer.
	Call normal_stop observer if we haven't already.
	* inferior.h (suppress_stop_observer): Delete.
	* infrun.c (normal_stop): When deciding to suppress the
	normal_stop observer, check for proceed_to_finish instead of
	suppress_stop_observer.

---
 gdb/infcall.c  |    3 ---
 gdb/infcmd.c   |   24 ++++++++----------------
 gdb/inferior.h |    4 ----
 gdb/infrun.c   |   15 ++++++++++-----
 4 files changed, 18 insertions(+), 28 deletions(-)

Index: src/gdb/infcall.c
===================================================================
--- src.orig/gdb/infcall.c	2009-03-17 05:25:17.000000000 +0000
+++ src/gdb/infcall.c	2009-03-17 05:27:45.000000000 +0000
@@ -331,7 +331,6 @@ run_inferior_call (struct thread_info *c
   volatile struct gdb_exception e;
   int saved_async = 0;
   int saved_suppress_resume_observer = suppress_resume_observer;
-  int saved_suppress_stop_observer = suppress_stop_observer;
   ptid_t call_thread_ptid = call_thread->ptid;
   char *saved_target_shortname = xstrdup (target_shortname);
 
@@ -344,7 +343,6 @@ run_inferior_call (struct thread_info *c
     saved_async = target_async_mask (0);
 
   suppress_resume_observer = 1;
-  suppress_stop_observer = 1;
 
   TRY_CATCH (e, RETURN_MASK_ALL)
     proceed (real_pc, TARGET_SIGNAL_0, 0);
@@ -355,7 +353,6 @@ run_inferior_call (struct thread_info *c
   call_thread = NULL;
 
   suppress_resume_observer = saved_suppress_resume_observer;
-  suppress_stop_observer = saved_suppress_stop_observer;
 
   /* Don't restore the async mask if the target has changed,
      saved_async is for the original target.  */
Index: src/gdb/infcmd.c
===================================================================
--- src.orig/gdb/infcmd.c	2009-03-17 05:25:19.000000000 +0000
+++ src/gdb/infcmd.c	2009-03-17 05:49:08.000000000 +0000
@@ -169,8 +169,6 @@ struct gdb_environ *inferior_environ;
 
 /* When set, no calls to target_resumed observer will be made.  */
 int suppress_resume_observer = 0;
-/* When set, normal_stop will not call the normal_stop observer.  */
-int suppress_stop_observer = 0;
 \f
 /* Accessor routines. */
 
@@ -1346,13 +1344,16 @@ static void
 finish_command_continuation (void *arg)
 {
   struct finish_command_continuation_args *a = arg;
-
+  struct thread_info *tp = NULL;
   bpstat bs = NULL;
 
   if (!ptid_equal (inferior_ptid, null_ptid)
       && target_has_execution
       && is_stopped (inferior_ptid))
-    bs = inferior_thread ()->stop_bpstat;
+    {
+      tp = inferior_thread ();
+      bs = tp->stop_bpstat;
+    }
 
   if (bpstat_find_breakpoint (bs, a->breakpoint) != NULL
       && a->function != NULL)
@@ -1369,22 +1370,15 @@ finish_command_continuation (void *arg)
     }
 
   /* We suppress normal call of normal_stop observer and do it here so
-     that that *stopped notification includes the return value.  */
-  /* NOTE: This is broken in non-stop mode.  There is no guarantee the
-     next stop will be in the same thread that we started doing a
-     finish on.  This suppressing (or some other replacement means)
-     should be a thread property.  */
-  observer_notify_normal_stop (bs, 1 /* print frame */);
-  suppress_stop_observer = 0;
+     that the *stopped notification includes the return value.  */
+  if (bs != NULL && tp->proceed_to_finish)
+    observer_notify_normal_stop (bs, 1 /* print frame */);
   delete_breakpoint (a->breakpoint);
 }
 
 static void
 finish_command_continuation_free_arg (void *arg)
 {
-  /* NOTE: See finish_command_continuation.  This would go away, if
-     this suppressing is made a thread property.  */
-  suppress_stop_observer = 0;
   xfree (arg);
 }
 
@@ -1469,8 +1463,6 @@ finish_forward (struct symbol *function,
   old_chain = make_cleanup_delete_breakpoint (breakpoint);
 
   tp->proceed_to_finish = 1;    /* We want stop_registers, please...  */
-  make_cleanup_restore_integer (&suppress_stop_observer);
-  suppress_stop_observer = 1;
   proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 0);
 
   cargs = xmalloc (sizeof (*cargs));
Index: src/gdb/inferior.h
===================================================================
--- src.orig/gdb/inferior.h	2009-03-17 05:25:19.000000000 +0000
+++ src/gdb/inferior.h	2009-03-17 05:27:45.000000000 +0000
@@ -368,10 +368,6 @@ extern int debug_displaced;
 void displaced_step_dump_bytes (struct ui_file *file,
                                 const gdb_byte *buf, size_t len);
 
-
-/* When set, normal_stop will not call the normal_stop observer.  */
-extern int suppress_stop_observer;
-
 /* When set, no calls to target_resumed observer will be made.  */
 extern int suppress_resume_observer;
 
Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2009-03-17 05:25:21.000000000 +0000
+++ src/gdb/infrun.c	2009-03-17 05:47:32.000000000 +0000
@@ -4433,11 +4433,16 @@ Further execution is probably impossible
 
 done:
   annotate_stopped ();
-  if (!suppress_stop_observer
-      && !(target_has_execution
-	   && last.kind != TARGET_WAITKIND_SIGNALLED
-	   && last.kind != TARGET_WAITKIND_EXITED
-	   && inferior_thread ()->step_multi))
+
+  /* Suppress the stop observer if we're in the middle of a step n (n
+     > 1), doing a "finish" command, or returning from an inferior
+     function call.  */
+  if (!target_has_execution
+      || last.kind == TARGET_WAITKIND_SIGNALLED
+      || last.kind == TARGET_WAITKIND_EXITED
+      || (!inferior_thread ()->step_multi
+	  && !(inferior_thread ()->stop_bpstat
+	       && inferior_thread ()->proceed_to_finish))
     {
       if (!ptid_equal (inferior_ptid, null_ptid))
 	observer_notify_normal_stop (inferior_thread ()->stop_bpstat,

Re: [1/3] broken -thread-info output in non-stop mode, suppress_stop_observer

[Vladimir Prus]

On Tuesday 17 March 2009 09:21:02 Pedro Alves wrote:
> This patch removes the suppress_stop_observer global.  As indicated
> in the comments I'm removing, this was problematic for non-stop
> mode.
>
> To fix the broken -thread-info output mentioned here:
>
>  http://sourceware.org/ml/gdb-patches/2009-03/msg00236.html
>
> ... I wanted to add a thread_info->in_infcall flag to use in
> the 3rd patch in the series.  That meant I needed to get rid of
> the suppress_resume_observer global, which then brings
> us to this.
>
> Vladimir, et al, how does this look?

I did not look very carefully at details of conditions, but the
general idea looks solid. One detail is the new command you
add in infrun.c -- can it explicitly write down *why* we need to
suppress *stopped at this point, when doing finish?

Thanks,
Volodya

Re: [1/3] broken -thread-info output in non-stop mode, suppress_stop_observer

[Pedro Alves]

On Tuesday 17 March 2009 07:26:29, Vladimir Prus wrote:
> On Tuesday 17 March 2009 09:21:02 Pedro Alves wrote:
> > This patch removes the suppress_stop_observer global.  As indicated
> > in the comments I'm removing, this was problematic for non-stop
> > mode.
> >
> > To fix the broken -thread-info output mentioned here:
> >
> >  http://sourceware.org/ml/gdb-patches/2009-03/msg00236.html
> >
> > ... I wanted to add a thread_info->in_infcall flag to use in
> > the 3rd patch in the series.  That meant I needed to get rid of
> > the suppress_resume_observer global, which then brings
> > us to this.
> >
> > Vladimir, et al, how does this look?
> 
> I did not look very carefully at details of conditions, but the
> general idea looks solid. One detail is the new command you
> add in infrun.c -- can it explicitly write down *why* we need to
> suppress *stopped at this point, when doing finish?

Certainly, here it is.  Thank you.

Hmmm, I do wonder if we could rewrite "finish" to not rely on 
bpstat_find_breakpoint after normal_stop, but to instead add a new
BPSTAT_WHAT_FINISH and handle it from within handle_inferior_event's
internal breakpoint handling switch (where we handle
BPSTAT_WHAT_STEP_RESUME, etc.).  Since that runs before normal_stop,
we wouldn't have to suppress the normal_stop observer.

?

-- 
Pedro Alves

2009-03-17  Pedro Alves  <pedro@codesourcery.com>

	* infcall.c (run_inferior_call): Remove references to
	suppress_stop_observer.
	* infcmd.c (suppress_stop_observer): Delete.
	(finish_command_continuation): Remove NOTE.  Don't clear
	suppress_stop_observer anymore.
	(finish_command_continuation_free_arg): Likewise.
	(finish_forward): Remove references to suppress_stop_observer.
	Call normal_stop observer if we haven't already.
	* inferior.h (suppress_stop_observer): Delete.
	* infrun.c (normal_stop): When deciding to suppress the
	normal_stop observer, check for proceed_to_finish instead of
	suppress_stop_observer.

---
 gdb/infcall.c  |    3 ---
 gdb/infcmd.c   |   24 ++++++++----------------
 gdb/inferior.h |    4 ----
 gdb/infrun.c   |   24 +++++++++++++++++++-----
 4 files changed, 27 insertions(+), 28 deletions(-)

Index: src/gdb/infcall.c
===================================================================
--- src.orig/gdb/infcall.c	2009-03-17 05:25:17.000000000 +0000
+++ src/gdb/infcall.c	2009-03-17 16:14:06.000000000 +0000
@@ -331,7 +331,6 @@ run_inferior_call (struct thread_info *c
   volatile struct gdb_exception e;
   int saved_async = 0;
   int saved_suppress_resume_observer = suppress_resume_observer;
-  int saved_suppress_stop_observer = suppress_stop_observer;
   ptid_t call_thread_ptid = call_thread->ptid;
   char *saved_target_shortname = xstrdup (target_shortname);
 
@@ -344,7 +343,6 @@ run_inferior_call (struct thread_info *c
     saved_async = target_async_mask (0);
 
   suppress_resume_observer = 1;
-  suppress_stop_observer = 1;
 
   TRY_CATCH (e, RETURN_MASK_ALL)
     proceed (real_pc, TARGET_SIGNAL_0, 0);
@@ -355,7 +353,6 @@ run_inferior_call (struct thread_info *c
   call_thread = NULL;
 
   suppress_resume_observer = saved_suppress_resume_observer;
-  suppress_stop_observer = saved_suppress_stop_observer;
 
   /* Don't restore the async mask if the target has changed,
      saved_async is for the original target.  */
Index: src/gdb/infcmd.c
===================================================================
--- src.orig/gdb/infcmd.c	2009-03-17 05:25:19.000000000 +0000
+++ src/gdb/infcmd.c	2009-03-17 16:14:06.000000000 +0000
@@ -169,8 +169,6 @@ struct gdb_environ *inferior_environ;
 
 /* When set, no calls to target_resumed observer will be made.  */
 int suppress_resume_observer = 0;
-/* When set, normal_stop will not call the normal_stop observer.  */
-int suppress_stop_observer = 0;
 \f
 /* Accessor routines. */
 
@@ -1346,13 +1344,16 @@ static void
 finish_command_continuation (void *arg)
 {
   struct finish_command_continuation_args *a = arg;
-
+  struct thread_info *tp = NULL;
   bpstat bs = NULL;
 
   if (!ptid_equal (inferior_ptid, null_ptid)
       && target_has_execution
       && is_stopped (inferior_ptid))
-    bs = inferior_thread ()->stop_bpstat;
+    {
+      tp = inferior_thread ();
+      bs = tp->stop_bpstat;
+    }
 
   if (bpstat_find_breakpoint (bs, a->breakpoint) != NULL
       && a->function != NULL)
@@ -1369,22 +1370,15 @@ finish_command_continuation (void *arg)
     }
 
   /* We suppress normal call of normal_stop observer and do it here so
-     that that *stopped notification includes the return value.  */
-  /* NOTE: This is broken in non-stop mode.  There is no guarantee the
-     next stop will be in the same thread that we started doing a
-     finish on.  This suppressing (or some other replacement means)
-     should be a thread property.  */
-  observer_notify_normal_stop (bs, 1 /* print frame */);
-  suppress_stop_observer = 0;
+     that the *stopped notification includes the return value.  */
+  if (bs != NULL && tp->proceed_to_finish)
+    observer_notify_normal_stop (bs, 1 /* print frame */);
   delete_breakpoint (a->breakpoint);
 }
 
 static void
 finish_command_continuation_free_arg (void *arg)
 {
-  /* NOTE: See finish_command_continuation.  This would go away, if
-     this suppressing is made a thread property.  */
-  suppress_stop_observer = 0;
   xfree (arg);
 }
 
@@ -1469,8 +1463,6 @@ finish_forward (struct symbol *function,
   old_chain = make_cleanup_delete_breakpoint (breakpoint);
 
   tp->proceed_to_finish = 1;    /* We want stop_registers, please...  */
-  make_cleanup_restore_integer (&suppress_stop_observer);
-  suppress_stop_observer = 1;
   proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 0);
 
   cargs = xmalloc (sizeof (*cargs));
Index: src/gdb/inferior.h
===================================================================
--- src.orig/gdb/inferior.h	2009-03-17 05:25:19.000000000 +0000
+++ src/gdb/inferior.h	2009-03-17 16:14:06.000000000 +0000
@@ -368,10 +368,6 @@ extern int debug_displaced;
 void displaced_step_dump_bytes (struct ui_file *file,
                                 const gdb_byte *buf, size_t len);
 
-
-/* When set, normal_stop will not call the normal_stop observer.  */
-extern int suppress_stop_observer;
-
 /* When set, no calls to target_resumed observer will be made.  */
 extern int suppress_resume_observer;
 
Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2009-03-17 05:25:21.000000000 +0000
+++ src/gdb/infrun.c	2009-03-17 16:24:17.000000000 +0000
@@ -4433,11 +4433,25 @@ Further execution is probably impossible
 
 done:
   annotate_stopped ();
-  if (!suppress_stop_observer
-      && !(target_has_execution
-	   && last.kind != TARGET_WAITKIND_SIGNALLED
-	   && last.kind != TARGET_WAITKIND_EXITED
-	   && inferior_thread ()->step_multi))
+
+  /* Suppress the stop observer if we're in the middle of:
+
+     - a step n (n > 1), as there still more steps to be done.
+
+     - a "finish" command, as the observer will be called in
+       finish_command_continuation, so it can include the inferior
+       function's return value.
+
+     - calling an inferior function, as we pretend we inferior didn't
+       run at all.  The return value of the call is handled by the
+       expression evaluator, through call_function_by_hand.  */
+
+  if (!target_has_execution
+      || last.kind == TARGET_WAITKIND_SIGNALLED
+      || last.kind == TARGET_WAITKIND_EXITED
+      || (!inferior_thread ()->step_multi
+	  && !(inferior_thread ()->stop_bpstat
+	       && inferior_thread ()->proceed_to_finish)))
     {
       if (!ptid_equal (inferior_ptid, null_ptid))
 	observer_notify_normal_stop (inferior_thread ()->stop_bpstat,

Re: [1/3] broken -thread-info output in non-stop mode, suppress_stop_observer

[Joel Brobecker]

> Hmmm, I do wonder if we could rewrite "finish" to not rely on 
> bpstat_find_breakpoint after normal_stop, but to instead add a new
> BPSTAT_WHAT_FINISH and handle it from within handle_inferior_event's
> internal breakpoint handling switch (where we handle
> BPSTAT_WHAT_STEP_RESUME, etc.).  Since that runs before normal_stop,
> we wouldn't have to suppress the normal_stop observer.

Seems like worth a try indeed. It might simplify how the "finish"
command is implemented too.

-- 
Joel

Re: [1/3] broken -thread-info output in non-stop mode, suppress_stop_observer

[Pedro Alves]

I've checked in the 3 patches in the series.

-- 
Pedro Alves