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

[Tom Tromey <tromey@redhat.com>]

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