mi_async_p: Use the default run target (PR gdb/18077)

  When using -exec-run in mi-async mode on a fresh gdb launch, we can see
that it is not actually done asynchronously.

The problem is that when we issue -exec-run, the linux native target is
not pushed yet. So when the code in mi_cmd_exec-run checks if we support
async (by calling mi_async_p), tdefault_can_async_p from the dummy
target answers 0.

I am not certain of the conceptual correctness of this solution, but it
seems to work. It changes mi_async_p so that it uses find_run_target()
instead of using the current_target. When -exec-run is used before the
native target is pushed, mi_async_p will now report that the target that
will eventually be used for running supports async, instead of saying
that the current target (dummy) does not.

I added a small testcase that I copied from mi-async.exp. Please
indicate if you think it should be integrated to an existing test rather
than in a new test.

I have two questions regarding the test:

 - Why do we have mi_expect_stop and mi_expect_interrupt? It seems like
   the functionality of _interrupt could be integrated in _stop.
 - The signal reported when interrupting a thread changes when in non-stop vs all-stop:

   non-stop: *stopped,reason="signal-received",signal-name="0",signal-meaning="Signal 0",..
   all-stop: *stopped,reason="signal-received",signal-name="SIGINT",signal-meaning="Interrupt",...

   As a consequence, mi_expect_interrupt only works with non-stop.

gdb/ChangeLog:

	* mi/mi-main.c (mi_async_p): Use find_run_target to check for async
	support.

gdb/testsuite/ChangeLog:

	* gdb.mi/mi-async-run.c: New file.
	* gdb.mi/mi-async-run.exp: New file.
---
 gdb/mi/mi-main.c                      |  9 +++++-
 gdb/testsuite/gdb.mi/mi-async-run.c   | 30 ++++++++++++++++++
 gdb/testsuite/gdb.mi/mi-async-run.exp | 60 +++++++++++++++++++++++++++++++++++
 3 files changed, 98 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi-async-run.c
 create mode 100644 gdb/testsuite/gdb.mi/mi-async-run.exp
  

On 03/03/2015 09:45 PM, Simon Marchi wrote:
> When using -exec-run in mi-async mode on a fresh gdb launch, we can see
> that it is not actually done asynchronously.
> 
> The problem is that when we issue -exec-run, the linux native target is
> not pushed yet. So when the code in mi_cmd_exec-run checks if we support
> async (by calling mi_async_p), tdefault_can_async_p from the dummy
> target answers 0.
> 
> I am not certain of the conceptual correctness of this solution, but it
> seems to work. It changes mi_async_p so that it uses find_run_target()
> instead of using the current_target. When -exec-run is used before the
> native target is pushed, mi_async_p will now report that the target that
> will eventually be used for running supports async, instead of saying
> that the current target (dummy) does not.

This is not correct.  E.g., when some target is already pushed,
and it's one that does support async, but can't "run", in other places
that we use mi_async_p we should be consulting the already connected
target, not fallback to the run target.
Please make sure to test with native and gdbserver in both
remote and extended remote modes, to cover different modes of
operation, though you're likely not seeing an issue with
"target remote", which does not support "run", just because that
does implement t->to_create_inferior, but that's for
extended-remote, really (see find_run_target).

I think we need to make run_one_inferior itself check whether the
run target can async, instead of using mi_async_p() there.  Likewise
for

Note that the "mi_async && target_can_async_p()" checks intend to
mimic GDB's behavior before target-async was the default.  In order
gdb's, if you did "set target-async on" and then
-exec-run/continue/step/whatever, gdb would just ignore the target-async
request.  This is actually documented:

 On some targets, @value{GDBN} is capable of processing MI commands
 even while the target is running.  This is called @dfn{asynchronous
 command execution} (@pxref{Background Execution}).  The frontend may
 specify a preferrence for asynchronous execution using the
 @code{-gdb-set mi-async 1} command, which should be emitted before
 either running the executable or attaching to the target.  After the
 frontend has started the executable or attached to the target, it can
 find if asynchronous execution is enabled using the
 @code{-list-target-features} command.

