* [patch] Fix for PR gdb/10838
@ 2009-11-12 0:35 Paul Pluzhnikov
2009-11-12 0:43 ` Daniel Jacobowitz
0 siblings, 1 reply; 10+ messages in thread
From: Paul Pluzhnikov @ 2009-11-12 0:35 UTC (permalink / raw)
To: gdb-patches; +Cc: ppluzhnikov
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 */
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [patch] Fix for PR gdb/10838 2009-11-12 0:35 [patch] Fix for PR gdb/10838 Paul Pluzhnikov @ 2009-11-12 0:43 ` Daniel Jacobowitz 2009-11-12 0:48 ` Paul Pluzhnikov 0 siblings, 1 reply; 10+ messages in thread From: Daniel Jacobowitz @ 2009-11-12 0:43 UTC (permalink / raw) To: Paul Pluzhnikov; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] Fix for PR gdb/10838 2009-11-12 0:43 ` Daniel Jacobowitz @ 2009-11-12 0:48 ` Paul Pluzhnikov 2009-11-14 2:04 ` Paul Pluzhnikov 0 siblings, 1 reply; 10+ messages in thread From: Paul Pluzhnikov @ 2009-11-12 0:48 UTC (permalink / raw) To: Paul Pluzhnikov, gdb-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] Fix for PR gdb/10838 2009-11-12 0:48 ` Paul Pluzhnikov @ 2009-11-14 2:04 ` Paul Pluzhnikov 2009-11-15 17:37 ` Daniel Jacobowitz 2009-11-16 15:11 ` Pedro Alves 0 siblings, 2 replies; 10+ messages in thread From: Paul Pluzhnikov @ 2009-11-14 2:04 UTC (permalink / raw) To: Paul Pluzhnikov, gdb-patches; +Cc: Doug Evans [-- 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) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] Fix for PR gdb/10838 2009-11-14 2:04 ` Paul Pluzhnikov @ 2009-11-15 17:37 ` Daniel Jacobowitz 2009-11-15 17:43 ` Doug Evans 2009-11-16 15:11 ` Pedro Alves 1 sibling, 1 reply; 10+ messages in thread From: Daniel Jacobowitz @ 2009-11-15 17:37 UTC (permalink / raw) To: Paul Pluzhnikov; +Cc: gdb-patches, Doug Evans 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] Fix for PR gdb/10838 2009-11-15 17:37 ` Daniel Jacobowitz @ 2009-11-15 17:43 ` Doug Evans 2009-11-15 17:47 ` Daniel Jacobowitz 0 siblings, 1 reply; 10+ messages in thread From: Doug Evans @ 2009-11-15 17:43 UTC (permalink / raw) To: Paul Pluzhnikov, gdb-patches 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] Fix for PR gdb/10838 2009-11-15 17:43 ` Doug Evans @ 2009-11-15 17:47 ` Daniel Jacobowitz 0 siblings, 0 replies; 10+ messages in thread From: Daniel Jacobowitz @ 2009-11-15 17:47 UTC (permalink / raw) To: Doug Evans; +Cc: Paul Pluzhnikov, gdb-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] Fix for PR gdb/10838 2009-11-14 2:04 ` Paul Pluzhnikov 2009-11-15 17:37 ` Daniel Jacobowitz @ 2009-11-16 15:11 ` Pedro Alves 2009-11-16 15:14 ` Pedro Alves 2009-11-16 16:05 ` Paul Pluzhnikov 1 sibling, 2 replies; 10+ messages in thread From: Pedro Alves @ 2009-11-16 15:11 UTC (permalink / raw) To: gdb-patches; +Cc: Paul Pluzhnikov, Doug Evans, Daniel Jacobowitz 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); ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] Fix for PR gdb/10838 2009-11-16 15:11 ` Pedro Alves @ 2009-11-16 15:14 ` Pedro Alves 2009-11-16 16:05 ` Paul Pluzhnikov 1 sibling, 0 replies; 10+ messages in thread From: Pedro Alves @ 2009-11-16 15:14 UTC (permalink / raw) To: gdb-patches; +Cc: Paul Pluzhnikov, Doug Evans, Daniel Jacobowitz 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] Fix for PR gdb/10838 2009-11-16 15:11 ` Pedro Alves 2009-11-16 15:14 ` Pedro Alves @ 2009-11-16 16:05 ` Paul Pluzhnikov 1 sibling, 0 replies; 10+ messages in thread From: Paul Pluzhnikov @ 2009-11-16 16:05 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Doug Evans, Daniel Jacobowitz 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2009-11-16 16:05 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-11-12 0:35 [patch] Fix for PR gdb/10838 Paul Pluzhnikov 2009-11-12 0:43 ` Daniel Jacobowitz 2009-11-12 0:48 ` Paul Pluzhnikov 2009-11-14 2:04 ` Paul Pluzhnikov 2009-11-15 17:37 ` Daniel Jacobowitz 2009-11-15 17:43 ` Doug Evans 2009-11-15 17:47 ` Daniel Jacobowitz 2009-11-16 15:11 ` Pedro Alves 2009-11-16 15:14 ` Pedro Alves 2009-11-16 16:05 ` Paul Pluzhnikov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox