[Bug gdb/26642] New: "maintenance set target-async off" is broken on GNU/Linux

[Bug gdb/26642] New: "maintenance set target-async off" is broken on GNU/Linux

[simark at simark dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=26642

            Bug ID: 26642
           Summary: "maintenance set target-async off" is broken on
                    GNU/Linux
           Product: gdb
           Version: HEAD
            Status: NEW
          Severity: normal
          Priority: P2
         Component: gdb
          Assignee: unassigned at sourceware dot org
          Reporter: simark at simark dot ca
  Target Milestone: ---

Before commit 5b6d1e4fa4fc ("Multi-target support"), I'm able to run to main
when using "maintenance set target-async off".  With the commit, it just hangs.

Before:

$ ./gdb -q --data-directory=data-directory/ -nx test  -ex "set debug infrun 1"
-ex "maintenance set target-async off"
Reading symbols from test...
(gdb) b main
Breakpoint 1 at 0x4004e8: file test.c, line 6.
(gdb) r
Starting program: /home/smarchi/build/binutils-gdb/gdb/test 
infrun: proceed (addr=0x7ffff7dd7c30, signal=GDB_SIGNAL_0)
infrun: proceed: resuming process 15009
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread
[process 15009] at 0x7ffff7dd7c30
infrun: prepare_to_wait
infrun: target_wait (-1.0.0, status) =
infrun:   15009.15009.0 [process 15009],
infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: stop_pc = 0x7ffff7ddb120
infrun: BPSTAT_WHAT_SINGLE
infrun: no stepping, continue
infrun: resume (step=1, signal=GDB_SIGNAL_0), trap_expected=1, current thread
[process 15009] at 0x7ffff7ddb120
infrun: prepare_to_wait
infrun: target_wait (-1.0.0, status) =
infrun:   15009.15009.0 [process 15009],
infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: stop_pc = 0x7ffff7ddb121
infrun: no stepping, continue
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread
[process 15009] at 0x7ffff7ddb121
infrun: prepare_to_wait
infrun: target_wait (-1.0.0, status) =
infrun:   15009.15009.0 [process 15009],
infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: handle_inferior_event status->kind = stopped, signal = GDB_SIGNAL_TRAP
infrun: stop_pc = 0x4004e8
infrun: BPSTAT_WHAT_STOP_NOISY
infrun: stop_waiting
infrun: stop_all_threads
infrun: stop_all_threads, pass=0, iterations=0
infrun:   process 15009 not executing
infrun: stop_all_threads, pass=1, iterations=1
infrun:   process 15009 not executing
infrun: stop_all_threads done

Breakpoint 1, main () at test.c:6
6           for (int i = 0; i < 2; i++) {
(gdb) 


After:

$ ./gdb -q --data-directory=data-directory/ -nx test  -ex "set debug infrun 1"
-ex "maintenance set target-async off"
Reading symbols from test...
(gdb) b main
Breakpoint 1 at 0x4004e8: file test.c, line 6.
(gdb) r
Starting program: /home/smarchi/build/binutils-gdb/gdb/test 
infrun: proceed (addr=0x7ffff7dd7c30, signal=GDB_SIGNAL_0)
infrun: proceed: resuming process 17528
infrun: resume (step=0, signal=GDB_SIGNAL_0), trap_expected=0, current thread
[process 17528] at 0x7ffff7dd7c30
infrun: prepare_to_wait
<hangs>

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/26642] "maintenance set target-async off" is broken on GNU/Linux

[simark at simark dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=26642

Simon Marchi <simark at simark dot ca> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |10.1

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/26642] "maintenance set target-async off" is broken on GNU/Linux

[simark at simark dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=26642

--- Comment #1 from Simon Marchi <simark at simark dot ca> ---
The difference I see between before and after is that linux_nat_target::wait
now gets called with TARGET_WNOHANG, whereas before it didn't.

In target-async off, we expect the wait to be blocking.

Before, fetch_inferior_event did this:

    ecs->ptid = do_target_wait (waiton_ptid, &ecs->ws,
                                target_can_async_p () ? TARGET_WNOHANG : 0);

Note that we didn't pass TARGET_WNOHANG when the target was not async.

Now, it does:

    if (!do_target_wait (minus_one_ptid, ecs, TARGET_WNOHANG))
      return;

So TARGET_WNOHANG is always passed.

It makes a difference if the timing is bad:

1. we resume execution, the infrun async handler is marked
2. linux_nat_target::wait is called with WNOHANG, there's no event to report
3. we go back waiting on the event loop
4. SIGCHLD finally arrives, but the event loop is not woken up (because we are
not in async)

If I put a sleep at the beginning of linux_nat_target::wait, it ensures the
SIGCHILD arrives before we call wait, so we don't miss it.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/26642] "maintenance set target-async off" is broken on GNU/Linux

[simark at simark dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=26642

--- Comment #2 from Simon Marchi <simark at simark dot ca> ---
This appears to fix it, not sure if it's correct though.


diff --git a/gdb/infrun.c b/gdb/infrun.c
index 6532b06ae52..f05811e5e49 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3533,6 +3533,9 @@ do_target_wait_1 (inferior *inf, ptid_t ptid,

   /* But if we don't find one, we'll have to wait.  */

+  if (!target_is_async_p ())
+    options &= ~TARGET_WNOHANG;
+
   if (deprecated_target_wait_hook)
     event_ptid = deprecated_target_wait_hook (ptid, status, options);
   else

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/26642] "maintenance set target-async off" is broken on GNU/Linux

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26642

--- Comment #3 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Simon Marchi <simark@sourceware.org>:

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

commit d3a071228e8f7cf9da017f2d0d6c28468a652795
Author: Simon Marchi <simon.marchi@polymtl.ca>
Date:   Tue Oct 13 12:01:19 2020 -0400

    gdb: don't pass TARGET_WNOHANG to targets that can't async (PR 26642)

    Debugging with "maintenance set target-async off" on Linux has been
    broken since 5b6d1e4fa4f ("Multi-target support").

    The issue is easy to reproduce:

        $ ./gdb -q --data-directory=data-directory -nx ./test
        Reading symbols from ./test...
        (gdb) maintenance set target-async off
        (gdb) start
        Temporary breakpoint 1 at 0x1151: file test.c, line 5.
        Starting program: /home/simark/build/binutils-gdb/gdb/test

    ... and it hangs there.

    The difference between pre-5b6d1e4fa4f and 5b6d1e4fa4f is that
    fetch_inferior_event now calls target_wait with TARGET_WNOHANG for
    non-async-capable targets, whereas it didn't before.

    For non-async-capable targets, this is how it's expected to work when
    resuming execution:

    1. we call resume
    2. the infrun async handler is marked in prepare_to_wait, to immediately
       wake up the event loop when we get back to it
    3. fetch_inferior_event calls the target's wait method without
       TARGET_WNOHANG, effectively blocking until the target has something
       to report

    However, since we call the target's wait method with TARGET_WNOHANG,
    this happens:

    1. we call resume
    2. the infrun async handler is marked in prepare_to_wait, to immediately
       wake up the event loop when we get back to it
    3. fetch_inferior_event calls the target's wait method with
       TARGET_WNOHANG, the target has nothing to report yet
    4. we go back to blocking on the event loop
    5. SIGCHLD finally arrives, but the event loop is not woken up, because
       we are not in async mode.  Normally, we should have been stuck in
       waitpid the SIGCHLD would have unblocked us.

    We end up in this situation because these two necessary conditions are
    met:

    1. GDB uses the TARGET_WNOHANG option with a target that can't do async.
       I don't think this makes sense.  I mean, it's technically possible,
       the doc for TARGET_WNOHANG is:

      /* Return immediately if there's no event already queued.  If this
         options is not requested, target_wait blocks waiting for an
         event.  */
      TARGET_WNOHANG = 1,

       ... which isn't in itself necessarily incompatible with synchronous
       targets.  It could be possible for a target to support non-blocking
       polls, while not having a way to asynchronously wake up the event
       loop, which is also necessary to support async.  But as of today,
       we don't expect GDB and sync targets to work this way.

    2. The linux-nat target, even in the mode where it emulates a
       synchronous target (with "maintenance set target-async off") respects
       TARGET_WNOHANG.  Other non-async targets, such as windows_nat_target,
       simply don't check / support TARGET_WNOHANG, so their wait method is
       always blocking.

    Fix the first issue by avoiding using TARGET_WNOHANG on non-async
    targets, in do_target_wait_1.  Add an assert in target_wait to verify it
    doesn't happen.

    The new test gdb.base/maint-target-async-off.exp is a simple test that
    just tries running to main and then to the end of the program, with
    "maintenance set target-async off".

    gdb/ChangeLog:

            PR gdb/26642
            * infrun.c (do_target_wait_1): Clear TARGET_WNOHANG if the
            target can't do async.
            * target.c (target_wait): Assert that we don't pass
            TARGET_WNOHANG to a target that can't async.

    gdb/testsuite/ChangeLog:

            PR gdb/26642
            * gdb.base/maint-target-async-off.c: New test.
            * gdb.base/maint-target-async-off.exp: New test.

    Change-Id: I69ad3a14598863d21338a8c4e78700a58ce7ad86

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/26642] "maintenance set target-async off" is broken on GNU/Linux

[simark at simark dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=26642

Simon Marchi <simark at simark dot ca> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #4 from Simon Marchi <simark at simark dot ca> ---
Fixed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/26642] "maintenance set target-async off" is broken on GNU/Linux

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26642

--- Comment #5 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The gdb-10-branch branch has been updated by Simon Marchi
<simark@sourceware.org>:

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

commit f9052d5c0ee65f6ebe0037aabfa7e321306e5fd8
Author: Simon Marchi <simon.marchi@polymtl.ca>
Date:   Tue Oct 13 12:01:19 2020 -0400

    gdb: don't pass TARGET_WNOHANG to targets that can't async (PR 26642)

    Debugging with "maintenance set target-async off" on Linux has been
    broken since 5b6d1e4fa4f ("Multi-target support").

    The issue is easy to reproduce:

        $ ./gdb -q --data-directory=data-directory -nx ./test
        Reading symbols from ./test...
        (gdb) maintenance set target-async off
        (gdb) start
        Temporary breakpoint 1 at 0x1151: file test.c, line 5.
        Starting program: /home/simark/build/binutils-gdb/gdb/test

    ... and it hangs there.

    The difference between pre-5b6d1e4fa4f and 5b6d1e4fa4f is that
    fetch_inferior_event now calls target_wait with TARGET_WNOHANG for
    non-async-capable targets, whereas it didn't before.

    For non-async-capable targets, this is how it's expected to work when
    resuming execution:

    1. we call resume
    2. the infrun async handler is marked in prepare_to_wait, to immediately
       wake up the event loop when we get back to it
    3. fetch_inferior_event calls the target's wait method without
       TARGET_WNOHANG, effectively blocking until the target has something
       to report

    However, since we call the target's wait method with TARGET_WNOHANG,
    this happens:

    1. we call resume
    2. the infrun async handler is marked in prepare_to_wait, to immediately
       wake up the event loop when we get back to it
    3. fetch_inferior_event calls the target's wait method with
       TARGET_WNOHANG, the target has nothing to report yet
    4. we go back to blocking on the event loop
    5. SIGCHLD finally arrives, but the event loop is not woken up, because
       we are not in async mode.  Normally, we should have been stuck in
       waitpid the SIGCHLD would have unblocked us.

    We end up in this situation because these two necessary conditions are
    met:

    1. GDB uses the TARGET_WNOHANG option with a target that can't do async.
       I don't think this makes sense.  I mean, it's technically possible,
       the doc for TARGET_WNOHANG is:

      /* Return immediately if there's no event already queued.  If this
         options is not requested, target_wait blocks waiting for an
         event.  */
      TARGET_WNOHANG = 1,

       ... which isn't in itself necessarily incompatible with synchronous
       targets.  It could be possible for a target to support non-blocking
       polls, while not having a way to asynchronously wake up the event
       loop, which is also necessary to support async.  But as of today,
       we don't expect GDB and sync targets to work this way.

    2. The linux-nat target, even in the mode where it emulates a
       synchronous target (with "maintenance set target-async off") respects
       TARGET_WNOHANG.  Other non-async targets, such as windows_nat_target,
       simply don't check / support TARGET_WNOHANG, so their wait method is
       always blocking.

    Fix the first issue by avoiding using TARGET_WNOHANG on non-async
    targets, in do_target_wait_1.  Add an assert in target_wait to verify it
    doesn't happen.

    The new test gdb.base/maint-target-async-off.exp is a simple test that
    just tries running to main and then to the end of the program, with
    "maintenance set target-async off".

    gdb/ChangeLog:

            PR gdb/26642
            * infrun.c (do_target_wait_1): Clear TARGET_WNOHANG if the
            target can't do async.
            * target.c (target_wait): Assert that we don't pass
            TARGET_WNOHANG to a target that can't async.

    gdb/testsuite/ChangeLog:

            PR gdb/26642
            * gdb.base/maint-target-async-off.c: New test.
            * gdb.base/maint-target-async-off.exp: New test.

    Change-Id: I69ad3a14598863d21338a8c4e78700a58ce7ad86

-- 
You are receiving this mail because:
You are on the CC list for the bug.