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

[Pedro Alves <palves at redhat dot com>]

On 11/14/2013 09:56 PM, Tom Tromey wrote:
>>>>>> "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.

Sorry, I wasn't clear.  I understand what as a whole the patch is trying
to do; what I don't understand is why a hack was necessary, or its
implementation.  E.g., what exactly fails if the hack is not
in place?; Why this spot for the hack?; What's the predicate used in
the hack actually checking?

Now that I looked again a little closer, I recalled that GDB in sync
mode always outputs a silly extra prompt right after ^running (in response
to execution commands), before the target stops, even though GDB is
not ready for input then.  Guess this hack is related?

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

I guess we could see it either way.  -list-target-features lists
target features.  So with GDB today, until "set target-async on"
is issued, the target doesn't support async.  After the series,
the target does support async even if MI itself isn't async.
E.g., I'd've expected 'interpreter-exec mi "-list-target-features"'
issued from the cli to list "async".

Given the chicken and egg thing already exists today, this makes
me think no frontend is actually looking for this feature... (?)

Anyway, fine with me to leave this as you have it for now, and
maybe reconsider it after the series is in.

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

Yeah, well.  TBC, MI async was not really designed around "set target-async"
specifically.  Async support goes all the way back to 1999.  At the time only
the remote target supported async, but it wasn't actually the remote target.
You'd do "target async" instead of "target remote" (that's still visible in
gdb.base/async.exp).  So by then, the frontend always knew what to expect,
as it was the one who chose whether to async or not.  When we started fixing
async bitrot (and fixing lots of things) almost 10 years later,
"target async" and "target remote" were merged, and a knob added to
enable async-ness (and then GNU/Linux learnt async too).

Fundamentally, in a perfect world, MI should have always just printed
the prompt whenever GDB is ready for further frontend input.  Then, a
frontend that is aware of that, and that understands async (that is,
is aware that gdb may be ready for further input before the target
stops) would be able to cope with sync output too.  But, unfortunately,
in sync mode, GDB prints the prompt too right after sync execution
commands, before the target stops.  :-/  So one could say that even
without async in the picture, the prompt in sync mode has always
been wrong...  In async mode, I don't think MI could ever emulate that,
as the extra prompt would confuse the frontend.  Well, maybe it could,
if it had been defined that after ^running there's always an extra
prompt that should be ignored.  Oh well...

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

Yeah, I recall that.

  https://sourceware.org/ml/gdb-patches/2011-12/msg00810.html

The main issue was that ^C doesn't work for interrupting the
target in async mode (you have to use -exec-interrupt).  But I can
imagine that the frontends would get confused by not seeing the extra
prompt gdb puts out for (no good reason) after resumption commands in
sync mode.

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

But you're dropping the sync path.  How come the test doesn't fail
on all other targets but GNU/Linux|Remote after this?

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

Now I'm _really_ confused.  I (and Vladimir back when he was hacking on
async) have done something like that many times in the past.  You
use a dejagnu board that does something like:

  set GDBFLAGS "${GDBFLAGS} -ex \"set target-async on\""

That's how async mode has been tested so far.  That's why
we call mi_detect_async right after starting gdb.  Before this
series, stuff isn't supposed to be breaking all over with that.
Last I tried you'd only get a few regressions (like the bugs
this series fixes), and I was assuming that you had done that
too, and that at least before patch 9, testing gdb like that
would be regression free compared to a default sync run.

-- 
Pedro Alves