RFA/gdbserver: GDB internal-error debugging threaded program with breakpoint and forks (was: "Re: RFH: failed assert debugging threaded+fork program over gdbserver")

[Joel Brobecker <brobecker at adacore dot com>]

> I'm thinking the right thing to do, here is to enhance select_event_lwp
> to look for threads that received a fork/vfork event first, and report
> that event to gdb ahead of all the other kinds of events.

That seems to work well. Attached is the patch doing this.

The commit's revision log gives I think a reasonable overview of
what the issue is and how the patch fixes it. But more information
that I found while I was investigating this which is only tangential
to the issue can be found in the following message, exchanged on
this list:

    https://www.sourceware.org/ml/gdb-patches/2016-05/msg00207.html
    https://www.sourceware.org/ml/gdb-patches/2016-05/msg00209.html
    https://www.sourceware.org/ml/gdb-patches/2016-06/msg00391.html

gdb/gdbserver/ChangeLog:

        * linux-low.c (select_fork_vfork_lwp_callback): New function.
        (select_event_lwp): Give priority fork/vfork events over all
        other types of events.

gdb/testsuite/ChangeLog:

        * gdb.ada/bp_and_fork: New test.

Tested on x86_64-linux.
OK to apply?

Thanks!
-- 
Joel

>From cccc5072db8eeefbdfe11840c4e6e2330bac46a8 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Fri, 24 Jun 2016 11:56:32 -0400
Subject: [PATCH] GDB internal-error debugging threaded program with breakpoint
 and forks