I think it'd be cleaner if when "set mi-async on" (the new spelling of
"set target-async on", it's just an alias) is in effect with a target
that really CANNOT async, we'd still make -exec-run try to run
in async mode, which would error out just like if the user does
"run&" on such a target, but alas, that's not how previous gdb
releases worked.  We'd need to hear from frontend developers whether
that's a desirable change.

> 
> I added a small testcase that I copied from mi-async.exp. Please
> indicate if you think it should be integrated to an existing test rather
> than in a new test.
> 
> I have two questions regarding the test:
> 
>  - Why do we have mi_expect_stop and mi_expect_interrupt? It seems like
>    the functionality of _interrupt could be integrated in _stop.

So we can cope with differences like: ...

>  - The signal reported when interrupting a thread changes when in non-stop vs all-stop:
> 
>    non-stop: *stopped,reason="signal-received",signal-name="0",signal-meaning="Signal 0",..
>    all-stop: *stopped,reason="signal-received",signal-name="SIGINT",signal-meaning="Interrupt",...

... these?

> 
>    As a consequence, mi_expect_interrupt only works with non-stop.

Could you fix it?  I think expecting both signals would be fine.

> +#include <unistd.h>
> +
> +int main ()

int
main (void)


> +# The purpose of this test if to verify that -exec-run with mi-async on
> +# results in asynchronous execution (PR 18077).
> +
> +# The plan is for async mode to become the default but toggle for now.
> +set saved_gdbflags $GDBFLAGS
> +set GDBFLAGS [concat $GDBFLAGS " -ex \"set mi-async on\""]
> +

Can you make this a regular "-gdb-set mi-async", after GDB is
started, please?  (You'll then need to call mi_detect_async too.)

> +load_lib mi-support.exp
> +
> +gdb_exit
> +if [mi_gdb_start] {
> +    continue
> +}
> +
> +standard_testfile
> +
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
> +     untested mi-async.exp
> +     return -1
> +}
> +
> +mi_delete_breakpoints
> +mi_gdb_reinitialize_dir $srcdir/$subdir
> +mi_gdb_load ${binfile}
> +
> +# Necessary for mi_expect_interrupt to work, as the reported signal is not the
> +# same in all-stop.
> +mi_gdb_test "-gdb-set non-stop 1" ".*"
> +
> +proc linux_async_run_test {} {

Drop the "linux" prefix, please.

> +    global mi_gdb_prompt
> +    global hex

These don't appear to be used.

> +
> +    mi_run_cmd
> +    mi_gdb_test "123-exec-interrupt --all" "123\\^done" "send interrupt command"
> +    mi_expect_interrupt "expect interrupt"
> +}
> +
> +linux_async_run_test
> +
> +mi_gdb_exit
> +
> +set GDBFLAGS $saved_gdbflags
> +
> +return 0
> 

Thanks,
Pedro Alves
  

Hi Pedro,

On 03/04/2015 05:18 AM, Pedro Alves wrote:
> On 03/03/2015 09:45 PM, Simon Marchi wrote:
>> When using -exec-run in mi-async mode on a fresh gdb launch, we can see
>> that it is not actually done asynchronously.
>>
>> The problem is that when we issue -exec-run, the linux native target is
>> not pushed yet. So when the code in mi_cmd_exec-run checks if we support
>> async (by calling mi_async_p), tdefault_can_async_p from the dummy
>> target answers 0.
>>
>> I am not certain of the conceptual correctness of this solution, but it
>> seems to work. It changes mi_async_p so that it uses find_run_target()
>> instead of using the current_target. When -exec-run is used before the
>> native target is pushed, mi_async_p will now report that the target that
>> will eventually be used for running supports async, instead of saying
>> that the current target (dummy) does not.
> 
> This is not correct.  E.g., when some target is already pushed,
> and it's one that does support async, but can't "run", in other places
> that we use mi_async_p we should be consulting the already connected
> target, not fallback to the run target.
> Please make sure to test with native and gdbserver in both
> remote and extended remote modes, to cover different modes of
> operation, though you're likely not seeing an issue with
> "target remote", which does not support "run", just because that
> does implement t->to_create_inferior, but that's for
> extended-remote, really (see find_run_target).
> 
> I think we need to make run_one_inferior itself check whether the
> run target can async, instead of using mi_async_p() there.  Likewise
> for

