[patch+7.8?] Fix 'gcore' with exited threads

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Mon, 23 Jun 2014 17:06:37 +0200, Pedro Alves wrote:
> On 06/09/2014 09:30 PM, Jan Kratochvil wrote:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1099405
> >
> > Program received signal SIGABRT, Aborted.
> > [...]
> > (gdb) gcore foobar
> > Couldn't get registers: No such process.
> > (gdb) info threads
> > [...]
> > (gdb) gcore foobar
> > Saved corefile foobar
> > (gdb)
> >
> > gcore tries to access the exited thread:
> > [Thread 0x7ffff7fce700 (LWP 6895) exited]
> > ptrace(PTRACE_GETREGS, 6895, 0, 0x7fff18167dd0) = -1 ESRCH (No such process)
> 
> Note this will still happen if you have the exited thread selected,
> as in that case the thread can't be deleted:

Confirmed, testcase extended.


> It seems to me linux_corefile_thread_callback should skip exited
> threads too.

Done.


> > Without the TRY_CATCH protection testsuite FAILs for:
> > 	FAIL: gdb.threads/gcore-thread.exp: save a zeroed-threads corefile
> > 	FAIL: gdb.threads/gcore-thread.exp: core0file: re-load generated corefile (bad file format)
> 
> What does the log show ?

gcore .../gdb/testsuite/gdb.threads/gcore-thread0.test^M
Cannot find new threads: debugger service failed^M
(gdb) FAIL: gdb.threads/gcore-thread.exp: save a zeroed-threads corefile
+
core .../gdb/testsuite/gdb.threads/gcore-thread0.test^M
".../gdb/testsuite/gdb.threads/gcore-thread0.test" is not a core dump: File format not recognized^M
(gdb) FAIL: gdb.threads/gcore-thread.exp: core0file: re-load generated corefile (bad file format)


> > Maybe the TRY_CATCH could be more inside update_thread_list().
> 
> I'll assume "info threads" is failing at that point too
> then.  Maybe we should downgrade whatever error is triggering
> to a warning?

It has other disadvantages.  Warning may get lost in some other messages and
user may miss some thread(s) during a falsely successfully finished command.


> > Similar update_thread_list() call is IMO missing in procfs_make_note_section()
> > but I do not have where to verify that change.
> 
> I wonder whether we should update the thread list in generic
> code (write_gcore_file).

This is one of the general problems of GDB that one cannot do any change
affecting platforms which one cannot (or at least not easily enough) test on.

Platform not automatically being tested by Jenkins for any patch submitted to
Gerrit should be officially unsupported.


> > +gdb_test_multiple "help gcore" "help gcore" {
> > +    -re "Undefined command: .gcore.*\r\n$gdb_prompt $" {
> 
> Is this coming from copy/paste of existing tests?

Yes.


> I believe this is is stale -- gcore.o has been in COMMON_OBS for a while
> now.  I think the actual error will be whatever the default for the target
> method throws.

Likewise two paragraphs above.  Not sure what I should do with it:
 * If I remove the test from this my testcase and someone later fixes
   the 6 existing instances of this test this only test will remain unfixed.
 * If I waste the time reading the sources what should be the expected output
   I risk the test will not work anyway as I read it wrongly.
   And I will probably wrongly modify 6 other testsuite cases.
 * I can try to find a platform without gcore but in most cases in the past my
   attempts to build GDB on any non-Linux platforms failed.

I find this problem generally out of the scope of this mail thread / patch.


> > +	# gcore command not supported -- nothing to test here.
> > +	unsupported "gdb does not support gcore on this target"
> > +	return -1
> > +    }
> > +    -re "Save a core file .*\r\n$gdb_prompt $" {
> > +	pass "help gcore"
> > +    }
> > +}


No regressions on {x86_64,x86_64-m32,i686}-fedora21pre-linux-gnu.


Thanks,
Jan

gdb/
2014-08-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* linux-tdep.c (linux_corefile_thread_callback): Ignore THREAD_EXITED.
	(linux_make_corefile_notes): call update_thread_list, protected against
	exceptions.

