Re: [PATCH] mi_async_p: Use the default run target (PR gdb/18077)

[Pedro Alves <palves at redhat dot com>]

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