Is it possible that the last paragraph and the next one are missing some
parts? I'd like to have the complete information before I try to answer
something intelligible :).

> Note that the "mi_async && target_can_async_p()" checks intend to
> mimic GDB's behavior before target-async was the default.  In order
> gdb's, if you did "set target-async on" and then
> -exec-run/continue/step/whatever, gdb would just ignore the target-async
> request.  This is actually documented:
> 
>  On some targets, @value{GDBN} is capable of processing MI commands
>  even while the target is running.  This is called @dfn{asynchronous
>  command execution} (@pxref{Background Execution}).  The frontend may
>  specify a preferrence for asynchronous execution using the
>  @code{-gdb-set mi-async 1} command, which should be emitted before
>  either running the executable or attaching to the target.  After the
>  frontend has started the executable or attached to the target, it can
>  find if asynchronous execution is enabled using the
>  @code{-list-target-features} command.
> 
> I think it'd be cleaner if when "set mi-async on" (the new spelling of
> "set target-async on", it's just an alias) is in effect with a target
> that really CANNOT async, we'd still make -exec-run try to run
> in async mode, which would error out just like if the user does
> "run&" on such a target, but alas, that's not how previous gdb
> releases worked.  We'd need to hear from frontend developers whether
> that's a desirable change.
>
>>
>> I added a small testcase that I copied from mi-async.exp. Please
>> indicate if you think it should be integrated to an existing test rather
>> than in a new test.
>>
>> I have two questions regarding the test:
>>
>>  - Why do we have mi_expect_stop and mi_expect_interrupt? It seems like
>>    the functionality of _interrupt could be integrated in _stop.
> 
> So we can cope with differences like: ...
> 
>>  - The signal reported when interrupting a thread changes when in non-stop vs all-stop:
>>
>>    non-stop: *stopped,reason="signal-received",signal-name="0",signal-meaning="Signal 0",..
>>    all-stop: *stopped,reason="signal-received",signal-name="SIGINT",signal-meaning="Interrupt",...
> 
> ... these?
> 
>>
>>    As a consequence, mi_expect_interrupt only works with non-stop.
> 
> Could you fix it?  I think expecting both signals would be fine.
>
>> +#include <unistd.h>
>> +
>> +int main ()
> 
> int
> main (void)
> 
> 
>> +# The purpose of this test if to verify that -exec-run with mi-async on
>> +# results in asynchronous execution (PR 18077).
>> +
>> +# The plan is for async mode to become the default but toggle for now.
>> +set saved_gdbflags $GDBFLAGS
>> +set GDBFLAGS [concat $GDBFLAGS " -ex \"set mi-async on\""]
>> +
> 
> Can you make this a regular "-gdb-set mi-async", after GDB is
> started, please?  (You'll then need to call mi_detect_async too.)
>
>> +load_lib mi-support.exp
>> +
>> +gdb_exit
>> +if [mi_gdb_start] {
>> +    continue
>> +}
>> +
>> +standard_testfile
>> +
>> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
>> +     untested mi-async.exp
>> +     return -1
>> +}
>> +
>> +mi_delete_breakpoints
>> +mi_gdb_reinitialize_dir $srcdir/$subdir
>> +mi_gdb_load ${binfile}
>> +
>> +# Necessary for mi_expect_interrupt to work, as the reported signal is not the
>> +# same in all-stop.
>> +mi_gdb_test "-gdb-set non-stop 1" ".*"
>> +
>> +proc linux_async_run_test {} {
> 
> Drop the "linux" prefix, please.
> 
>> +    global mi_gdb_prompt
>> +    global hex
> 
> These don't appear to be used.
> 
>> +
>> +    mi_run_cmd
>> +    mi_gdb_test "123-exec-interrupt --all" "123\\^done" "send interrupt command"
>> +    mi_expect_interrupt "expect interrupt"
>> +}
>> +
>> +linux_async_run_test
>> +
>> +mi_gdb_exit
>> +
>> +set GDBFLAGS $saved_gdbflags
>> +
>> +return 0
>>
> 
> Thanks,
> Pedro Alves
>
  

