Re: [RFA] fix *stopped for CLI commands

Vladimir Prus
2009-02-12T08:46:09Z

[-- Attachment #1: Type: text/plain, Size: 1625 bytes --]

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

[-- Attachment #2: stopped.diff --]
[-- Type: text/x-diff, Size: 9491 bytes --]

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;
}