[patch] Fix for PR gdb/10838

[patch] Fix for PR gdb/10838

[Paul Pluzhnikov]

Greetings,

In http://sourceware.org/bugzilla/show_bug.cgi?id=10838
gdb attach; detach; attach sequence leaves libpthread in hosed state.

The root cause turned out to be that this:

      td_event_emptyset (&events);
      info->td_ta_set_event_p (info->thread_agent, &events);

is a no-op -- glibc ORs new event bits in, but doesn't clear any already
set bits. To clear them, one must call td_ta_clear_event() instead.

Here is a patch which fixes this in gdb and gdbserver.

Tested (GDB only) on Linux/x86_64 with no regressions.

Thanks,
--
Paul Pluzhnikov

gdb/ChangeLog:

2009-11-11  Paul Pluzhnikov  <ppluzhnikov@google.com>

	PR gdb/10838
	* linux-thread-db.c (thread_db_info): New member.
	(disable_thread_event_reporting): Call td_ta_clear_event.

gdbserver/ChangeLog:

2009-11-11  Paul Pluzhnikov  <ppluzhnikov@google.com>

	PR gdb/10838
	* thread-db.c (thread_db_free): Call td_ta_clear_event.

Index: linux-thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-thread-db.c,v
retrieving revision 1.67
diff -u -p -u -r1.67 linux-thread-db.c
--- linux-thread-db.c	3 Nov 2009 17:14:56 -0000	1.67
+++ linux-thread-db.c	12 Nov 2009 00:21:02 -0000
@@ -141,6 +141,8 @@ struct thread_db_info
 				  td_event_e event, td_notify_t *ptr);
   td_err_e (*td_ta_set_event_p) (const td_thragent_t *ta,
 				 td_thr_events_t *event);
+  td_err_e (*td_ta_clear_event_p) (const td_thragent_t *ta,
+				   td_thr_events_t *event);
   td_err_e (*td_ta_event_getmsg_p) (const td_thragent_t *ta,
 				    td_event_msg_t *msg);
 