On 03/04/2015 07:29 PM, Simon Marchi wrote:
> Hi Pedro,
> 
> On 03/04/2015 05:18 AM, Pedro Alves wrote:
>> > On 03/03/2015 09:45 PM, Simon Marchi wrote:
>>> >> When using -exec-run in mi-async mode on a fresh gdb launch, we can see
>>> >> that it is not actually done asynchronously.
>>> >>
>>> >> The problem is that when we issue -exec-run, the linux native target is
>>> >> not pushed yet. So when the code in mi_cmd_exec-run checks if we support
>>> >> async (by calling mi_async_p), tdefault_can_async_p from the dummy
>>> >> target answers 0.
>>> >>
>>> >> I am not certain of the conceptual correctness of this solution, but it
>>> >> seems to work. It changes mi_async_p so that it uses find_run_target()
>>> >> instead of using the current_target. When -exec-run is used before the
>>> >> native target is pushed, mi_async_p will now report that the target that
>>> >> will eventually be used for running supports async, instead of saying
>>> >> that the current target (dummy) does not.
>> > 
>> > This is not correct.  E.g., when some target is already pushed,
>> > and it's one that does support async, but can't "run", in other places
>> > that we use mi_async_p we should be consulting the already connected
>> > target, not fallback to the run target.
>> > Please make sure to test with native and gdbserver in both
>> > remote and extended remote modes, to cover different modes of
>> > operation, though you're likely not seeing an issue with
>> > "target remote", which does not support "run", just because that
>> > does implement t->to_create_inferior, but that's for
>> > extended-remote, really (see find_run_target).
>> > 
>> > I think we need to make run_one_inferior itself check whether the
>> > run target can async, instead of using mi_async_p() there.  Likewise
>> > for
> Is it possible that the last paragraph and the next one are missing some
> parts? I'd like to have the complete information before I try to answer
> something intelligible :).

Just nevermind that "Likewise ...".  I was going to
say "Likewise for attach", but then I noticed that "-exec-attach" always
maps to "attach", but missed deleting that bit.  :-)

Let me know if things still aren't clear.

Thanks,
Pedro Alves