Debugging through gdbserver a program uses threads hitting breakpoints
as well as doing a fork/exec, can lead to an internal error. This
happens randomly, since it depends on timing, but here is what we can
see from the user's perpective:

    % gdb a_test
    (gdb) break a_test.adb:30
    (gdb) break a_test.adb:39
    (gdb) target remote my_board:4444
    (gdb) continue
    Continuing.
    [...]
    [New Thread 866.868]
    [New Thread 866.869]
    [New Thread 870.870]
    /[...]/gdb/thread.c:89: internal-error: thread_info* inferior_thread(): Assertion `tp' failed.
    A problem internal to GDB has been detected,
    further debugging may prove unreliable.
    Quit this debugging session? (y or n)

What happens is that the inferior forks around the same time its
threads hit one of the breakpoints we inserted. As a result,
one single round of linux_wait_for_event_filtered, is able to
fetch breakpoints events from our inferior's two threads, as well
as the fork event reported by the inferior's main thread, all
in one go.

What GDBserver then has to do next, is decide which of these events
to report to GDB first. Currently, it gives priority to threads that
we are single-stepping, and then just grabs one of the threads
that have a pending event, selected at random (see select_event_lwp).
At the point where see the problem, the single-step-out-of-breakpoint
has already been completed, so we do not have any thread doing
a single-step, and so GDBserver selects one of the threads that
received a SIGTRAP. This can been seen by looking at the debug
traces, which contain the following hint:

        SEL: Found 3 SIGTRAP events, selecting #1

This leads GDBserver to report a SIGTRAP event on that thread.
Processing that event, GDB requests the updated list of threads,
which now contains the forked process' thread. Seeing that thread
with a different PID which confuses GDB, eventually leading to
the internal error.

This patch fixes the issue by make sure GDBserver always reports
fork/vfork events first. That way, GDB is aware that the new thread
comes from the fork/vfork event. In particular, in the mode where
we are in (follow the parent only), it knows to ignore the new
thread, thus avoiding the issue entirely.

gdb/gdbserver/ChangeLog:

        * linux-low.c (select_fork_vfork_lwp_callback): New function.
        (select_event_lwp): Give priority fork/vfork events over all
        other types of events.

gdb/testsuite/ChangeLog:

        * gdb.ada/bp_and_fork: New test.

Tested on x86_64-linux.
---
 gdb/gdbserver/linux-low.c                    | 44 +++++++++++++++++-
 gdb/testsuite/gdb.ada/bp_and_fork.exp        | 41 +++++++++++++++++
 gdb/testsuite/gdb.ada/bp_and_fork/a_test.adb | 68 ++++++++++++++++++++++++++++
 3 files changed, 152 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.ada/bp_and_fork.exp
 create mode 100644 gdb/testsuite/gdb.ada/bp_and_fork/a_test.adb

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index dd92e78..d71595b 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -2826,6 +2826,23 @@ count_events_callback (struct inferior_list_entry *entry, void *data)
   return 0;
 }
 
+/* Select the first LWP (if any) for which a fork or vfork event
+   was reported.  */
+
+static int
+select_fork_vfork_lwp_callback (struct inferior_list_entry *entry, void *data)
+{
+  struct thread_info *thread = (struct thread_info *) entry;
+  struct lwp_info *lp = get_thread_lwp (thread);
+
+  if ((lp->waitstatus.kind == TARGET_WAITKIND_FORKED
+       || thread->last_status.kind == TARGET_WAITKIND_VFORKED)
+      && lp->status_pending_p)
+    return 1;
+  else
+    return 0;
+}
+
 /* Select the LWP (if any) that is currently being single-stepped.  */
 
 static int
@@ -2871,6 +2888,31 @@ select_event_lwp (struct lwp_info **orig_lp)
   int random_selector;
   struct thread_info *event_thread = NULL;
 
+  /* If an LWP forked/vforked, select that LWP over all the others,
+     so as to tell GDB right away about the the new child process
+     being the result of a fork/vfork.
+
+     It is important to do so first, because GDB needs to be told
+     about fork/vfork threads to allow GDB to separate the new
+     process' thread from the threads of the current (parent)
+     inferior.  In particular, we ran into an issue where GDB
+     requested the list of threads which included both parent
+     and the thread of the fork/vfork, and because GDB didn't know
+     about the fork/vfork, did not ignore the fork/vfork. This
+     eventually caused an internal error because that thread had
+     a PID for which there was not corresponding thread (see
+     inferior_thread).  */
+  event_thread
+    = (struct thread_info *) find_inferior (&all_threads,
+					    select_fork_vfork_lwp_callback,
+					    NULL);
+  if (event_thread != NULL)
+    {
+      if (debug_threads)
+	debug_printf ("SEL: Select fork/vfork %s\n",
+		      target_pid_to_str (ptid_of (event_thread)));
+    }
+
   /* In all-stop, give preference to the LWP that is being
      single-stepped.  There will be at most one, and it's the LWP that
      the core is most interested in.  If we didn't do this, then we'd
@@ -2879,7 +2921,7 @@ select_event_lwp (struct lwp_info **orig_lp)
      report the pending SIGTRAP, and the core, not having stepped the
      thread, wouldn't understand what the trap was for, and therefore
      would report it to the user as a random signal.  */
-  if (!non_stop)
+  if (event_thread == NULL && !non_stop)
     {
       event_thread
 	= (struct thread_info *) find_inferior (&all_threads,
diff --git a/gdb/testsuite/gdb.ada/bp_and_fork.exp b/gdb/testsuite/gdb.ada/bp_and_fork.exp
new file mode 100644
index 0000000..e9ec08c
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/bp_and_fork.exp
@@ -0,0 +1,41 @@
+# Copyright 2016 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib "ada.exp"
+
+standard_ada_testfile a_test
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug]] != "" } {
+  return -1
+}
+
+clean_restart ${testfile}
+
+set bp_loc_1 [gdb_get_line_number "Task 1" ${testdir}/a_test.adb]
+gdb_test "break a_test.adb:$bp_loc_1" \
+         "Breakpoint $decimal at $hex: file .*a_test.adb, line $decimal\\."
+
+set bp_loc_2 [gdb_get_line_number "Task 2" ${testdir}/a_test.adb]
+gdb_test "break a_test.adb:$bp_loc_2" \
+         "Breakpoint $decimal at $hex: file .*a_test.adb, line $decimal\\."
+
+gdb_run_cmd
+gdb_test "" \
+         "Breakpoint $decimal, a_test.task$decimal \\(.*\\).*" \
+         "run to first breakpoint"
+
+gdb_test "continue" \
+         "Breakpoint $decimal, a_test.task$decimal \\(.*\\).*" \
+         "continue to second breakpoint"
diff --git a/gdb/testsuite/gdb.ada/bp_and_fork/a_test.adb b/gdb/testsuite/gdb.ada/bp_and_fork/a_test.adb
new file mode 100644
index 0000000..095c2eb
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/bp_and_fork/a_test.adb
@@ -0,0 +1,68 @@
+--  Copyright 2003-2016 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+with System;
+with Text_Io;
+
+procedure A_Test is
+
+  Command : constant String := "echo Hello World &" & Ascii.Nul;
+  Status : Integer;
+
+  ---------------------------------
+  -- System shell command interface
+  ---------------------------------
+  function Shell_Cmd (Command : in System.Address) return Integer;
+  pragma Import (C, Shell_Cmd, "system");
+
+  ------------------
+  -- Start 2 threads
+  ------------------
+
+  task Task1 is
+    entry Start_Task;
+  end Task1;
+
+  task Task2 is
+    entry Start_Task;
+  end Task2;
+
+  task body Task1 is
+  begin
+    select  -- Task 1
+      accept Start_Task;
+    or
+      terminate;
+    end select;
+  end Task1;
+
+  task body Task2 is
+  begin
+    select  -- Task 2
+      accept Start_Task;
+    or
+      terminate;
+    end select;
+  end Task2;
+
+  procedure My_Routine is
+  begin
+    Text_Io.Put_Line ("Program successful");
+  end My_Routine;
+
+begin
+  Status := Shell_Cmd (Command'Address);
+  My_Routine;
+end A_Test;
-- 
2.1.4