@@ -701,6 +703,7 @@ try_thread_db_load_1 (struct thread_db_i
   /* These are not essential.  */
   info->td_ta_event_addr_p = dlsym (info->handle, "td_ta_event_addr");
   info->td_ta_set_event_p = dlsym (info->handle, "td_ta_set_event");
+  info->td_ta_clear_event_p = dlsym (info->handle, "td_ta_clear_event");
   info->td_ta_event_getmsg_p = dlsym (info->handle, "td_ta_event_getmsg");
   info->td_thr_event_enable_p = dlsym (info->handle, "td_thr_event_enable");
   info->td_thr_tls_get_addr_p = dlsym (info->handle, "td_thr_tls_get_addr");
@@ -907,14 +910,14 @@ thread_db_load (void)
 static void
 disable_thread_event_reporting (struct thread_db_info *info)
 {
-  if (info->td_ta_set_event_p != NULL)
+  if (info->td_ta_clear_event_p != NULL)
     {
       td_thr_events_t events;
 
       /* Set the process wide mask saying we aren't interested in any
 	 events anymore.  */
-      td_event_emptyset (&events);
-      info->td_ta_set_event_p (info->thread_agent, &events);
+      td_event_fillset (&events);
+      info->td_ta_clear_event_p (info->thread_agent, &events);
     }
 
   info->td_create_bp_addr = 0;
Index: gdbserver/thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/thread-db.c,v
retrieving revision 1.26
diff -u -p -u -r1.26 thread-db.c
--- gdbserver/thread-db.c	29 Oct 2009 17:43:44 -0000	1.26
+++ gdbserver/thread-db.c	12 Nov 2009 00:21:02 -0000
@@ -759,6 +759,19 @@ thread_db_free (struct process_info *pro
     {
 #ifndef USE_LIBTHREAD_DB_DIRECTLY
       td_err_e (*td_ta_delete_p) (td_thragent_t *);
+      td_err_e (*td_ta_clear_event_p) (const td_thragent_t *ta,
+				       td_thr_events_t *event);
+
+      td_ta_clear_event_p = dlsym (thread_db->handle, "td_ta_clear_event");
+      if (td_ta_clear_event_p != NULL)
+	{
+	  td_thr_events_t events;
+
+	  /* Set the process wide mask saying we aren't interested in any
+	     events anymore.  */
+	  td_event_fillset (&events);
+	  (*td_ta_clear_event_p) (thread_db->thread_agent, &events);
+	}
 
       td_ta_delete_p = dlsym (thread_db->handle, "td_ta_delete");
       if (td_ta_delete_p != NULL)
@@ -766,6 +779,10 @@ thread_db_free (struct process_info *pro
 
       dlclose (thread_db->handle);
 #else
+      td_thd_events_t events;
+
+      td_event_fillset (&events);
+      td_ta_clear_event (thread_db->thread_agent, &events);
       td_ta_delete (thread_db->thread_agent);
 #endif  /* USE_LIBTHREAD_DB_DIRECTLY  */
 

Re: [patch] Fix for PR gdb/10838

[Daniel Jacobowitz]

On Wed, Nov 11, 2009 at 04:35:03PM -0800, Paul Pluzhnikov wrote:
> Greetings,
> 
> In http://sourceware.org/bugzilla/show_bug.cgi?id=10838
> gdb attach; detach; attach sequence leaves libpthread in hosed state.
> 
> The root cause turned out to be that this:
> 
>       td_event_emptyset (&events);
>       info->td_ta_set_event_p (info->thread_agent, &events);
> 
> is a no-op -- glibc ORs new event bits in, but doesn't clear any already
> set bits. To clear them, one must call td_ta_clear_event() instead.
> 
> Here is a patch which fixes this in gdb and gdbserver.
> 
> Tested (GDB only) on Linux/x86_64 with no regressions.

OK, thanks.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Fix for PR gdb/10838

[Paul Pluzhnikov]

On Wed, Nov 11, 2009 at 4:42 PM, Daniel Jacobowitz <drow@false.org> wrote:
> On Wed, Nov 11, 2009 at 04:35:03PM -0800, Paul Pluzhnikov wrote:

> OK, thanks.

That was fast :-)
So committed.

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Fix for PR gdb/10838

[Paul Pluzhnikov]

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

2009/11/11 Paul Pluzhnikov <ppluzhnikov@google.com>:

> That was fast :-)

Too fast, as it turns out :-(
I didn't test the gdbserver part, but this patch introduced a
gdbserver crash here:

Program terminated with signal 11, Segmentation fault.

#0  0x0000000000408b75 in look_up_one_symbol (name=0x7f6dc44ee3a2
"__nptl_threads_events", addrp=0x7fffcce720c8) at
../../../src/gdb/gdbserver/remote-utils.c:1374
1374      for (sym = proc->symbol_cache; sym; sym = sym->next)

proc is NULL.
Called from:

#0  0x0000000000408b75 in look_up_one_symbol () at
../../../src/gdb/gdbserver/remote-utils.c:1374
#1  0x000000000041ae0e in ps_pglobal_lookup () at
../../../src/gdb/gdbserver/proc-service.c:68
#2  0x00007f6dc44ed60d in td_ta_clear_event () from /lib/libthread_db.so.1
#3  0x000000000041ac8b in thread_db_free (proc=0x6311a0) at
../../../src/gdb/gdbserver/thread-db.c:773
#4  0x0000000000411b44 in linux_remove_process (process=0x6311a0) at
../../../src/gdb/gdbserver/linux-low.c:267

Attached patch fixes that (tested with gdbserver on Linux/x86_64 this time).
The gdb.mi/mi-non-stop-exit.exp test is still failing (regression
since 2009-10-27), but apparently for a different reason.

Thanks,
-- 
Paul Pluzhnikov


2009-11-13  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* proc-service.c (ps_pglobal_lookup): Handle error return
	* remote-utils.c (look_up_one_symbol): Don't crash if the process is gone.

[-- Attachment #2: gdbserver-crash-20091113.txt --]
[-- Type: text/plain, Size: 1220 bytes --]

Index: gdbserver/proc-service.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/proc-service.c,v
retrieving revision 1.14
diff -u -p -u -r1.14 proc-service.c
--- gdbserver/proc-service.c	13 Oct 2009 13:51:21 -0000	1.14
+++ gdbserver/proc-service.c	14 Nov 2009 01:04:48 -0000
@@ -65,7 +65,7 @@ ps_pglobal_lookup (gdb_ps_prochandle_t p
 {
   CORE_ADDR addr;
 
-  if (look_up_one_symbol (name, &addr) == 0)
+  if (look_up_one_symbol (name, &addr) <= 0)
     return PS_NOSYM;
 
   *sym_addr = (psaddr_t) (unsigned long) addr;
Index: gdbserver/remote-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
retrieving revision 1.68
diff -u -p -u -r1.68 remote-utils.c
--- gdbserver/remote-utils.c	6 Jul 2009 18:31:20 -0000	1.68
+++ gdbserver/remote-utils.c	14 Nov 2009 01:04:48 -0000
@@ -1369,6 +1369,9 @@ look_up_one_symbol (const char *name, CO
   struct process_info *proc;
 
   proc = current_process ();
+  if (proc == NULL)
+    /* Could happen if the process has just exited.  */
+    return -1;
 
   /* Check the cache first.  */
   for (sym = proc->symbol_cache; sym; sym = sym->next)

Re: [patch] Fix for PR gdb/10838

[Daniel Jacobowitz]

On Fri, Nov 13, 2009 at 06:04:41PM -0800, Paul Pluzhnikov wrote:
> Attached patch fixes that (tested with gdbserver on Linux/x86_64 this time).
> The gdb.mi/mi-non-stop-exit.exp test is still failing (regression
> since 2009-10-27), but apparently for a different reason.

This is fine if you haven't checked it in.

Is the non-stop failure new, or just intermittent?  I believe I posted
a patch to refuse to do non-stop on targets where it won't work, but
it was limited to the remote target and I never revisited.  We don't
have x86_64 displaced stepping yet, right?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Fix for PR gdb/10838

[Doug Evans]

On Sun, Nov 15, 2009 at 9:36 AM, Daniel Jacobowitz <drow@false.org> wrote:
> We don't
> have x86_64 displaced stepping yet, right?

Actually we do.

Re: [patch] Fix for PR gdb/10838

[Daniel Jacobowitz]

On Sun, Nov 15, 2009 at 09:41:57AM -0800, Doug Evans wrote:
> On Sun, Nov 15, 2009 at 9:36 AM, Daniel Jacobowitz <drow@false.org> wrote:
> > We don't
> > have x86_64 displaced stepping yet, right?
> 
> Actually we do.

Hmm.  Maybe I should be more concerned, then.  I've been getting
intermittent non-stop failures... e.g. in mi-nsmoribund.exp.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Fix for PR gdb/10838

[Pedro Alves]

On Saturday 14 November 2009 02:04:41, Paul Pluzhnikov wrote:

> Program terminated with signal 11, Segmentation fault.
> 
> #0  0x0000000000408b75 in look_up_one_symbol (name=0x7f6dc44ee3a2
> "__nptl_threads_events", addrp=0x7fffcce720c8) at
> ../../../src/gdb/gdbserver/remote-utils.c:1374
> 1374      for (sym = proc->symbol_cache; sym; sym = sym->next)
> 
> proc is NULL.
> Called from:
> 
> #0  0x0000000000408b75 in look_up_one_symbol () at
> ../../../src/gdb/gdbserver/remote-utils.c:1374
> #1  0x000000000041ae0e in ps_pglobal_lookup () at
> ../../../src/gdb/gdbserver/proc-service.c:68
> #2  0x00007f6dc44ed60d in td_ta_clear_event () from /lib/libthread_db.so.1
> #3  0x000000000041ac8b in thread_db_free (proc=0x6311a0) at
> ../../../src/gdb/gdbserver/thread-db.c:773
> #4  0x0000000000411b44 in linux_remove_process (process=0x6311a0) at
> ../../../src/gdb/gdbserver/linux-low.c:267

Having look_up_one_symbol handle the null process case
tastes like papering over an earlier issue.

> Attached patch fixes that (tested with gdbserver on Linux/x86_64 this time).

Why are we calling td_ta_clear_event at all if the
process is gone?  I think we should do that only if detaching,
like this (untested)?  This is what GDB does too.  The fact
that td_ta_clear_event wants to poke at a dead inferior is
a good indication of something not right.  If the process is
gone (or we didn't actually manage to connect to
this thread_db), there's no event mask to clear.

Also, thread_db_free assumes its PROC argument is the
current inferior (due to the fact that proc-service.c
relies on current_inferior currently).  This assumption
could/should be removed, but I'll leave that for
another day.

-- 
Pedro Alves

2009-11-16  Pedro Alves  <pedro@codesourcery.com>

	gdb/gdbserver/
	* linux-low.c (linux_remove_process): Add `detaching' parameter.
	Pass it to thread_db_free.
	(linux_kill, linux_detach, linux_wait_1): Adjust to pass the
	proper `detaching' argument to linux_remove_process.
	* linux-low.h (thread_db_free): Add `detaching' parameter.
	* thread-db.c (thread_db_init): Pass true as `detaching' argument
	to thread_db_free.
	(thread_db_free): Add `detaching' parameter.  Only
	call td_ta_clear_event if detaching from process.

---
 gdb/gdbserver/linux-low.c |   10 +++++-----
 gdb/gdbserver/linux-low.h |    2 +-
 gdb/gdbserver/thread-db.c |   26 +++++++++++++-------------
 3 files changed, 19 insertions(+), 19 deletions(-)

Index: src/gdb/gdbserver/linux-low.c
===================================================================
--- src.orig/gdb/gdbserver/linux-low.c	2009-11-16 02:07:20.000000000 +0000
+++ src/gdb/gdbserver/linux-low.c	2009-11-16 14:31:45.000000000 +0000
@@ -259,12 +259,12 @@ linux_add_process (int pid, int attached
    also freeing all private data.  */
 
 static void
-linux_remove_process (struct process_info *process)
+linux_remove_process (struct process_info *process, int detaching)
 {
   struct process_info_private *priv = process->private;
 
 #ifdef USE_THREAD_DB
-  thread_db_free (process);
+  thread_db_free (process, detaching);
 #endif
 
   free (priv->arch_private);
@@ -663,7 +663,7 @@ linux_kill (int pid)
     } while (lwpid > 0 && WIFSTOPPED (wstat));
 
   delete_lwp (lwp);
-  linux_remove_process (process);
+  linux_remove_process (process, 0);
   return 0;
 }
 
@@ -752,7 +752,7 @@ linux_detach (int pid)
 
   delete_all_breakpoints ();
   find_inferior (&all_threads, linux_detach_one_lwp, &pid);
-  linux_remove_process (process);
+  linux_remove_process (process, 1);
   return 0;
 }
 
@@ -1378,7 +1378,7 @@ retry:
 	  struct process_info *process = find_process_pid (pid);
 
 	  delete_lwp (lwp);
-	  linux_remove_process (process);
+	  linux_remove_process (process, 0);
 
 	  current_inferior = NULL;
 
Index: src/gdb/gdbserver/linux-low.h
===================================================================
--- src.orig/gdb/gdbserver/linux-low.h	2009-11-16 02:07:20.000000000 +0000
+++ src/gdb/gdbserver/linux-low.h	2009-11-16 12:51:09.000000000 +0000
@@ -201,7 +201,7 @@ struct lwp_info *find_lwp_pid (ptid_t pt
 
 /* From thread-db.c  */
 int thread_db_init (int use_events);
-void thread_db_free (struct process_info *);
+void thread_db_free (struct process_info *, int detaching);
 int thread_db_handle_monitor_command (char *);
 int thread_db_get_tls_address (struct thread_info *thread, CORE_ADDR offset,
 			       CORE_ADDR load_module, CORE_ADDR *address);
Index: src/gdb/gdbserver/thread-db.c
===================================================================
--- src.orig/gdb/gdbserver/thread-db.c	2009-11-16 12:51:06.000000000 +0000
+++ src/gdb/gdbserver/thread-db.c	2009-11-16 13:51:22.000000000 +0000
@@ -737,7 +737,7 @@ thread_db_init (int use_events)
       if (use_events && thread_db_enable_reporting () == 0)
 	{
 	  /* Keep trying; maybe event reporting will work later.  */
-	  thread_db_free (proc);
+	  thread_db_free (proc, 0);
 	  return 0;
 	}
       thread_db_find_new_threads ();
@@ -752,38 +752,38 @@ thread_db_init (int use_events)
 /* Disconnect from libthread_db and free resources.  */
 
 void
-thread_db_free (struct process_info *proc)
+thread_db_free (struct process_info *proc, int detaching)
 {
   struct thread_db *thread_db = proc->private->thread_db;
   if (thread_db)
     {
-#ifndef USE_LIBTHREAD_DB_DIRECTLY
       td_err_e (*td_ta_delete_p) (td_thragent_t *);
       td_err_e (*td_ta_clear_event_p) (const td_thragent_t *ta,
 				       td_thr_events_t *event);
 
+#ifndef USE_LIBTHREAD_DB_DIRECTLY
       td_ta_clear_event_p = dlsym (thread_db->handle, "td_ta_clear_event");
-      if (td_ta_clear_event_p != NULL)
+      td_ta_delete_p = dlsym (thread_db->handle, "td_ta_delete");
+#else
+      td_ta_delete_p = &td_ta_delete;
+      td_ta_clear_event_p = &td_ta_clear_event;
+#endif
+
+      if (detaching && td_ta_clear_event_p != NULL)
 	{
 	  td_thr_events_t events;
 
-	  /* Set the process wide mask saying we aren't interested in any
-	     events anymore.  */
+	  /* Set the process wide mask saying we aren't interested
+	     in any events anymore.  */
 	  td_event_fillset (&events);
 	  (*td_ta_clear_event_p) (thread_db->thread_agent, &events);
 	}
 
-      td_ta_delete_p = dlsym (thread_db->handle, "td_ta_delete");
       if (td_ta_delete_p != NULL)
 	(*td_ta_delete_p) (thread_db->thread_agent);
 
+#ifndef USE_LIBTHREAD_DB_DIRECTLY
       dlclose (thread_db->handle);
-#else
-      td_thr_events_t events;
-
-      td_event_fillset (&events);
-      td_ta_clear_event (thread_db->thread_agent, &events);
-      td_ta_delete (thread_db->thread_agent);
 #endif  /* USE_LIBTHREAD_DB_DIRECTLY  */
 
       free (thread_db);

Re: [patch] Fix for PR gdb/10838

[Pedro Alves]

On Monday 16 November 2009 15:10:43, Pedro Alves wrote:

> like this (untested)?  This is what GDB does too.  The fact

I've actually tested this on x86_64-linux (after having written that...).
Fixes the crashes, has no regressions.

-- 
Pedro Alves

Re: [patch] Fix for PR gdb/10838

[Paul Pluzhnikov]

On Mon, Nov 16, 2009 at 7:10 AM, Pedro Alves <pedro@codesourcery.com> wrote:

> 2009-11-16  Pedro Alves  <pedro@codesourcery.com>
>
>        gdb/gdbserver/
>        * linux-low.c (linux_remove_process): Add `detaching' parameter.
>        Pass it to thread_db_free.

Looks good to me.

Thanks!
-- 
Paul Pluzhnikov