RFH: failed assert debugging threaded+fork program over gdbserver

RFH: failed assert debugging threaded+fork program over gdbserver

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 4731 bytes --]

Hello,

I have noticed the following problem, when debugging a program which
uses both threads and fork. The program is attached in copy, and
it was compiled by simply doing:

    % gnatmake -g a_test

The issue appears only randomly, but it seems to show up fairly
reliably when using certain versions of GNU/Linux such as RHES7,
or WRSLinux. I also see it on Ubuntu, but less reliably. Here is
what I have found, debugging on WRSLinux (we set it up as a cross,
but it should be the same with native GNU/Linux distros):

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

The error happens because GDBserver returns a list of threads
to GDB where a new thread as a different PID (870 in the case
above, instead of 866).

What this does is that it makes remote_notice_new_inferior
think that there is a new inferior (which is actually true,
in fact), thus causing it to call remote_add_inferior, which
does the following:

      /* In the traditional debugging scenario, there's a 1-1 match
         between program/address spaces.  We simply bind the inferior
         to the program space's address space.  */
      inf = current_inferior ();
      inferior_appeared (inf, pid);

These two lines cause the PID of the current inferior, to be changed
to the PID of the new process (from the fork). This is where I *think*
we're making the mistake; see below...

However, remote_notice_new_inferior also calls notice_new_inferior
a few lines later, which starts by setting up a cleanup to restoreu
the current thread:

  if (!ptid_equal (inferior_ptid, null_ptid))
    make_cleanup_restore_current_thread ();

At that point in time, the current thread, which is the thread
that receveived the event, is one of the thread belonging to
the original inferior (pid=866). That's what we setup to restore.
And unfortunately, the restoration does not go according to plan,
because our inferior list now still has one inferior in it, except
that its PID is no longer 866, but rather 870. If we look at
thread.c::do_restore_current_thread_cleanup, we see:

    tp = find_thread_ptid (old->inferior_ptid);

    /* If the previously selected thread belonged to a process that has
       in the mean time been deleted (due to normal exit, detach, etc.),
       then don't revert back to it, but instead simply drop back to no
       thread selected.  */
    if (tp
        && find_inferior_ptid (tp->ptid) != NULL)
      restore_current_thread (old->inferior_ptid);
    else
      {
        restore_current_thread (null_ptid);
        set_current_inferior (find_inferior_id (old->inf_id));
      }

In our case, find_inferior_ptid no longer finds an inferior with
the old PID, and so we go into the else branch, causing us to
set the inferior_ptid to the null_ptid. This causes problems
a little later, when doing a normal_stop:

    /* Notify observers about the stop.  This is where the interpreters
       print the stop event.  */
    if (!ptid_equal (inferior_ptid, null_ptid))
      observer_notify_normal_stop (inferior_thread ()->control.stop_bpstat,
                                   stop_print_frame);
    else
      observer_notify_normal_stop (NULL, stop_print_frame);

In our case, we're in the "else" branch. This leads to cli_on_normal_stop,
which calls print_stop_event -> print_stop_location, which starts by
calling inferior_thread:

  struct thread_info *tp = inferior_thread ();

And looking at inferior_thread, we see:

  struct thread_info *tp = find_thread_ptid (inferior_ptid);
  gdb_assert (tp);

We trip the assertion before inferior_ptid is the null_ptid.

At first sight, I think that the main problem is that we muck the
current_inferior's pid when we really shouldn't. I'm not really sure
how the new PID should be handled though, which is why I'm asking
for advice here.

I think it also unearthed a secondary issue - looks like normal_stop
really isn't prepared to handle a null inferior_ptid, even though
the fact that we call it after having checked that inferior_ptid is
null indicates that we should. But what does it mean, to be showing
where we stopped, when we don't know which thread caused the stop???
I think discussing this separately would be best, but I wanted to
mention it here, so it doesn't get overlooked.

Any advice on how I should be fixing the issue?

Thanks!
-- 
Joel

[-- Attachment #2: a_test.adb --]
[-- Type: text/plain, Size: 1652 bytes --]

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

  --------------------------------------------------------------
  -- From "gvd" set break point on line below by selecting line.
  -- Run program by clicking "Run" button, some warnings are
  -- logged and process goes into limbo.
  --
  -- Note, it appears to work if you "gdb" command line to set
  -- break point as follows: "b My_Routine".
  -- However, it does NOT work using "b 50".
  -- Turns out that "gdb" works intermittently, only.
  --
  -- Another interesting anomoly is that the process never seems
  -- to quit when run from "gvd" or "gdb" and no break points,
  -- but does run to completion without the debugger.
  --------------------------------------------------------------
  My_Routine;
end A_Test;

Re: RFH: failed assert debugging threaded+fork program over gdbserver

[Don Breazeal]

On 5/12/2016 10:16 AM, Joel Brobecker wrote:
> Hello,
> 
> I have noticed the following problem, when debugging a program which
> uses both threads and fork. The program is attached in copy, and
> it was compiled by simply doing:
> 
>     % gnatmake -g a_test
> 
> The issue appears only randomly, but it seems to show up fairly
> reliably when using certain versions of GNU/Linux such as RHES7,
> or WRSLinux. I also see it on Ubuntu, but less reliably. Here is
> what I have found, debugging on WRSLinux (we set it up as a cross,
> but it should be the same with native GNU/Linux distros):
> 
>     % 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) 
> 
> The error happens because GDBserver returns a list of threads
> to GDB where a new thread as a different PID (870 in the case
> above, instead of 866).

Hi Joel,
This is not supposed to happen.  In remote.c:remote_update_thread_list
there is a call to remove_new_fork_children that is explicitly supposed
to prevent this scenario.  The new fork child thread should be deleted
from the thread list ("context") before we call remote_notice_new_inferior.

We don't want the remote to report new threads related to the fork child
until after we have handled the fork using infrun.c:follow_fork. (Note:
it looks like the function comment for remove_new_fork_children is
stale, since there are scenarios other than being stopped at a fork
catchpoint where this can occur.)

> Any advice on how I should be fixing the issue?

It looks like there is a case that remove_new_fork_children isn't
handling correctly with your test+target(s). I'd start there to find out
how the new thread is getting through.
Thanks
--Don

Re: RFH: failed assert debugging threaded+fork program over gdbserver

[Joel Brobecker]

Hello,

I finally had some time to look into this again. I think I have
a better understanding.

On Thu, May 12, 2016 at 10:42:04AM -0700, Don Breazeal wrote:
> On 5/12/2016 10:16 AM, Joel Brobecker wrote:
> > Hello,
> > 
> > I have noticed the following problem, when debugging a program which
> > uses both threads and fork. The program is attached in copy, and
> > it was compiled by simply doing:
> > 
> >     % gnatmake -g a_test
> > 
> > The issue appears only randomly, but it seems to show up fairly
> > reliably when using certain versions of GNU/Linux such as RHES7,
> > or WRSLinux. I also see it on Ubuntu, but less reliably. Here is
> > what I have found, debugging on WRSLinux (we set it up as a cross,
> > but it should be the same with native GNU/Linux distros):
> > 
> >     % 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) 
> > 
> > The error happens because GDBserver returns a list of threads
> > to GDB where a new thread as a different PID (870 in the case
> > above, instead of 866).
> 
> This is not supposed to happen.  In remote.c:remote_update_thread_list
> there is a call to remove_new_fork_children that is explicitly supposed
> to prevent this scenario.  The new fork child thread should be deleted
> from the thread list ("context") before we call remote_notice_new_inferior.
> 
> We don't want the remote to report new threads related to the fork child
> until after we have handled the fork using infrun.c:follow_fork. (Note:
> it looks like the function comment for remove_new_fork_children is
> stale, since there are scenarios other than being stopped at a fork
> catchpoint where this can occur.)
> 
> > Any advice on how I should be fixing the issue?
> 
> It looks like there is a case that remove_new_fork_children isn't
> handling correctly with your test+target(s). I'd start there to find out
> how the new thread is getting through.

