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

Tom Tromey
2013-11-14T21:56:20Z

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> static int
>> mi_interpreter_prompt_p (void *data)
>> {

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

Yeah, this is the biggest hack.
I will try to comment it some more.

The fundamental issue here I ran into is that MI is very odd about when
it prints a prompt. So, the hacks are needed to keep the behavior
consistent -- even though, IMNSHO, the behavior doesn't actually make
any sense.

I don't even see why MI needs a prompt, but of course that can't be
addressed until someone wants to roll out MI3.

>> 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");

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

That is what I thought, too, but IIRC if one changes this, then a test
will fail.

Also it is consistent with what gdb does today:

(gdb)
-list-target-features
^done,features=[]
(gdb)
set target-async on
&"set target-async on\n"
^done
(gdb)
-list-target-features
^done,features=["async"]
(gdb)

Strange but true. Actually I think this is symptomatic of the general
issue where MI paid attention to "set target-async", whereas I think in
a clean design it would not.

>> -# 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"

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

Not sure if you remember the story.

When I started this project I was working under the belief that "set
target-async" was a "please enable a feature" sort of option -- that is,
it ought to have no user visible effect other than making the "&"
feature available; and as such I could simply enable it always, fix the
test suite failures, and deprecate the option.

However, it turns out that this model did not fit the reality. MI used
the target-async setting not just to put the target into async mode and
to enable the "&" feature, but also to change its output style in
various spots.

There's a thread you can dig up where Marc Khouzam says they changed
Eclipse to disable target-async explicitly, just to work around the
oddities that ensued.

For this test case the check may in fact be irrelevant, since we aren't
enabling target-async. However if that is so, we might as well drop it
anyway on account of clarity.

Or maybe this is intended to support running the test suite with some
pre-canned MI sequence to enable target async. I would guess nobody
ever does this, since I think when I tried something like this (naively
setting target_async_permitted = 1), stuff broke all over. Which is
apparently intentional.

Tom