Bug 20045 - With mi-async on, -exec-continue outputs two (gdb) prompts
Summary: With mi-async on, -exec-continue outputs two (gdb) prompts
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: mi (show other bugs)
Version: HEAD
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-04 18:12 UTC by Simon Marchi
Modified: 2016-05-18 14:30 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Marchi 2016-05-04 18:12:33 UTC
Test program:

int main() {
	for (;;);
	return 0;
}

Build:
  $ gcc test.c -g3 -O0 -o test

Start GDB:
  $./gdb -nx -i mi -ex "set mi-async on" test

Commands:
  -exec-run --start
  -exec-continue

GDB will output two prompts:

  -exec-continue
  ^running
  *running,thread-id="all"
  (gdb) 
  (gdb)

while we would expect to have just one.

This problem wasn't there with gdb 7.8, but was there in 7.9.
Comment 1 Simon Marchi 2016-05-04 19:10:37 UTC
Bisecting found this commit to have introduced the behavior:

198297aafb4f7a9717be8370581b048ae9107c14
Linux: make target_is_async_p return false when async is off
Comment 2 Simon Marchi 2016-05-04 21:09:04 UTC
Ok, from what I understand, in an ideal world, the first (gdb) wouldn't be there.  It's printed here:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/mi/mi-interp.c;h=b37dc96ce4e0d4095a570b2e70ca360a64a21a58;hb=HEAD#l993

It seems like it's just there to keep an old behaviour, where doing -exec-continue in synchronous mode prints the prompt immediately, and a second time when the target stops.  Setting aside backwards compatibility, gdb should only print the prompt when the target stops, since that's when it's ready to take new commands.  Otherwise, it's just lying to the frontend.  But in that case, we probably have to live with it.

What I don't understand is that the condition evaluates to true here:

1001       if (!target_is_async_p () || sync_execution)
1002         fputs_unfiltered ("(gdb) \n", raw_stdout);

