[RFC] Fix for gdb crash in "info thread" after exec().

[RFC] Fix for gdb crash in "info thread" after exec().

[Paul Pluzhnikov]

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

Greetings,

Gdb (CVS Head) crashes when 'info thread' is executed after program exec()s:

$ cat execl.c
#include <unistd.h>
int
main (int argc, char* argv[])
{
  if (argc == 1)
    execl (argv[0], argv[0], "second", NULL);

  return 0;
}

$ gcc -g execl.c -pthread && ./gdb -nx -q a.out
(gdb) b main
Breakpoint 1 at 0x8048388: file execl.c, line 5.
(gdb) r
Starting program: /usr/local/build/gdb/a.out
[Thread debugging using libthread_db enabled]

Breakpoint 1, main (argc=1, argv=0xffffd744) at execl.c:5
5         if (argc == 1)
(gdb) c
Continuing.
warning: Error removing breakpoint -2
warning: Error removing breakpoint -3
[New process 5418]
Executing new program: /usr/local/build/gdb/a.out
warning: Cannot initialize thread debugging library: generic error
[Thread debugging using libthread_db enabled]

Breakpoint 1, main (argc=2, argv=0xffffd744) at execl.c:5
5         if (argc == 1)
(gdb) inf thread
../../src/gdb/linux-nat.c:2960: internal-error:
linux_nat_thread_alive: Assertion `is_lwp (ptid)' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) y


The problem is that gdb maintains thread list which is not properly
re-initialized after exec().

Attached patch (originally by Doug Evans <dje@google.com>) fixes
that, and adds a test case for it.

Thanks,
-- 
Paul Pluzhnikov

ChangeLog:
2008-05-12  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* gdb/linux-thread-dl.c (check_for_thread_db): reinitialize
	thread list after exec().

testsuite/ChangeLog:
2008-05-12  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* gdb.threads/execl.c, gdb.threads/execl.exp: Add test for
	"info threads" after exec.

[-- Attachment #2: gdb-20080512-patch.txt --]
[-- Type: text/plain, Size: 3392 bytes --]

ChangeLog:
2008-05-12  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* gdb/linux-thread-dl.c (check_for_thread_db): reinitialize
	thread list after exec().

testsuite/ChangeLog:
2008-05-12  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* gdb.threads/execl.c, gdb.threads/execl.exp: Add test for
	"info threads" after exec.


Index: gdb/linux-thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-thread-db.c,v
retrieving revision 1.42
diff -u -p -u -r1.42 linux-thread-db.c
--- gdb/linux-thread-db.c	4 May 2008 14:12:34 -0000	1.42
+++ gdb/linux-thread-db.c	13 May 2008 01:28:48 -0000
@@ -613,6 +613,10 @@ check_for_thread_db (void)
       push_target (&thread_db_ops);
       using_thread_db = 1;
 
+      /* Existing entries in thread_list might not have thread_info.private
+	 set, so clear it out.  */
+      init_thread_list ();
+
       enable_thread_event_reporting ();
       thread_db_find_new_threads ();
       break;
Index: gdb/testsuite/gdb.threads/execl.c
===================================================================
RCS file: gdb/testsuite/gdb.threads/execl.c
diff -N gdb/testsuite/gdb.threads/execl.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.threads/execl.c	13 May 2008 01:28:48 -0000
@@ -0,0 +1,12 @@
+/* Test handling thread control across an execl.  */
+
+#include <unistd.h>
+
+int
+main (int argc, char* argv[])
+{
+  if (argc == 1)
+    execl (argv[0], argv[0], "second", NULL);
+
+  return 0;
+}
Index: gdb/testsuite/gdb.threads/execl.exp
===================================================================
RCS file: gdb/testsuite/gdb.threads/execl.exp
diff -N gdb/testsuite/gdb.threads/execl.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.threads/execl.exp	13 May 2008 01:28:48 -0000
@@ -0,0 +1,48 @@
+# Copyright (C) 2008 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/>.
+
+# Please email any bugs, comments, and/or additions to this file to:
+# bug-gdb@prep.ai.mit.edu
+
+# Test handling of threads across an execl.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+set testfile "execl"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug "incdir=${objdir}"]] != "" } {
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+runto_main
+
+# When continuing from this point we'll hit the breakpoint in main()
+# again, this time in the exec'd process.
+gdb_test "continue" ".*Breakpoint 1, main.*" \
+    "continue across exec"
+
+gdb_test "info threads" ".*" "info threads after exec"
+
+gdb_test "continue" ".*Program exited normally\\." \
+    "continue to end"

Re: [RFC] Fix for gdb crash in "info thread" after exec().

[Paul Pluzhnikov]

Ping.

http://sourceware.org/ml/gdb-patches/2008-05/msg00386.html

On Mon, May 12, 2008 at 6:38 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> Greetings,
>
> Gdb (CVS Head) crashes when 'info thread' is executed after program exec()s:

-- 
Paul Pluzhnikov

Re: [RFC] Fix for gdb crash in "info thread" after exec().

[Paul Pluzhnikov]

Ping.

http://sourceware.org/ml/gdb-patches/2008-05/msg00386.html

Also re-sending the patch as inline plain-text.

--
Paul Pluzhnikov

ChangeLog:
2008-05-12  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* gdb/linux-thread-dl.c (check_for_thread_db): reinitialize
	thread list after exec().

testsuite/ChangeLog:
2008-05-12  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* gdb.threads/execl.c, gdb.threads/execl.exp: Add test for
	"info threads" after exec.


Index: gdb/linux-thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-thread-db.c,v
retrieving revision 1.42
diff -u -p -u -r1.42 linux-thread-db.c
--- gdb/linux-thread-db.c	4 May 2008 14:12:34 -0000	1.42
+++ gdb/linux-thread-db.c	13 May 2008 01:28:48 -0000
@@ -613,6 +613,10 @@ check_for_thread_db (void)
       push_target (&thread_db_ops);
       using_thread_db = 1;
 
+      /* Existing entries in thread_list might not have thread_info.private
+	 set, so clear it out.  */
+      init_thread_list ();
+
       enable_thread_event_reporting ();
       thread_db_find_new_threads ();
       break;
Index: gdb/testsuite/gdb.threads/execl.c
===================================================================
RCS file: gdb/testsuite/gdb.threads/execl.c
diff -N gdb/testsuite/gdb.threads/execl.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.threads/execl.c	13 May 2008 01:28:48 -0000
@@ -0,0 +1,12 @@
+/* Test handling thread control across an execl.  */
+
+#include <unistd.h>
+
+int
+main (int argc, char* argv[])
+{
+  if (argc == 1)
+    execl (argv[0], argv[0], "second", NULL);
+
+  return 0;
+}
Index: gdb/testsuite/gdb.threads/execl.exp
===================================================================
RCS file: gdb/testsuite/gdb.threads/execl.exp
diff -N gdb/testsuite/gdb.threads/execl.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.threads/execl.exp	13 May 2008 01:28:48 -0000
@@ -0,0 +1,48 @@
+# Copyright (C) 2008 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/>.
+
+# Please email any bugs, comments, and/or additions to this file to:
+# bug-gdb@prep.ai.mit.edu
+
+# Test handling of threads across an execl.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+set testfile "execl"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug "incdir=${objdir}"]] != "" } {
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+runto_main
+
+# When continuing from this point we'll hit the breakpoint in main()
+# again, this time in the exec'd process.
+gdb_test "continue" ".*Breakpoint 1, main.*" \
+    "continue across exec"
+
+gdb_test "info threads" ".*" "info threads after exec"
+
+gdb_test "continue" ".*Program exited normally\\." \
+    "continue to end"

Re: [RFC] Fix for gdb crash in "info thread" after exec().

[Pedro Alves]

A Tuesday 27 May 2008 20:07:02, Paul Pluzhnikov wrote:
> Ping.
>
> http://sourceware.org/ml/gdb-patches/2008-05/msg00386.html
>
> Also re-sending the patch as inline plain-text.
>

I agree that the threads should not survive across an exec, but
the crash you're reporting suggests that something is
trimming the ptid when it shouldn't, and that ptid is getting
into the thread list.

Here's a patch that handles this a bit more generically:
 http://sourceware.org/ml/gdb-patches/2008-05/msg00230.html

In your test case, what is probably happening, is that 
linux-nat.c/linux-thread-db.c is escaping an event ptid which
isn't in the thread list, which doesn't have a ptid.lwp member
set, so you're hitting this in handle_inferior_event:

  /* If it's a new process, add it to the thread database */

  ecs->new_thread_event = (!ptid_equal (ecs->ptid, inferior_ptid)
			   && !ptid_equal (ecs->ptid, minus_one_ptid)
			   && !in_thread_list (ecs->ptid));

  if (ecs->ws.kind != TARGET_WAITKIND_EXITED
      && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED && ecs->new_thread_event)
    add_thread (ecs->ptid);

Which I've been claiming is bad...

Could you confirm that this hunk of my patch, 

Index: src/gdb/linux-thread-db.c
===================================================================
--- src.orig/gdb/linux-thread-db.c	2008-05-06 12:22:31.000000000 +0100
+++ src/gdb/linux-thread-db.c	2008-05-06 12:53:18.000000000 +0100
@@ -840,7 +840,7 @@ thread_db_wait (ptid_t ptid, struct targ
       unpush_target (&thread_db_ops);
       using_thread_db = 0;
 
-      return pid_to_ptid (GET_PID (ptid));
+      return ptid;
     }
 
   /* If we do not know about the main thread yet, this would be a good time 
to

... fixes the issue, and that you were hitting that new_thread_event piece
in infrun.c:handle_inferior_event while handling a TARGET_WAITKIND_EXECD ?

We may need some more interface cleanup to clear the current thread
list across an exec, if the original process had threads, but I don't
think your call is in the right place.

-- 
Pedro Alves

Re: [RFC] Fix for gdb crash in "info thread" after exec().

[Paul Pluzhnikov]

On Tue, May 27, 2008 at 1:06 PM, Pedro Alves <pedro@codesourcery.com> wrote:

> Could you confirm that this hunk of my patch,
>
> Index: src/gdb/linux-thread-db.c
> ===================================================================
> --- src.orig/gdb/linux-thread-db.c      2008-05-06 12:22:31.000000000 +0100
> +++ src/gdb/linux-thread-db.c   2008-05-06 12:53:18.000000000 +0100
> @@ -840,7 +840,7 @@ thread_db_wait (ptid_t ptid, struct targ
>       unpush_target (&thread_db_ops);
>       using_thread_db = 0;
>
> -      return pid_to_ptid (GET_PID (ptid));
> +      return ptid;
>     }
>
>   /* If we do not know about the main thread yet, this would be a good time
> to
>
> ... fixes the issue,

Confirmed.

> and that you were hitting that new_thread_event piece
> in infrun.c:handle_inferior_event while handling a TARGET_WAITKIND_EXECD ?

I am not sure I understand that second question correctly.

The 'ecs->new_thread_event' is set to 0 while handling
TARGET_WAITKIND_EXECD, and add_thread() is not called for it.

> We may need some more interface cleanup to clear the current thread
> list across an exec, if the original process had threads, but I don't
> think your call is in the right place.

Yes, it did feel out of place to me as well.

Thanks,
-- 
Paul Pluzhnikov

Re: [RFC] Fix for gdb crash in "info thread" after exec().

[Paul Pluzhnikov]

On Tue, May 27, 2008 at 1:06 PM, Pedro Alves <pedro@codesourcery.com> wrote:

> Could you confirm that this hunk of my patch,
>
> Index: src/gdb/linux-thread-db.c
> ===================================================================
> --- src.orig/gdb/linux-thread-db.c      2008-05-06 12:22:31.000000000 +0100
> +++ src/gdb/linux-thread-db.c   2008-05-06 12:53:18.000000000 +0100
> @@ -840,7 +840,7 @@ thread_db_wait (ptid_t ptid, struct targ
>       unpush_target (&thread_db_ops);
>       using_thread_db = 0;
>
> -      return pid_to_ptid (GET_PID (ptid));
> +      return ptid;
>     }
>
>   /* If we do not know about the main thread yet, this would be a good time
> to
>
> ... fixes the issue,

Confirmed.

> and that you were hitting that new_thread_event piece
> in infrun.c:handle_inferior_event while handling a TARGET_WAITKIND_EXECD ?

I am not sure I understand that second question correctly.

The 'ecs->new_thread_event' is set to 0 while handling
TARGET_WAITKIND_EXECD, and add_thread() is not called for it.

> We may need some more interface cleanup to clear the current thread
> list across an exec, if the original process had threads, but I don't
> think your call is in the right place.

Yes, it did feel out of place to me as well.

Thanks,
-- 
Paul Pluzhnikov