Re: [RFA] fix *stopped for CLI commands

[Vladimir Prus <vladimir at codesourcery dot com>]

On Saturday 07 February 2009 00:11:27 Tom Tromey wrote:
> >>>>> "Vladimir" == Vladimir Prus <vladimir@codesourcery.com> writes:
> 
> Vladimir> This patch fixes this by making MI observer print frame
> Vladimir> again, into MI uiout, if necessary. It passes all MI tests
> Vladimir> in (sync,async)x(native,gdbserver) combinations.  How does
> Vladimir> this look, and are non-MI changes OK? If approved, I'll add
> Vladimir> a test that CLI commands result in proper *stopped.
> 
> I don't understand all the implications of the core change, but I do
> like how it moves some MI logic out of the core and into MI.
> 
> Vladimir> -@deftypefun void normal_stop (struct bpstats *@var{bs})
> Vladimir> +@deftypefun void normal_stop (struct bpstats *@var{bs}, int @var{print_frame})
> Vladimir>  The inferior has stopped for real.
> Vladimir>  @end deftypefun
>  
> I would like to ask for documentation describing the meaning of the
> new argument.

The attached patch does so. Eli, can you see if the doc change is fine?

I've also added testcase, as promised. One detail I forgot to say in the
original email is that this patch does not make *stopped emitted for CLI
identical to *stopped for the corresponding MI commands. In particular,
output for CLI does not include 'reason' field sometimes, and for 'finish'
it will not include the return value. I think this is fine -- CLI commands
are meant to be issued by the user in the console, and therefore user will
see the CLI output from such commands. The only purpose of MI *stopped is
to let MI frontend know that the program is not stopped in a different place.

- Volodya

commit 8fb6bdc851dc83207839e5d06c13925746c211f2
Author: Vladimir Prus <vladimir@codesourcery.com>
Date:   Fri Feb 6 10:39:17 2009 +0300

    Include frame information for *stopped due to CLI commands.
    
    	gdb/doc/
    	* observer.texi: Add parameter 'print_frame' to normal_stop
    	observer.
    
    	* ada-tasks.c (ada_normal_stop_observer): Adjust prototype.
    	* infcmd.c (finish_command_continuation): Pass '1' for
    	'print_frame' parameter to the observer.
    	* infrun.c (normal_stop): Don't print mi-specific information
    	here. Pass 'stop_print_frame' to the 'print_frame' parameter
    	of the observer.
    	* mi/mi-interp.c (mi_on_normal_stop): Adjust prototype.
    	If we need to print frame, and current uiout is not the MI one,
    	print frame again.
    
    	gdb/testsuite/
    	* lib/mi-support.exp (mi_expect_stop): Adjust the order of fields.
    	(mi_expect_interrupt): Likewise.
    	* gdb.mi/mi-cli.exp: Check that "step" results in proper *stopped
    	response.

diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
index 5176d75..3639472 100644
--- a/gdb/ada-tasks.c
+++ b/gdb/ada-tasks.c
@@ -951,7 +951,7 @@ ada_task_list_changed (void)
 /* The 'normal_stop' observer notification callback.  */
 
 static void
-ada_normal_stop_observer (struct bpstats *unused_args)
+ada_normal_stop_observer (struct bpstats *unused_args, int unused_args2)
 {
   /* The inferior has been resumed, and just stopped. This means that
      our task_list needs to be recomputed before it can be used again.  */
diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi
index 636658a..7ca1f8a 100644
--- a/gdb/doc/observer.texi
+++ b/gdb/doc/observer.texi
@@ -88,8 +88,12 @@ Send a notification to all @var{event} observers.
 
 The following observable events are defined:
 
-@deftypefun void normal_stop (struct bpstats *@var{bs})
-The inferior has stopped for real.
+@deftypefun void normal_stop (struct bpstats *@var{bs}, int @var{print_frame})
+The inferior has stopped for real.  The  @var{bs} parameter describes
+the breakpoints were are stopped at, if any.  The @var{print_frame}
+parameter indicates whether the @value{GDBN} core suggests that the
+location where the inferiour has stopped is known and should be
+reported to the user.
 @end deftypefun
 
 @deftypefun void target_changed (struct target_ops *@var{target})
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 3696f79..188077c 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1374,7 +1374,7 @@ finish_command_continuation (void *arg)
      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);
+  observer_notify_normal_stop (bs, 1 /* print frame */);
   suppress_stop_observer = 0;
   delete_breakpoint (a->breakpoint);
 }
diff --git a/gdb/infrun.c b/gdb/infrun.c
index c11c71a..e50ef49 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4351,22 +4351,6 @@ Further execution is probably impossible.\n"));
 	      internal_error (__FILE__, __LINE__, _("Unknown value."));
 	    }
 
-	  if (ui_out_is_mi_like_p (uiout))
-	    {
-
-	      ui_out_field_int (uiout, "thread-id",
-				pid_to_thread_id (inferior_ptid));
-	      if (non_stop)
-		{
-		  struct cleanup *back_to = make_cleanup_ui_out_list_begin_end 
-		    (uiout, "stopped-threads");
-		  ui_out_field_int (uiout, NULL,
-				    pid_to_thread_id (inferior_ptid));		  		  
-		  do_cleanups (back_to);
-		}
-	      else
-		ui_out_field_string (uiout, "stopped-threads", "all");
-	    }
 	  /* The behavior of this routine with respect to the source
 	     flag is:
 	     SRC_LINE: Print only source line
@@ -4421,9 +4405,10 @@ done:
 	   && inferior_thread ()->step_multi))
     {
       if (!ptid_equal (inferior_ptid, null_ptid))
-	observer_notify_normal_stop (inferior_thread ()->stop_bpstat);
+	observer_notify_normal_stop (inferior_thread ()->stop_bpstat,
+				     stop_print_frame);
       else
-	observer_notify_normal_stop (NULL);
+	observer_notify_normal_stop (NULL, stop_print_frame);
     }
 
   if (target_has_execution)
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 8e2b230..7f52549 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -52,7 +52,7 @@ static void mi1_command_loop (void);
 
 static void mi_insert_notify_hooks (void);
 static void mi_remove_notify_hooks (void);
-static void mi_on_normal_stop (struct bpstats *bs);
+static void mi_on_normal_stop (struct bpstats *bs, int print_frame);
 
 static void mi_new_thread (struct thread_info *t);
 static void mi_thread_exit (struct thread_info *t);
@@ -322,17 +322,46 @@ mi_inferior_exit (int pid)
 }
 
 static void
-mi_on_normal_stop (struct bpstats *bs)
+mi_on_normal_stop (struct bpstats *bs, int print_frame)
 {
   /* Since this can be called when CLI command is executing,
      using cli interpreter, be sure to use MI uiout for output,
      not the current one.  */
-  struct ui_out *uiout = interp_ui_out (top_level_interpreter ());
+  struct ui_out *mi_uiout = interp_ui_out (top_level_interpreter ());
   struct mi_interp *mi = top_level_interpreter_data ();
 
+  if (print_frame)
+    {
+      if (uiout != mi_uiout)
+	{
+	  /* The normal_stop function has printed frame information into 
+	     CLI uiout, or some other non-MI uiout.  There's no way we
+	     can extra proper fields from random uiout object, so we print
+	     the frame again.  In practice, this can only happen when running
+	     a CLI command in MI.  */
+	  struct ui_out *saved_uiout = uiout;
+	  uiout = mi_uiout;
+	  print_stack_frame (get_selected_frame (NULL), 0, SRC_AND_LOC);
+	  uiout = saved_uiout;
+	}
+
+      ui_out_field_int (mi_uiout, "thread-id",
+			pid_to_thread_id (inferior_ptid));
+      if (non_stop)
+	{
+	  struct cleanup *back_to = make_cleanup_ui_out_list_begin_end 
+	    (mi_uiout, "stopped-threads");
+	  ui_out_field_int (mi_uiout, NULL,
+			    pid_to_thread_id (inferior_ptid));		  		  
+	  do_cleanups (back_to);
+	}
+      else
+	ui_out_field_string (mi_uiout, "stopped-threads", "all");
+    }
+  
   fputs_unfiltered ("*stopped", raw_stdout);
-  mi_out_put (uiout, raw_stdout);
-  mi_out_rewind (uiout);
+  mi_out_put (mi_uiout, raw_stdout);
+  mi_out_rewind (mi_uiout);
   fputs_unfiltered ("\n", raw_stdout);
   gdb_flush (raw_stdout);
 }
diff --git a/gdb/testsuite/gdb.mi/mi-cli.exp b/gdb/testsuite/gdb.mi/mi-cli.exp
index b77a4d4..b469acb 100644
--- a/gdb/testsuite/gdb.mi/mi-cli.exp
+++ b/gdb/testsuite/gdb.mi/mi-cli.exp
@@ -140,6 +140,9 @@ mi_gdb_test "500-stack-select-frame 0" \
   {500\^done} \
   "-stack-select-frame 0"
 
+mi_execute_to "interpreter-exec console step" "" "callee4" "" ".*basics.c" "29" \
+    "" "check *stopped from CLI command"
+
 # NOTE: cagney/2003-02-03: Not yet.
 # mi_gdb_test "-break-insert -t basics.c:$line_main_hello" \
 #   {.*=breakpoint-create,number="3".*\^done} \
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index be9b530..f62c240 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -1020,13 +1020,13 @@ proc mi_expect_stop { reason func args file line extra test } {
 
     set any "\[^\n\]*"
 
-    verbose -log "mi_expect_stop: expecting: \\*stopped,${r}${a}${bn}thread-id=\"$decimal\",stopped-threads=$any,frame=\{addr=\"$hex\",func=\"$func\",args=$args,file=\"$any$file\",fullname=\"${fullname_syntax}$file\",line=\"$line\"\}$after_stopped\r\n($thread_selected_re)?$prompt_re"
+    verbose -log "mi_expect_stop: expecting: \\*stopped,${r}${a}${bn}frame=\{addr=\"$hex\",func=\"$func\",args=$args,file=\"$any$file\",fullname=\"${fullname_syntax}$file\",line=\"$line\"\}$after_stopped,thread-id=\"$decimal\",stopped-threads=$any\r\n($thread_selected_re)?$prompt_re"
     gdb_expect {
-	-re "\\*stopped,${r}${a}${bn}thread-id=\"$decimal\",stopped-threads=$any,frame=\{addr=\"$hex\",func=\"$func\",args=$args,file=\"$any$file\",fullname=\"${fullname_syntax}$file\",line=\"($line)\"\}$after_stopped\r\n($thread_selected_re)?$prompt_re" {
+	-re "\\*stopped,${r}${a}${bn}frame=\{addr=\"$hex\",func=\"$func\",args=$args,file=\"$any$file\",fullname=\"${fullname_syntax}$file\",line=\"($line)\"\}$after_stopped,thread-id=\"$decimal\",stopped-threads=$any\r\n($thread_selected_re)?$prompt_re" {
 	    pass "$test"
             return $expect_out(2,string)
 	}
-	-re "\\*stopped,${r}${a}${bn}thread-id=\"$decimal\",stopped-threads=$any,frame=\{addr=\"$hex\",func=\"$any\",args=\[\\\[\{\]$any\[\\\]\}\],file=\"$any\",fullname=\"${fullname_syntax}$any\",line=\"\[0-9\]*\"\}$any\r\n$prompt_re" {
+	-re "\\*stopped,${r}${a}${bn}frame=\{addr=\"$hex\",func=\"$any\",args=\[\\\[\{\]$any\[\\\]\}\],file=\"$any\",fullname=\"${fullname_syntax}$any\",line=\"\[0-9\]*\"\}thread-id=\"$decimal\",stopped-threads=$any\r\n$prompt_re" {
             verbose -log "got $expect_out(buffer)"
 	    fail "$test (stopped at wrong place)"
 	    return -1
@@ -1061,9 +1061,9 @@ proc mi_expect_interrupt { test } {
     set any "\[^\n\]*"
 
     # A signal can land anywhere, just ignore the location
-    verbose -log "mi_expect_interrupt: expecting: \\*stopped,${r},thread-id=\"$decimal\",stopped-threads=$any\r\n$prompt_re"
+    verbose -log "mi_expect_interrupt: expecting: \\*stopped,${r}$any\r\n$prompt_re"
     gdb_expect {
-	-re "\\*stopped,${r},thread-id=\"$decimal\",stopped-threads=$any\r\n$prompt_re" {
+	-re "\\*stopped,${r}$any\r\n$prompt_re" {
 	    pass "$test"
 	    return 0;
 	}