> 
>> > Note that the "mi_async && target_can_async_p()" checks intend to
>> > mimic GDB's behavior before target-async was the default.  In order
>> > gdb's, if you did "set target-async on" and then
>> > -exec-run/continue/step/whatever, gdb would just ignore the target-async
>> > request.  This is actually documented:
>> > 
>> >  On some targets, @value{GDBN} is capable of processing MI commands
>> >  even while the target is running.  This is called @dfn{asynchronous
>> >  command execution} (@pxref{Background Execution}).  The frontend may
>> >  specify a preferrence for asynchronous execution using the
>> >  @code{-gdb-set mi-async 1} command, which should be emitted before
>> >  either running the executable or attaching to the target.  After the
>> >  frontend has started the executable or attached to the target, it can
>> >  find if asynchronous execution is enabled using the
>> >  @code{-list-target-features} command.
>> > 
>> > I think it'd be cleaner if when "set mi-async on" (the new spelling of
>> > "set target-async on", it's just an alias) is in effect with a target
>> > that really CANNOT async, we'd still make -exec-run try to run
>> > in async mode, which would error out just like if the user does
>> > "run&" on such a target, but alas, that's not how previous gdb
>> > releases worked.  We'd need to hear from frontend developers whether
>> > that's a desirable change.
>> >
>>> >>
>>> >> I added a small testcase that I copied from mi-async.exp. Please
>>> >> indicate if you think it should be integrated to an existing test rather
>>> >> than in a new test.
>>> >>
>>> >> I have two questions regarding the test:
>>> >>
>>> >>  - Why do we have mi_expect_stop and mi_expect_interrupt? It seems like
>>> >>    the functionality of _interrupt could be integrated in _stop.
>> > 
>> > So we can cope with differences like: ...
>> > 
>>> >>  - The signal reported when interrupting a thread changes when in non-stop vs all-stop:
>>> >>
>>> >>    non-stop: *stopped,reason="signal-received",signal-name="0",signal-meaning="Signal 0",..
>>> >>    all-stop: *stopped,reason="signal-received",signal-name="SIGINT",signal-meaning="Interrupt",...
>> > 
>> > ... these?
>> > 
>>> >>
>>> >>    As a consequence, mi_expect_interrupt only works with non-stop.
>> > 
>> > Could you fix it?  I think expecting both signals would be fine.
>> >
>>> >> +#include <unistd.h>
>>> >> +
>>> >> +int main ()
>> > 
>> > int
>> > main (void)
>> > 
>> > 
>>> >> +# The purpose of this test if to verify that -exec-run with mi-async on
>>> >> +# results in asynchronous execution (PR 18077).
>>> >> +
>>> >> +# The plan is for async mode to become the default but toggle for now.
>>> >> +set saved_gdbflags $GDBFLAGS
>>> >> +set GDBFLAGS [concat $GDBFLAGS " -ex \"set mi-async on\""]
>>> >> +
>> > 
>> > Can you make this a regular "-gdb-set mi-async", after GDB is
>> > started, please?  (You'll then need to call mi_detect_async too.)
>> >
>>> >> +load_lib mi-support.exp
>>> >> +
>>> >> +gdb_exit
>>> >> +if [mi_gdb_start] {
>>> >> +    continue
>>> >> +}
>>> >> +
>>> >> +standard_testfile
>>> >> +
>>> >> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
>>> >> +     untested mi-async.exp
>>> >> +     return -1
>>> >> +}
>>> >> +
>>> >> +mi_delete_breakpoints
>>> >> +mi_gdb_reinitialize_dir $srcdir/$subdir
>>> >> +mi_gdb_load ${binfile}
>>> >> +
>>> >> +# Necessary for mi_expect_interrupt to work, as the reported signal is not the
>>> >> +# same in all-stop.
>>> >> +mi_gdb_test "-gdb-set non-stop 1" ".*"
>>> >> +
>>> >> +proc linux_async_run_test {} {
>> > 
>> > Drop the "linux" prefix, please.
>> > 
>>> >> +    global mi_gdb_prompt
>>> >> +    global hex
>> > 
>> > These don't appear to be used.
>> > 
>>> >> +
>>> >> +    mi_run_cmd
>>> >> +    mi_gdb_test "123-exec-interrupt --all" "123\\^done" "send interrupt command"
>>> >> +    mi_expect_interrupt "expect interrupt"
>>> >> +}
>>> >> +
>>> >> +linux_async_run_test
>>> >> +
>>> >> +mi_gdb_exit
>>> >> +
>>> >> +set GDBFLAGS $saved_gdbflags
>>> >> +
>>> >> +return 0
>>> >>
>> > 
>> > Thanks,
>> > Pedro Alves
>> >
  