I started by looking at remove_new_fork_children, which, in essence,
remove threads whose target_waitstatus->kind is fork or vforked.
And for that to happen, I think the remote (gdbserver) has to send
GDB a stop:fork packet, something like this:

    Packet received: T05fork:p4bc4.4bc4;06:0100000000000000;07:d0ddffffff7f0000;10:6a4f83f7ff7f0000;thread:p4ba5.4ba5;core:5;

However, this is not what happens, in our case. When continuing,
we just receive a SIGTRAP event, at which point GDB asks for the
thread list, and by then, the thread for the forked process is
in the list, leading to the situation described above.

Deciphering the gdbserver logs, I think I understand why this is
happening. After we're done single-stepping out of our breakpoint,
and we have resumed our program we go into a wait loop in
linux_wait_for_event_filtered, which, as I understand it,
pulls event from the kernel, but without doing a blocking wait
(WNOHANG), and as long as events are found, we keep waiting
for more:

  /* Always pull all events out of the kernel.  We'll randomly select
     an event LWP out of all that have events, to prevent
     starvation.  */
  while (event_child == NULL)
    {
      [...]
      ret = my_waitpid (-1, wstatp, options | WNOHANG);
      if (ret > 0)
        {
          [...]
          continue;
        }

After resuming, we go through a couple of loops where I think
we discover the new threads that are getting started. Each
time, we resume the threads via:

      /* Now that we've pulled all events out of the kernel, resume
         LWPs that don't have an interesting event to report.  */
      if (stopping_threads == NOT_STOPPING_THREADS)
        for_each_inferior (&all_threads, resume_stopped_resumed_lwps);

But shortly after, we get the following batch of events:

| leader_pid=2178, leader_lp!=NULL=1, num_lwps=3, zombie=0
| sigsuspend'ing
| sigchld_handler
| my_waitpid (-1, 0x40000001)
| my_waitpid (-1, 0x40000001): status(137f), 2190
| LWFE: waitpid(-1, ...) returned 2190, ERRNO-OK
| LLW: waitpid 2190 received Stopped (signal) (stopped)
| my_waitpid (-1, 0x40000001)
| my_waitpid (-1, 0x40000001): status(57f), 2189
| LWFE: waitpid(-1, ...) returned 2189, ERRNO-OK
| LLW: waitpid 2189 received Trace/breakpoint trap (stopped)
| pc is 0x1000336c
| CSBB: LWP 2178.2189 stopped by software breakpoint
| my_waitpid (-1, 0x40000001)
| my_waitpid (-1, 0x40000001): status(57f), 2188
| LWFE: waitpid(-1, ...) returned 2188, ERRNO-OK
| LLW: waitpid 2188 received Trace/breakpoint trap (stopped)
| pc is 0x10003218
| CSBB: LWP 2178.2188 stopped by software breakpoint
| my_waitpid (-1, 0x40000001)
| my_waitpid (-1, 0x40000001): status(1057f), 2178
| LWFE: waitpid(-1, ...) returned 2178, ERRNO-OK
| LLW: waitpid 2178 received Trace/breakpoint trap (stopped)
| pc is 0xfe87158
| HEW: Got fork event from LWP 2178, new child is 2190  <<<<--- fork!
| 
| pc is 0xfe87158
| my_waitpid (-1, 0x40000001)
| my_waitpid (-1, 0x40000001): status(ffffffff), 0
| LWFE: waitpid(-1, ...) returned 0, ERRNO-OK

This tells me that we get 3 SIGTRAP events from our program's
threads hitting a breakpoint, *and* the fork event. The fork
event is confirmed by some logs printed right after:

| LWP 2178: extended event with waitstatus status->kind = forked
| Hit a non-gdbserver trap event.

Now that we've have interesting stuff to report, we decide to
stop all LWPS:

| >>>> entering stop_all_lwps
| stop_all_lwps (stop, except=none)
| wait_for_sigstop: pulling events
| my_waitpid (-1, 0x40000001)
| my_waitpid (-1, 0x40000001): status(ffffffff), 0
| LWFE: waitpid(-1, ...) returned 0, ERRNO-OK
| leader_pid=2178, leader_lp!=NULL=1, num_lwps=3, zombie=0
| leader_pid=2190, leader_lp!=NULL=1, num_lwps=1, zombie=0
| LLW: exit (no unwaited-for LWP)
| stop_all_lwps done, setting stopping_threads back to !stopping
| <<<< exiting stop_all_lwps

Because all our threads had hit a breakpoint, we can see above
that none of the lwps got sent a sigstop signal, and therefore
waitpid returns zero as expected - confirmed by "(no unwaited-for LWP)".

And this is where I think things go wrong - instead of reporting
the fork event to GDB, we look at all our threads, find that none
of them was being single-stepped, and so select one of the threads
that got a SIGTRAP at random, reporting the event of that thread:

| SEL: Found 3 SIGTRAP events, selecting #1
| Checking whether LWP 2178 needs to move out of the jump pad...no
| Checking whether LWP 2190 needs to move out of the jump pad...no
| linux_wait_1 ret = LWP 2178.2188, 1, 5
| <<<< exiting linux_wait_1
| Writing resume reply for LWP 2178.2188:1
| putpkt ("$T05swbreak:;01:482009e0;40:10003218;thread:p882.88c;core:0;#75"); [noack mode]

This is in linux-low.c::select_event_lwp

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.

But I ran out of time to try this today - so I decided to send my notes
about this so far. I hope I am on the right track!

-- 
Joel

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]

