[Bug gdb/26199] GDB goes in busy loop when interrupting non-stop program

["cvs-commit at gcc dot gnu.org" <sourceware-bugzilla@sourceware.org>]

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

--- Comment #7 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Pedro Alves <palves@sourceware.org>:

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

commit b3e3a4c11496dca710c62e32db80e27dd7301223
Author: Simon Marchi <simon.marchi@polymtl.ca>
Date:   Sat Jul 4 13:33:19 2020 +0100

    Fix GDB busy loop when interrupting non-stop program (PR 26199)

    When interrupting a program in non-stop, the program gets interrupted
    correctly, but GDB busy loops (the event loop is always woken up).

    Here is how to reproduce it:

     1. Start GDB: ./gdb -nx --data-directory=data-directory -ex "set non-stop
1" --args  /bin/sleep 60
     2. Run the program with "run"
     3. Interrupt with ^C.
     4. Look into htop, see GDB taking 100% CPU

    Debugging `handle_file_event`, we see that the event source that wakes
    up the event loop is the linux-nat one:

     (top-gdb) p file_ptr.proc
     $5 = (handler_func *) 0xb9cccd <handle_target_event(int, gdb_client_data)>
                                     ^^^^^^^^^^^^^^^^^^^
                                             |
                                             \-- the linux-nat callback

    Debugging fetch_inferior_event and do_target_wait, we see that we
    don't actually call `wait` on the linux-nat target, because
    inferior_matches returns false:

     auto inferior_matches = [&wait_ptid] (inferior *inf)
       {
         return (inf->process_target () != NULL
                 && (threads_are_executing (inf->process_target ())
                     || threads_are_resumed_pending_p (inf))
                 && ptid_t (inf->pid).matches (wait_ptid));
       };

    because `threads_are_executing` is false.

    What happens is:

     1. User types ctrl-c, that writes in the linux-nat pipe, waking up
        the event source.

     2. linux-nat's wait gets called, the SIGINT event is returned, but
        before returning, it marks the pipe again, in order for wait to
        get called again:

        /* If we requested any event, and something came out, assume there
           may be more.  If we requested a specific lwp or process, also
           assume there may be more.  */
        if (target_is_async_p ()
            && ((ourstatus->kind != TARGET_WAITKIND_IGNORE
                 && ourstatus->kind != TARGET_WAITKIND_NO_RESUMED)
                || ptid != minus_one_ptid))
          async_file_mark ();

     3. The SIGINT event is handled, the program is stopped, the stop
        notification is printed.

     4. The event loop is woken up again because of the `async_file_mark`
        of step 2.

     5. Because `inferior_matches` returns false, we never call
        linux-nat's wait, so the pipe stays readable.

     6. Goto 4.

    Pedro says:

    This commit fixes it by letting do_target_wait call target_wait even
    if threads_are_executing is false.  This will normally result in the
    target returning TARGET_WAITKIND_NO_RESUMED, and _not_ marking its
    event source again.  This results in infrun only calling into the
    target only once (i.e., breaking the busy loop).

    Note that the busy loop bug didn't trigger in all-stop mode because
    all-stop handles this by unregistering the target from the event loop
    as soon as it was all stopped -- see
    inf-loop.c:inferior_event_handler's INF_EXEC_COMPLETE handling.  If we
    remove that non-stop check from inferior_event_handler, and replace
    the target_has_execution check for threads_are_executing instead, it
    also fixes the issue for non-stop.  I considered that as the final
    solution, but decided that the solution proposed here instead is just
    simpler and more future-proof design.  With the
    TARGET_WAITKIND_NO_RESUMED handling fixes done in the previous
    patches, I think it should be possible to always keep the target
    registered in the event loop, meaning we could eliminate the
    target_async(0) call from inferior_event_handler as well as most of
    the target_async(1) calls in the target backends.  That would allow in
    the future e.g., the remote target reporting asynchronous
    notifications even if all threads are stopped.  I haven't attempted
    that, though.

    gdb/ChangeLog:
    yyyy-mm-dd  Simon Marchi  <simon.marchi@polymtl.ca>
                Pedro Alves  <pedro@palves.net>

            PR gdb/26199
            * infrun.c (threads_are_resumed_pending_p): Delete.
            (do_target_wait): Remove threads_are_executing and
            threads_are_resumed_pending_p checks from the inferior_matches
            lambda.  Update comments.

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