Re: [PATCH v4 9/9] enable target-async

Pedro Alves
2013-11-12T19:58:25Z

Hi Tom,

still not a full review, but I thought I'd send out a
couple comments as I have them.

On 10/22/2013 06:59 PM, Tom Tromey wrote:
> /* Never display the default GDB prompt in MI case. */
>
> static int
> mi_interpreter_prompt_p (void *data)
> {

Looks quite odd for a predicate function to actually have
side effects. I guess this is the hack you mentioned?
I think this is missing a comment explaining what is
going on. It's not obvious at all to me.

> + if (!interp_quiet_p (NULL))
> + {
> + if (!target_is_async_p ()
> + || (!sync_execution
> + && (!target_async_permitted
> + || iterate_over_threads (thread_command_not_mi,
> + NULL) == NULL)))
> + {
> + fputs_unfiltered ("(gdb) \n", raw_stdout);
> + gdb_flush (raw_stdout);
> + }
> + }
> +
> return 0;
> }

> @@ -1837,7 +1851,7 @@ mi_cmd_list_target_features (char *command, char **argv, int argc)
> struct ui_out *uiout = current_uiout;
>
> cleanup = make_cleanup_ui_out_list_begin_end (uiout, "features");
> - if (target_can_async_p ())
> + if (mi_target_can_async_p ())
> ui_out_field_string (uiout, NULL, "async");
> if (target_can_execute_reverse)
> ui_out_field_string (uiout, NULL, "reverse");

Hmm, not sure this is right. This supposedly returns the set of
supported features. But mi_target_can_async_p returns false
until the frontend enables target-async. So this change creates
a sort of chicken and egg situation.

> --- a/gdb/testsuite/gdb.mi/mi-cli.exp
> +++ b/gdb/testsuite/gdb.mi/mi-cli.exp
> @@ -134,20 +134,7 @@ mi_gdb_test "500-stack-select-frame 0" \
> {500\^done} \
> "-stack-select-frame 0"
>
> -# When a CLI command is entered in MI session, the respose is different in
> -# sync and async modes. In sync mode normal_stop is called when current
> -# interpreter is CLI. So:
> -# - print_stop_reason prints stop reason in CLI uiout, and we don't show it
> -# in MI
> -# - The stop position is printed, and appears in MI 'console' channel.
> -#
> -# In async mode the stop event is processed when we're back to MI interpreter,
> -# so the stop reason is printed into MI uiout an.
> -if {$async} {
> - set reason "end-stepping-range"
> -} else {
> - set reason ""
> -}
> +set reason "end-stepping-range"

I'm a little confused by this one. Isn't it still necessary
for targets that don't do async?

--
Pedro Alves