[-- Attachment #1: Type: text/plain, Size: 1071 bytes --]

> 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

[-- Attachment #2: 0001-GDB-internal-error-debugging-threaded-program-with-b.patch --]
[-- Type: text/x-diff, Size: 9578 bytes --]

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

RE: RFH: failed assert debugging threaded+fork program over gdbserver

[Breazeal, Don]

> -----Original Message-----
> From: Joel Brobecker [mailto:brobecker@adacore.com]
> Sent: Thursday, June 23, 2016 4:00 PM
> To: Breazeal, Don
> Cc: gdb-patches@sourceware.org
> Subject: Re: RFH: failed assert debugging threaded+fork program over
> gdbserver
> 
> Hello,
> 
> I finally had some time to look into this again. I think I have a
> better understanding.
> 
> On Thu, May 12, 2016 at 10:42:04AM -0700, Don Breazeal wrote:
> > On 5/12/2016 10:16 AM, Joel Brobecker wrote:
> > > Hello,
> > >
> > > I have noticed the following problem, when debugging a program
> which
> > > uses both threads and fork. The program is attached in copy, and
> it
> > > was compiled by simply doing:
> > >
> > >     % gnatmake -g a_test
> > >
> > > The issue appears only randomly, but it seems to show up fairly
> > > reliably when using certain versions of GNU/Linux such as RHES7,
> or
> > > WRSLinux. I also see it on Ubuntu, but less reliably. Here is what
> I
> > > have found, debugging on WRSLinux (we set it up as a cross, but it
> > > should be the same with native GNU/Linux distros):
> > >
> > >     % 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)
> > >
> > > The error happens because GDBserver returns a list of threads to
> GDB
> > > where a new thread as a different PID (870 in the case above,
> > > instead of 866).
> >
> > This is not supposed to happen.  In
> remote.c:remote_update_thread_list
> > there is a call to remove_new_fork_children that is explicitly
> > supposed to prevent this scenario.  The new fork child thread should
> > be deleted from the thread list ("context") before we call
> remote_notice_new_inferior.
> >
> > We don't want the remote to report new threads related to the fork
> > child until after we have handled the fork using
> infrun.c:follow_fork. (Note:
> > it looks like the function comment for remove_new_fork_children is
> > stale, since there are scenarios other than being stopped at a fork
> > catchpoint where this can occur.)
> >
> > > Any advice on how I should be fixing the issue?
> >
> > It looks like there is a case that remove_new_fork_children isn't
> > handling correctly with your test+target(s). I'd start there to find
> > out how the new thread is getting through.
> 
> I started by looking at remove_new_fork_children, which, in essence,
> remove threads whose target_waitstatus->kind is fork or vforked.
> And for that to happen, I think the remote (gdbserver) has to send GDB
> a stop:fork packet, something like this:
> 
>     Packet received:
> T05fork:p4bc4.4bc4;06:0100000000000000;07:d0ddffffff7f0000;10:6a4f83f7
> ff7f0000;thread:p4ba5.4ba5;core:5;
> 
> However, this is not what happens, in our case. When continuing, we
> just receive a SIGTRAP event, at which point GDB asks for the thread
> list, and by then, the thread for the forked process is in the list,
> leading to the situation described above.
> 
> Deciphering the gdbserver logs, I think I understand why this is
> happening. After we're done single-stepping out of our breakpoint, and
> we have resumed our program we go into a wait loop in
> linux_wait_for_event_filtered, which, as I understand it, pulls event
> from the kernel, but without doing a blocking wait (WNOHANG), and as
> long as events are found, we keep waiting for more:
> 
>   /* Always pull all events out of the kernel.  We'll randomly select
>      an event LWP out of all that have events, to prevent
>      starvation.  */
>   while (event_child == NULL)
>     {
>       [...]
>       ret = my_waitpid (-1, wstatp, options | WNOHANG);
>       if (ret > 0)
>         {
>           [...]
>           continue;
>         }
> 
> After resuming, we go through a couple of loops where I think we
> discover the new threads that are getting started. Each time, we
> resume the threads via:
> 
>       /* Now that we've pulled all events out of the kernel, resume
>          LWPs that don't have an interesting event to report.  */
>       if (stopping_threads == NOT_STOPPING_THREADS)
>         for_each_inferior (&all_threads, resume_stopped_resumed_lwps);
> 
> But shortly after, we get the following batch of events:
> 
> | leader_pid=2178, leader_lp!=NULL=1, num_lwps=3, zombie=0
> | sigsuspend'ing sigchld_handler my_waitpid (-1, 0x40000001)
> my_waitpid
> | (-1, 0x40000001): status(137f), 2190
> | LWFE: waitpid(-1, ...) returned 2190, ERRNO-OK
> | LLW: waitpid 2190 received Stopped (signal) (stopped) my_waitpid (-
> 1,
> | 0x40000001) my_waitpid (-1, 0x40000001): status(57f), 2189
> | LWFE: waitpid(-1, ...) returned 2189, ERRNO-OK
> | LLW: waitpid 2189 received Trace/breakpoint trap (stopped) pc is
> | 0x1000336c
> | CSBB: LWP 2178.2189 stopped by software breakpoint my_waitpid (-1,
> | 0x40000001) my_waitpid (-1, 0x40000001): status(57f), 2188
> | LWFE: waitpid(-1, ...) returned 2188, ERRNO-OK
> | LLW: waitpid 2188 received Trace/breakpoint trap (stopped) pc is
> | 0x10003218
> | CSBB: LWP 2178.2188 stopped by software breakpoint my_waitpid (-1,
> | 0x40000001) my_waitpid (-1, 0x40000001): status(1057f), 2178
> | LWFE: waitpid(-1, ...) returned 2178, ERRNO-OK
> | LLW: waitpid 2178 received Trace/breakpoint trap (stopped) pc is
> | 0xfe87158
> | HEW: Got fork event from LWP 2178, new child is 2190  <<<<--- fork!
> |
> | pc is 0xfe87158
> | my_waitpid (-1, 0x40000001)
> | my_waitpid (-1, 0x40000001): status(ffffffff), 0
> | LWFE: waitpid(-1, ...) returned 0, ERRNO-OK
> 
> This tells me that we get 3 SIGTRAP events from our program's threads
> hitting a breakpoint, *and* the fork event. The fork event is
> confirmed by some logs printed right after:
> 
> | LWP 2178: extended event with waitstatus status->kind = forked Hit a
> | non-gdbserver trap event.
> 
> Now that we've have interesting stuff to report, we decide to stop all
> LWPS:
> 
> | >>>> entering stop_all_lwps
> | stop_all_lwps (stop, except=none)
> | wait_for_sigstop: pulling events
> | my_waitpid (-1, 0x40000001)
> | my_waitpid (-1, 0x40000001): status(ffffffff), 0
> | LWFE: waitpid(-1, ...) returned 0, ERRNO-OK leader_pid=2178,
> | leader_lp!=NULL=1, num_lwps=3, zombie=0 leader_pid=2190,
> | leader_lp!=NULL=1, num_lwps=1, zombie=0
> | LLW: exit (no unwaited-for LWP)
> | stop_all_lwps done, setting stopping_threads back to !stopping <<<<
> | exiting stop_all_lwps
> 
> Because all our threads had hit a breakpoint, we can see above that
> none of the lwps got sent a sigstop signal, and therefore waitpid
> returns zero as expected - confirmed by "(no unwaited-for LWP)".
> 
> And this is where I think things go wrong - instead of reporting the
> fork event to GDB, we look at all our threads, find that none of them
> was being single-stepped, and so select one of the threads that got a
> SIGTRAP at random, reporting the event of that thread:
> 
> | SEL: Found 3 SIGTRAP events, selecting #1 Checking whether LWP 2178
> | needs to move out of the jump pad...no Checking whether LWP 2190
> needs
> | to move out of the jump pad...no
> | linux_wait_1 ret = LWP 2178.2188, 1, 5 <<<< exiting linux_wait_1
> | Writing resume reply for LWP 2178.2188:1 putpkt
> | ("$T05swbreak:;01:482009e0;40:10003218;thread:p882.88c;core:0;#75");
> | [noack mode]
> 
> This is in linux-low.c::select_event_lwp
> 
> 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.
> 
> But I ran out of time to try this today - so I decided to send my
> notes about this so far. I hope I am on the right track!
> 
> --
> Joel

Hi Joel,
Thanks for following up on this.  Your diagnosis makes sense to me.  As for modifications to select_event_lwp, Pedro would be better suited to comment on potential side effects.
Thanks
--Don

Re: RFA/gdbserver: GDB internal-error debugging threaded program with breakpoint and forks

[Pedro Alves]

I haven't gone through this with fine-tooth comb yet, but,
will we still have the same problem if _two_ threads (or inferiors...)
fork at the "same" time, and we end up reporting one fork, while
leaving the another one pending?

Thanks,
Pedro Alves

Re: RFA/gdbserver: GDB internal-error debugging threaded program with breakpoint and forks

[Joel Brobecker]

> I haven't gone through this with fine-tooth comb yet, but,
> will we still have the same problem if _two_ threads (or inferiors...)
> fork at the "same" time, and we end up reporting one fork, while
> leaving the another one pending?

At the moment, I do not think so, because we seem to just process
the fork even without requesting an update of the thread list.
This is an extract of the remote protocol transmissions that
show us resume the execution of our program (vCont;c), receive
the fork event (T05fork), do a bunch of stuff to handle the
fork event (and in particular "D"etaching from the child process),
followed by the resumption of our program's execution (vCont;c):

    Sending packet: $vCont;c:p2992.-1#b4...
    Packet received: T05fork:p299c.299c;01:bffff5e0;40:0fe87158;thread:p2992.2992;core:1;
    Sending packet: $Hgp299c.299c#5b...Packet received: OK
    Sending packet: $z0,1000224c,4#f2...Packet received: OK
    Sending packet: $z0,10003218,4#c5...Packet received: OK
    Sending packet: $z0,1000336c,4#f6...Packet received: OK
    Sending packet: $D;299c#86...Packet received: OK
    Sending packet: $vCont;c:p2992.-1#b4...Packet received: T05swbreak:;01:482009e0;40:10003218;thread:p2992.299a;core:0;

I wish I could give you a stronger justification, but at least
we seem to be OK, so the patch could give us a fix while we think
a more solid approach through.

-- 
Joel

Re: RFA/gdbserver: GDB internal-error debugging threaded program with breakpoint and forks

[Pedro Alves]

On 06/24/2016 11:36 PM, Joel Brobecker wrote:
>> I haven't gone through this with fine-tooth comb yet, but,
>> will we still have the same problem if _two_ threads (or inferiors...)
>> fork at the "same" time, and we end up reporting one fork, while
>> leaving the another one pending?
> 
> At the moment, I do not think so, because we seem to just process
> the fork even without requesting an update of the thread list.

"catch fork" would make us stop though.

Thanks,
Pedro Alves

Re: RFA/gdbserver: GDB internal-error debugging threaded program with breakpoint and forks

[Joel Brobecker]

> >> I haven't gone through this with fine-tooth comb yet, but,
> >> will we still have the same problem if _two_ threads (or inferiors...)
> >> fork at the "same" time, and we end up reporting one fork, while
> >> leaving the another one pending?
> > 
> > At the moment, I do not think so, because we seem to just process
> > the fork even without requesting an update of the thread list.
> 
> "catch fork" would make us stop though.

:-(. Most likely. I had the weekend to mull this over. The only
possible solutions I can see are:

  a. Make gdbserver "hide" the threads that are children of forks
     until we've reported the corresponding fork event to GDB.

     But then, I think it's unclear what to do if the user does
     a "step" or "continue" while you have multiple pending
     fork events. That's probably a question that's likely not
     specific to forks, as you might have the same issue when
     requesting an action after seeing the first of multiple
     events received at the same time. Perhaps simply just return
     the next event without resuming anything? Is that what we do?

  b. Somehow enhance GDB to handle the extra unknown threads
     more gracefully.

I don't really see how (b) could work. It seems that (a) would
be more promising. That said, I would still consider my current
patch, as reporting the forks early allow us to either detach
from them earlier.

-- 
Joel

Re: RFA/gdbserver: GDB internal-error debugging threaded program with breakpoint and forks

[Pedro Alves]

On 06/27/2016 11:32 PM, Joel Brobecker wrote:
>>>> I haven't gone through this with fine-tooth comb yet, but,
>>>> will we still have the same problem if _two_ threads (or inferiors...)
>>>> fork at the "same" time, and we end up reporting one fork, while
>>>> leaving the another one pending?
>>>
>>> At the moment, I do not think so, because we seem to just process
>>> the fork even without requesting an update of the thread list.
>>
>> "catch fork" would make us stop though.
> 
> :-(. Most likely. I had the weekend to mull this over. The only
> possible solutions I can see are:
> 
>   a. Make gdbserver "hide" the threads that are children of forks
>      until we've reported the corresponding fork event to GDB.
> 

Agreed, I think we need to do this.  It's somewhat what
linux-nat.c does, except linux-nat.c hides the fork child
until target_follow_fork time.  

I say similar, instead of "just like", because the RSP doesn't
have a "follow fork / detach fork" concept.  That's because the model
of instead making the child visible as a regular process as soon as we
see the parent fork allows using regular detach, remove breakpoints, etc.
packets against the fork child.

We could make the server always hide the child until it gets some
new "unhide child now" packet, but I think all it would avoid is gdb
looking over the already-reported events and check whether a fork is
pending, so it seems unnecessary.

>      But then, I think it's unclear what to do if the user does
>      a "step" or "continue" while you have multiple pending
>      fork events. That's probably a question that's likely not
>      specific to forks, as you might have the same issue when
>      requesting an action after seeing the first of multiple
>      events received at the same time. Perhaps simply just return
>      the next event without resuming anything? Is that what we do?

Yes, that's what we do.  For gdb there's no difference between
the other pending event having happened to trigger at the same
time as the last reported event, vs it triggering immediately
on next resume.

> 
>   b. Somehow enhance GDB to handle the extra unknown threads
>      more gracefully.
> 
> I don't really see how (b) could work. It seems that (a) would
> be more promising. That said, I would still consider my current
> patch, as reporting the forks early allow us to either detach
> from them earlier.

My usual thought process is this: imagine we had (a) already.  Would we
have a particularly strong reason to complicate the code and do (b) on
its own?  Seems like not.  We could apply the same rationale for preferring
to report any other thread stopped at a breakpoint before the fork
events (so that we could move them past their breakpoints earlier).  Or
always prefer the stepping thread, as that's the thread the user is most
interested in (*).  Etc.

(*) - IIRC, the reason we prefer a stepped thread first is for correctness,
not because that's what the user is focused in.  It used to be that if a step
event got pending, and we reported some other event first, later when the
pending step event is finally reported as a plain SIGTRAP, if the thread that had
a pending step was now continued instead of stepped, infrun wouldn't
understanding what this SIGTRAP was about, since the thread was no longer
supposed to be single-stepping, and would thus report the SIGTRAP to the
user as a spurious signal.  With "maint set target-non-stop on", which is still
not the default with target remote, infrun.c:clear_proceed_status_thread
handles this scenario on the gdb side and discards the single-step, but with
plain all-stop, the spurious SIGTRAP probably would still happen.

Thanks,
Pedro Alves

Re: RFA/gdbserver: GDB internal-error debugging threaded program with breakpoint and forks

[Joel Brobecker]

> >   a. Make gdbserver "hide" the threads that are children of forks
> >      until we've reported the corresponding fork event to GDB.
> > 
> 
> Agreed, I think we need to do this.  It's somewhat what
> linux-nat.c does, except linux-nat.c hides the fork child
> until target_follow_fork time.  
[...]
> That said, I would still consider my current
> > patch, as reporting the forks early allow us to either detach
> > from them earlier.
> 
> My usual thought process is this: imagine we had (a) already.  Would we
> have a particularly strong reason to complicate the code and do (b) on
> its own?  Seems like not.  We could apply the same rationale for preferring
> to report any other thread stopped at a breakpoint before the fork
> events (so that we could move them past their breakpoints earlier).  Or
> always prefer the stepping thread, as that's the thread the user is most
> interested in (*).  Etc.
> 
> (*) - IIRC, the reason we prefer a stepped thread first is for
> correctness, not because that's what the user is focused in.  It used
> to be that if a step event got pending, and we reported some other
> event first, later when the pending step event is finally reported as
> a plain SIGTRAP, if the thread that had a pending step was now
> continued instead of stepped, infrun wouldn't understanding what this
> SIGTRAP was about, since the thread was no longer supposed to be
> single-stepping, and would thus report the SIGTRAP to the user as a
> spurious signal.  With "maint set target-non-stop on", which is still
> not the default with target remote,
> infrun.c:clear_proceed_status_thread handles this scenario on the gdb
> side and discards the single-step, but with plain all-stop, the
> spurious SIGTRAP probably would still happen.

I would have thought that we'd want GDB and GDBserver to be in sync
as quickly as possible, so as to release the new inferiors, but I guess
that doesn't really make any difference in practice. The issue with
reporting SIGTRAP from an old single-step is even more convincing that
my patch is actually wrong.  Sigh...

-- 
Joel