When using mi-async on, sync_execution is 0, that's fine.  However, target_is_async_p () returns false.  I don't understand, because we are currently executing, so the linux native target is active, and it should be async by default (I didn't touch maint set target-async).  I interrupted my debugged gdb at another time, while the inferior was executing, and then I could see that target_is_async_p would've returned true.  Is it possible that when all threads are stopped, we close the linux_nat_event_pipe, only to re-open it when we resume?

Anyway, it seems to me like this condition could only be 

  if (sync_execution)

Basically, if we are emulating a synchronous MI command, output the bogus (gdb).  When using mi-async off, sync_execution will be false, so we will never output it.  The result is this for mi-async off:


-exec-run --start
=breakpoint-created,bkpt={number="1",type="breakpoint",disp="del",enabled="y",addr="0x00000000004004f1",func="main",file="test.c",fullname="/home/emaisin/build/binutils-gdb/gdb/test.c",line="2",thread-groups=["i1"],times="0",original-location="main"}
=thread-group-started,id="i1",pid="1729"
=thread-created,id="1",group-id="i1"
=library-loaded,id="/lib64/ld-linux-x86-64.so.2",target-name="/lib64/ld-linux-x86-64.so.2",host-name="/lib64/ld-linux-x86-64.so.2",symbols-loaded="0",thread-group="i1"
^running
*running,thread-id="all"
(gdb) 
=library-loaded,id="/lib/x86_64-linux-gnu/libc.so.6",target-name="/lib/x86_64-linux-gnu/libc.so.6",host-name="/lib/x86_64-linux-gnu/libc.so.6",symbols-loaded="0",thread-group="i1"
=breakpoint-modified,bkpt={number="1",type="breakpoint",disp="del",enabled="y",addr="0x00000000004004f1",func="main",file="test.c",fullname="/home/emaisin/build/binutils-gdb/gdb/test.c",line="2",thread-groups=["i1"],times="1",original-location="main"}
~"\n"
~"Temporary breakpoint 1, main () at test.c:2\n"
~"2\t\tfor (;;);\n"
*stopped,reason="breakpoint-hit",disp="del",bkptno="1",frame={addr="0x00000000004004f1",func="main",args=[],file="test.c",fullname="/home/emaisin/build/binutils-gdb/gdb/test.c",line="2"},thread-id="1",stopped-threads="all",core="2"
=breakpoint-deleted,id="1"
(gdb) 
-exec-continue 
^running
*running,thread-id="all"
(gdb)
^C~"\nProgram"
~" received signal SIGINT, Interrupt.\n"
~"main () at test.c:2\n"
~"2\t\tfor (;;);\n"
*stopped,reason="signal-received",signal-name="SIGINT",signal-meaning="Interrupt",frame={addr="0x00000000004004f1",func="main",args=[],file="test.c",fullname="/home/emaisin/build/binutils-gdb/gdb/test.c",line="2"},thread-id="1",stopped-threads="all",core="1"
(gdb) 

Notice the (gdb) when -exec-run starts executing, and the other when the target stops (breakpoint at main).  Same for the -exec-continue.  That's the old (buggy but backwards compatible) behaviour.

And with mi-async on:

-exec-run --start
=breakpoint-created,bkpt={number="1",type="breakpoint",disp="del",enabled="y",addr="0x00000000004004f1",func="main",file="test.c",fullname="/home/emaisin/build/binutils-gdb/gdb/test.c",line="2",thread-groups=["i1"],times="0",original-location="main"}
=thread-group-started,id="i1",pid="1788"
=thread-created,id="1",group-id="i1"
=library-loaded,id="/lib64/ld-linux-x86-64.so.2",target-name="/lib64/ld-linux-x86-64.so.2",host-name="/lib64/ld-linux-x86-64.so.2",symbols-loaded="0",thread-group="i1"
^running
*running,thread-id="all"
(gdb) 
=library-loaded,id="/lib/x86_64-linux-gnu/libc.so.6",target-name="/lib/x86_64-linux-gnu/libc.so.6",host-name="/lib/x86_64-linux-gnu/libc.so.6",symbols-loaded="0",thread-group="i1"
=breakpoint-modified,bkpt={number="1",type="breakpoint",disp="del",enabled="y",addr="0x00000000004004f1",func="main",file="test.c",fullname="/home/emaisin/build/binutils-gdb/gdb/test.c",line="2",thread-groups=["i1"],times="1",original-location="main"}
~"\n"
~"Temporary breakpoint 1, main () at test.c:2\n"
~"2\t\tfor (;;);\n"
*stopped,reason="breakpoint-hit",disp="del",bkptno="1",frame={addr="0x00000000004004f1",func="main",args=[],file="test.c",fullname="/home/emaisin/build/binutils-gdb/gdb/test.c",line="2"},thread-id="1",stopped-threads="all",core="0"
=breakpoint-deleted,id="1"
-exec-continue
^running
*running,thread-id="all"
(gdb) 
^C&"Quit\n"
-exec-interrupt
^done
(gdb) 
~"\nProgram"
~" received signal SIGINT, Interrupt.\n"
~"main () at test.c:2\n"
~"2\t\tfor (;;);\n"
*stopped,reason="signal-received",signal-name="SIGINT",signal-meaning="Interrupt",frame={addr="0x00000000004004f1",func="main",args=[],file="test.c",fullname="/home/emaisin/build/binutils-gdb/gdb/test.c",line="2"},thread-id="1",stopped-threads="all",core="3"

Notice the single (gdb) after -exec-run.  This is right because gdb is truly ready to receive new MI commands at this point.  There is no (gdb) when the targets stops at main, because gdb was already ready.  Same goes for -exec-continue, there is a single (gdb).  Finally, there is a last one when gdb has executed -exec-interrupt.

I haven't done some extensive testing though, this is just the result of some quick debugging.
Comment 3 Pedro Alves 2016-05-04 22:28:12 UTC
Don't forget that -exec-run is always synchronous, due to PR 18077.  We should probably fix that.
Comment 4 Pedro Alves 2016-05-04 22:30:58 UTC
> Is it possible that when all threads are stopped, we close the 
> linux_nat_event_pipe, only to re-open it when we resume?

We do, in both cases with in linux_nat_async.  We close it with the target_async all in inf-loop.c:inferior_event_handler when the program stops, and reopen with the target_async call in linux_nat_resume when resuming.
Comment 5 Pedro Alves 2016-05-04 22:44:15 UTC
> Anyway, it seems to me like this condition could only be 
>
>  if (sync_execution)
>
> Basically, if we are emulating a synchronous MI command, output the bogus 
> (gdb).  When using mi-async off, sync_execution will be false, so we will 
> never output it.  The result is this for mi-async off:

I think you meant "When using mi-async on, sync_execution will be false".

But when "maint set target-async off" (i.e., on targets that don't do async), sync_execution will be off, even with sync commands.

This patch from the console series replaces that with a single enum that works the same for async and sync targets, so probably fixes this:

  https://sourceware.org/ml/gdb-patches/2016-02/msg00080.html

(haven't tried yet)

Though I think an easier and safe-for-7.11.1-even fix would be to call target_can_async instead of target_is_async here.
Comment 6 Simon Marchi 2016-05-05 14:16:04 UTC
(In reply to Pedro Alves from comment #3)
> Don't forget that -exec-run is always synchronous, due to PR 18077.  We
> should probably fix that.

Right, but we can probably treat it as a separate problem?
Comment 7 Pedro Alves 2016-05-05 14:22:46 UTC
Certainly.  Was just pointing it out to avoid taking conclusions out of what -exec-run currently outputs.
Comment 8 Simon Marchi 2016-05-05 15:51:54 UTC
(In reply to Pedro Alves from comment #5)
> > Anyway, it seems to me like this condition could only be 
> >
> >  if (sync_execution)
> >
> > Basically, if we are emulating a synchronous MI command, output the bogus 
> > (gdb).  When using mi-async off, sync_execution will be false, so we will 
> > never output it.  The result is this for mi-async off:
> 
> I think you meant "When using mi-async on, sync_execution will be false".

Oops, yes.

> But when "maint set target-async off" (i.e., on targets that don't do
> async), sync_execution will be off, even with sync commands.

I didn't think about that.  Does that mean that having mi-async on is impossible with sync targets?  That would make sense, since when the target is busy waiting on the target, it can't process MI commands. 

> This patch from the console series replaces that with a single enum that
> works the same for async and sync targets, so probably fixes this:
> 
>   https://sourceware.org/ml/gdb-patches/2016-02/msg00080.html
> 
> (haven't tried yet)

I tested with your console series (both palves/console-submit and palves/console-v3), and it indeed works fine on that.  You are always two steps ahead of the rest of the world!

> Though I think an easier and safe-for-7.11.1-even fix would be to call
> target_can_async instead of target_is_async here.

Is it a good idea to change such a behavior on a .1 release though?  If a front-end has to do something special to cope with the additional prompt on 7.11 (which I doubt, since we didn't have any bug reports from front-end developers, but still), it would make things even more complicated for them if 7.11.1 had a different behavior than 7.11.  Given that it's a mostly harmless bug, I suggest we leave all 7.11 gdbs behaving the same, and fix it for 7.12.  WDYT?
Comment 9 Pedro Alves 2016-05-17 19:21:55 UTC
I just see it as a regression / bug.  Seems better to me to contain the chances of frontends relying on the spurious prompt sooner than later.  But I definitely won't insist.
Comment 10 Sourceware Commits 2016-05-18 14:15:30 UTC
The master branch has been updated by Simon Marchi <simark@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=28addb40c77db5a5873172b62b6b7b43e5e05014

commit 28addb40c77db5a5873172b62b6b7b43e5e05014
Author: Simon Marchi <simon.marchi@ericsson.com>
Date:   Tue May 17 17:07:20 2016 -0400

    Fix double prompt output after run control MI commands with mi-async on (PR 20045)
    
    When you use a run control command (-exec-run, -exec-continue,
    -exec-next, ...) with mi-async on, an extra (gdb) prompt is displayed:
    
      -exec-continue
      ^running
      *running,thread-id="all"
      (gdb)
      (gdb)
    
    It doesn't seem to be a big problem for front-ends, since this behavior
    started in gdb 7.9 and we haven't heard anything about that.  However,
    it caused me some trouble while writing a test for PR 20039 [1].
    
    The problem comes from an extra (gdb) prompt that we write when running
    in mi-async off mode to emulate a past buggy behavior.  When executing a
    run control command synchronously, previous gdbs always printed a prompt
    right away, even though they are not ready to accept new MI commands
    until the target stops.  Only at this time should they display a prompt.
    But to keep backwards compatibility apparently, we print it anyway.
    Since commit 198297aaf, the condition that decides whether we should
    print that "bogus" prompt or not has become true, even when running with
    mi-async on.  Since we already print a prompt at the end of the
    asynchronous command execution, it results in two prompts for one
    command.
    
    The proposed fix is to call target_can_async_p instead of
    target_is_async_p, to make the condition:
    
      if (!target_can_async_p () || sync_execution)
        ... show prompt ...
    
    That shows the prompt if we are emulating a synchronous command on top
    of an asynchronous target (sync_execution) or if the target simply can't
    run asynchronously (!target_can_async_p ()).
    
    Note that this code is changed and this bug fixed by Pedro's separate
    console series, but I think it would be nice to have it fixed in the
    mean time.
    
    I ran the gdb.mi directory of the testsuite with mi-async on and off, I
    didn't see any regressions.
    
    gdb/ChangeLog:
    
    	* mi/mi-main.c (mi_on_resume): Call target_can_async_p instead
    	of target_is_async_p.
    
    [1] https://sourceware.org/ml/gdb-patches/2016-05/msg00075.html
Comment 11 Sourceware Commits 2016-05-18 14:15:37 UTC
The master branch has been updated by Simon Marchi <simark@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=9e8f9b05add4517189d7724ff3ed7c16f7b04daf

commit 9e8f9b05add4517189d7724ff3ed7c16f7b04daf
Author: Simon Marchi <simon.marchi@ericsson.com>
Date:   Wed May 18 10:13:12 2016 -0400

    Add mi-threads-interrupt.exp test (PR 20039)
    
    Add a new test for PR 20039.  The test spawns new threads, then tries to
    interrupt, continue, and interrupt again.  This use case was fixed by
    commit 5fe966540d6b748f825774868463003700f0c878 in master, but gdb 7.11
    is affected (so if you try it on the gdb-7.11-branch right now, the test
    will fail).
    
    New in v2, the test now handles mi-async on mode properly.  The failure
    was specific to mi-async off, but I don't think it's bad to test the
    same thing under async on mode.  I added a little hack when running in
    async mode to work around bug 20045.
    
    I also removed one continue/interrupt pair, as a single one was enough to
    trigger the problem.
    
    gdb/testsuite/ChangeLog:
    
    	* gdb.mi/mi-threads-interrupt.c: New file.
    	* gdb.mi/mi-threads-interrupt.exp: New file.
Comment 12 Sourceware Commits 2016-05-18 14:21:52 UTC
The gdb-7.11-branch branch has been updated by Simon Marchi <simark@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=f0a8d0dc70eddbf1e323e8c07f5092ff5e327548

commit f0a8d0dc70eddbf1e323e8c07f5092ff5e327548
Author: Simon Marchi <simon.marchi@ericsson.com>
Date:   Tue May 17 17:07:20 2016 -0400

    Fix double prompt output after run control MI commands with mi-async on (PR 20045)
    
    When you use a run control command (-exec-run, -exec-continue,
    -exec-next, ...) with mi-async on, an extra (gdb) prompt is displayed:
    
      -exec-continue
      ^running
      *running,thread-id="all"
      (gdb)
      (gdb)
    
    It doesn't seem to be a big problem for front-ends, since this behavior
    started in gdb 7.9 and we haven't heard anything about that.  However,
    it caused me some trouble while writing a test for PR 20039 [1].
    
    The problem comes from an extra (gdb) prompt that we write when running
    in mi-async off mode to emulate a past buggy behavior.  When executing a
    run control command synchronously, previous gdbs always printed a prompt
    right away, even though they are not ready to accept new MI commands
    until the target stops.  Only at this time should they display a prompt.
    But to keep backwards compatibility apparently, we print it anyway.
    Since commit 198297aaf, the condition that decides whether we should
    print that "bogus" prompt or not has become true, even when running with
    mi-async on.  Since we already print a prompt at the end of the
    asynchronous command execution, it results in two prompts for one
    command.
    
    The proposed fix is to call target_can_async_p instead of
    target_is_async_p, to make the condition:
    
      if (!target_can_async_p () || sync_execution)
        ... show prompt ...
    
    That shows the prompt if we are emulating a synchronous command on top
    of an asynchronous target (sync_execution) or if the target simply can't
    run asynchronously (!target_can_async_p ()).
    
    Note that this code is changed and this bug fixed by Pedro's separate
    console series, but I think it would be nice to have it fixed in the
    mean time.
    
    I ran the gdb.mi directory of the testsuite with mi-async on and off, I
    didn't see any regressions.
    
    gdb/ChangeLog:
    
    	* mi/mi-main.c (mi_on_resume): Call target_can_async_p instead
    	of target_is_async_p.
    
    [1] https://sourceware.org/ml/gdb-patches/2016-05/msg00075.html
Comment 13 Sourceware Commits 2016-05-18 14:21:58 UTC
The gdb-7.11-branch branch has been updated by Simon Marchi <simark@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=cf2cd51217c0b99f5370708cc3932c12a1f4edad

commit cf2cd51217c0b99f5370708cc3932c12a1f4edad
Author: Simon Marchi <simon.marchi@ericsson.com>
Date:   Wed May 18 10:13:12 2016 -0400

    Add mi-threads-interrupt.exp test (PR 20039)
    
    Add a new test for PR 20039.  The test spawns new threads, then tries to
    interrupt, continue, and interrupt again.  This use case was fixed by
    commit 5fe966540d6b748f825774868463003700f0c878 in master, but gdb 7.11
    is affected (so if you try it on the gdb-7.11-branch right now, the test
    will fail).
    
    New in v2, the test now handles mi-async on mode properly.  The failure
    was specific to mi-async off, but I don't think it's bad to test the
    same thing under async on mode.  I added a little hack when running in
    async mode to work around bug 20045.
    
    I also removed one continue/interrupt pair, as a single one was enough to
    trigger the problem.
    
    gdb/testsuite/ChangeLog:
    
    	* gdb.mi/mi-threads-interrupt.c: New file.
    	* gdb.mi/mi-threads-interrupt.exp: New file.
Comment 14 Simon Marchi 2016-05-18 14:30:23 UTC
Fix pushed.