On 03/04/2015 02:32 PM, Pedro Alves wrote:
> On 03/04/2015 07:29 PM, Simon Marchi wrote:
>> Hi Pedro,
>>
>> On 03/04/2015 05:18 AM, Pedro Alves wrote:
>>>> On 03/03/2015 09:45 PM, Simon Marchi wrote:
>>>>>> When using -exec-run in mi-async mode on a fresh gdb launch, we can see
>>>>>> that it is not actually done asynchronously.
>>>>>>
>>>>>> The problem is that when we issue -exec-run, the linux native target is
>>>>>> not pushed yet. So when the code in mi_cmd_exec-run checks if we support
>>>>>> async (by calling mi_async_p), tdefault_can_async_p from the dummy
>>>>>> target answers 0.
>>>>>>
>>>>>> I am not certain of the conceptual correctness of this solution, but it
>>>>>> seems to work. It changes mi_async_p so that it uses find_run_target()
>>>>>> instead of using the current_target. When -exec-run is used before the
>>>>>> native target is pushed, mi_async_p will now report that the target that
>>>>>> will eventually be used for running supports async, instead of saying
>>>>>> that the current target (dummy) does not.
>>>>
>>>> This is not correct.  E.g., when some target is already pushed,
>>>> and it's one that does support async, but can't "run", in other places
>>>> that we use mi_async_p we should be consulting the already connected
>>>> target, not fallback to the run target.
>>>> Please make sure to test with native and gdbserver in both
>>>> remote and extended remote modes, to cover different modes of
>>>> operation, though you're likely not seeing an issue with
>>>> "target remote", which does not support "run", just because that
>>>> does implement t->to_create_inferior, but that's for
>>>> extended-remote, really (see find_run_target).
>>>>
>>>> I think we need to make run_one_inferior itself check whether the
>>>> run target can async, instead of using mi_async_p() there.  Likewise
>>>> for
>> Is it possible that the last paragraph and the next one are missing some
>> parts? I'd like to have the complete information before I try to answer
>> something intelligible :).
> 
> Just nevermind that "Likewise ...".  I was going to
> say "Likewise for attach", but then I noticed that "-exec-attach" always
> maps to "attach", but missed deleting that bit.  :-)
> 
> Let me know if things still aren't clear.
> 
> Thanks,
> Pedro Alves

There is still the following paragraph that looks like it was split or
something:

> Note that the "mi_async && target_can_async_p()" checks intend to
> mimic GDB's behavior before target-async was the default.  In order
> gdb's, if you did "set target-async on" and then
> -exec-run/continue/step/whatever, gdb would just ignore the target-async
> request.  This is actually documented:

I think I get the gist of it, but I am asking just in case.

Simon
  

On 03/04/2015 07:35 PM, Simon Marchi wrote:

> There is still the following paragraph that looks like it was split or
> something:
> 
>> Note that the "mi_async && target_can_async_p()" checks intend to
>> mimic GDB's behavior before target-async was the default.  In order
>> gdb's, if you did "set target-async on" and then
>> -exec-run/continue/step/whatever, gdb would just ignore the target-async
>> request.  This is actually documented:
> 
> I think I get the gist of it, but I am asking just in case.

Sorry about that.  I was interrupted midway writing that email,
and looks like I didn't glue things the best I could...  Let
me expand.

Here what I was thinking was, it might look like that the fix
should just be to make run_one_inferior pass the mi_async
variable directly, like:

 -    mi_execute_cli_command (run_cmd, mi_async_p (),
 +    mi_execute_cli_command (run_cmd, mi_async,
			      mi_async_p () ? "&" : NULL);

But, that would make "set mi-async on; -exec-run" with a target
that can't do async error out with "Asynchronous execution not
supported on this target", which is not how MI is documented to behave.
The "mi_async && target_can_async_p()" checks inside mi_async_p(), and
its use here mimic older GDBs behavior before target-async
was the default.  IOW, in older gdb's, if you did "set target-async on"
and then -exec-run/continue/step/whatever with a non-async target,
gdb would just ignore the target-async request and do a
synchronous run.

Let me know whether it's better now.  :-)

Thanks,
Pedro Alves