This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v4 9/9] enable target-async
- From: Tom Tromey <tromey at redhat dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 14 Nov 2013 14:56:09 -0700
- Subject: Re: [PATCH v4 9/9] enable target-async
- Authentication-results: sourceware.org; auth=none
- References: <1382464769-2465-1-git-send-email-tromey at redhat dot com> <1382464769-2465-10-git-send-email-tromey at redhat dot com> <52828856 dot 9070904 at redhat dot 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