gdb/testsuite/
2014-08-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.threads/gcore-stale-thread.c: New file.
	* gdb.threads/gcore-stale-thread.exp: New file.

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index d0f1106..875f32e 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1194,6 +1194,11 @@ linux_corefile_thread_callback (struct thread_info *info, void *data)
 {
   struct linux_corefile_thread_data *args = data;
 
+  /* It can be current thread in non-stop mode
+     which cannot be removed by update_thread_list.  */
+  if (info->state == THREAD_EXITED)
+    return 0;
+
   if (ptid_get_pid (info->ptid) == args->pid)
     {
       struct cleanup *old_chain;
@@ -1445,6 +1450,7 @@ linux_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size,
   char *note_data = NULL;
   gdb_byte *auxv;
   int auxv_len;
+  volatile struct gdb_exception e;
 
   if (linux_fill_prpsinfo (&prpsinfo))
     {
@@ -1468,6 +1474,12 @@ linux_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size,
     }
 
   /* Thread register information.  */
+  TRY_CATCH (e, RETURN_MASK_ERROR)
+    {
+      update_thread_list ();
+    }
+  if (e.reason < 0)
+    exception_print (gdb_stderr, e);
   thread_args.gdbarch = gdbarch;
   thread_args.pid = ptid_get_pid (inferior_ptid);
   thread_args.obfd = obfd;
diff --git a/gdb/testsuite/gdb.threads/gcore-stale-thread.c b/gdb/testsuite/gdb.threads/gcore-stale-thread.c
new file mode 100644
index 0000000..944acba
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/gcore-stale-thread.c
@@ -0,0 +1,48 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 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/>.  */
+
+#include <pthread.h>
+#include <assert.h>
+
+static pthread_t main_thread;
+
+static void *
+start (void *arg)
+{
+  int i;
+
+  i = pthread_join (main_thread, NULL);
+  assert (i == 0);
+
+  return arg; /* break-here */
+}
+
+int
+main (void)
+{
+  pthread_t thread;
+  int i;
+
+  main_thread = pthread_self ();
+
+  i = pthread_create (&thread, NULL, start, NULL);
+  assert (i == 0);
+
+  pthread_exit (NULL);
+  assert (0);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/gcore-stale-thread.exp b/gdb/testsuite/gdb.threads/gcore-stale-thread.exp
new file mode 100644
index 0000000..dc73056
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/gcore-stale-thread.exp
@@ -0,0 +1,71 @@
+# Copyright 2014 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/>.
+
+# This file was written by Michael Snyder (msnyder@redhat.com)
+# This is a test for the gdb command "generate-core-file".
+
+standard_testfile
+set corefile [standard_output_file ${testfile}.core]
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable debug] != ""} {
+    return -1
+}
+
+clean_restart ${testfile}
+
+gdb_test_no_output "set non-stop on"
+
+gdb_test_multiple "help gcore" "help gcore" {
+    -re "Undefined command: .gcore.*\r\n$gdb_prompt $" {
+	# gcore command not supported -- nothing to test here.
+	unsupported "gdb does not support gcore on this target"
+	return -1
+    }
+    -re "Save a core file .*\r\n$gdb_prompt $" {
+	pass "help gcore"
+    }
+}
+
+if { ! [ runto_main ] } then {
+    return -1
+}
+
+gdb_test_multiple "info threads" "threads are supported" {
+    -re ".* main .*\r\n$gdb_prompt $" {
+	# OK, threads are supported.
+    }
+    -re "\r\n$gdb_prompt $" {
+	unsupported "gdb does not support threads on this target"
+	return -1
+    }
+}
+
+gdb_breakpoint ${srcfile}:[gdb_get_line_number "break-here"]
+# gdb_continue_to_breakpoint does not work as it uses "$gdb_prompt $" regex
+# which does not work due to the output: (gdb) [Thread ... exited]
+set name "continue to breakpoint: break-here"
+gdb_test_multiple "continue" $name {
+    -re "Breakpoint .* (at|in) .* break-here .*\r\n$gdb_prompt " {
+	pass $name
+    }
+}
+
+gdb_gcore_cmd "$corefile" "save a corefile"
+
+# Do not run "info threads" before "gcore" as it could workaround the bug
+# by discarding non-current exited threads.
+gdb_test "info threads" \
+         {The current thread <Thread ID 1> has terminated\.  See `help thread'\.} \
+	 "exited thread is current due to non-stop"