[patch][rfc] Allow GDB to search for the right libthread_db.so.1
Paul Pluzhnikov
ppluzhnikov@google.com
Fri Apr 10 19:06:00 GMT 2009
On Wed, Apr 8, 2009 at 2:22 PM, Thiago Jung Bauermann
<bauerman@br.ibm.com> wrote:
Thank you for comments and interest in this patch :-)
Attached is a revised version, which I believe addresses all the issues
you have raised.
>> /* Non-zero if we're using this module's target vector. */
>> -static int using_thread_db;
>> +static void *using_thread_db;
>
> The comment for this variable needs to be updated. It's not a binary
> flag anymore, so it should mention what the void * value is. The
> variable should have a different name too, to reflect its new meaning.
Done.
>> static int
>> -thread_db_load (void)
>> +try_thread_db_load_1(void *handle)
>> {
>
> I know that this function was undocumented already, but would you mind
> adding a brief comment describing what it does, and what its return
> value means?
>
> Since you are changing a bit its behaviour and purpose, it seems fair
> that you get to (briefly) document it. :-)
Done.
>> + /* Now attempt to open a connection to the thread library. */
>> + err = td_ta_new_p (&proc_handle, &thread_agent);
>> + if (err != TD_OK)
>> + {
>> + td_ta_new_p = NULL;
>> + if (info_verbose)
>> + printf_unfiltered (_("td_ta_new(): %s.\n"),
>> + thread_db_err_str (err));
>> + return 0;
>> + }
>
> If err != TD_OK && err != TD_NOLIBTHREAD, a warning should be emitted,
> like in the current version of the code:
>
> warning (_("Cannot initialize thread debugging library: %s"),
> thread_db_err_str (err));
That's not quite right: you could also see TD_VERSION here (if you try the
"wrong" libthread_db first). I've changed the code to emit a warning for
other cases.
>> @@ -447,9 +450,141 @@ thread_db_load (void)
>> td_thr_event_enable_p = dlsym (handle, "td_thr_event_enable");
>> td_thr_tls_get_addr_p = dlsym (handle, "td_thr_tls_get_addr");
>>
>> + printf_unfiltered (_("[Thread debugging using libthread_db enabled]\n"));
>> +
>> + init_thread_db_ops ();
>> + add_target (&thread_db_ops);
>
> Why can't you keep these calls in _initialize_thread_db? Isn't it wrong
> to call add_target whenever a libthread_db is loaded (I admit I don't
> know much about the target infrastructure in GDB)?
I believe you are quite correct. Fixed.
>> + handle = dlopen (library, RTLD_NOW);
>> + if (handle == NULL)
>> + {
>> + if (info_verbose)
>> + printf_unfiltered (_("dlopen(): %s.\n"), dlerror ());
>
> The parenthesis shouldn't be in the message. Also, suggest being a bit
> more descriptive: "dlopen failed: %s.\n"
Fixed.
> The other error and information messages which currently have
> parenthesis in them should also be fixed.
Fixed.
>> +static int
>> +thread_db_load_search ()
>> +{
>> + char path[PATH_MAX];
>
> This function can overflow path. Serious problem, IMHO.
Fixed.
>> + Dl_info info;
>> + const char *library = NULL;
>> + if (dladdr ((*td_ta_new_p), &info) != 0)
>> + library = info.dli_fname;
>> + if (library == NULL)
>> + library = LIBTHREAD_DB_SO;
>> + printf_unfiltered (_("Warning: guessed host libthread_db "
>> + "library \"%s\".\n"), library);
>
> Not sure this should be a warning. It's possible that the guessed
> libthread_db is correct. Or is it more likely that it's not?
With our Google-specific setting of LIBTHREAD_DB_SEARCH_PATH, we expect
correct libthread_db to always be found on that path; hence the warning.
But with empty search path (as in this patch) it definitely should not be.
Fixed accordingly.
> Also, I think it's clearer to rephrase it as:
>
> "Searched libthread_db library to use, selected %s"
>
> That is, avoid the (IMHO confusing) term "guessed library".
Done.
>> + }
>> + else
>> + printf_unfiltered (_("Warning: unable to guess libthread_db, "
>> + "thread debugging will not be available.\n"));
>
> Again, printf_unfiltered vs warning. I'd appreciate other opinions on
> this topic. Also, suggest rewording to avoid "guess":
>
> "Unable to find libthread_db matching inferior's thread library, thread
> debugging will not be available.\n".
Done.
>> +static int
>> +thread_db_load (void)
>> +{
>
> Please add a comment describing the function and its return value.
> Please also do this to the other functions which you introduced.
Done.
>> + msym = lookup_minimal_symbol ("nptl_version", NULL, NULL);
>> + if (!msym)
>> + msym = lookup_minimal_symbol ("__linuxthreads_version", NULL, NULL);
>> +
>> + if (!msym)
>> + /* No threads yet */
>> + return 0;
>
> Clever way to detect if a thread library is present. I can't comment on
> its correctness and reliability though.
I added one more symbol: __pthread_threads_events; really old versions of
LinuxThreads libpthread lacked the __linuxthreads_version.
> Perhaps others will have more
> insight. My only comment is that an alternative would be to search
> through the inferior's link map. Don't know if it would be better or
> worse.
Looking through link_map fails for statically-linked executables.
It is desireable to have a single GDB that "just works", regardless of
whether the executable was built statically or dynamically.
> Also, I assume you tested your patch in both NPTL systems and
> LinuxThread systems, right?
Yes: we have a mixture and I tested both.
>> + soname = solib_name_from_address (SYMBOL_VALUE_ADDRESS (msym));
>> + if (soname)
>> + {
>> + /* Attempt to load libthread_db from the same directory. */
>> + char path[PATH_MAX], *cp;
>> + strcpy (path, soname);
>> + cp = strrchr (path, '/');
>> + if (cp == NULL)
>> + {
>> + /* Expected to get fully resolved pathname, but got
>> + something else. Hope for the best. */
>> + printf_unfiltered (_("warning: (Internal error: "
>> + "solib_name_from_address() returned \"%s\".\n"),
>> + soname);
>> + return thread_db_load_search ();
>> + }
>
> "Internal error" has a specific meaning in GDB already, and it's not
> what you use it for here. I suggest rewording to:
>
> warning (_("Cannot obtain absolute path of thread library: %s"));
Fixed.
> Note that I changed from calling printf_unfiltered to warning. I'm not
> sure, but I gather the latter is preferred, right? (/me looks nervously
> at other GDB developers, seeking a nod.)
>
> Also, please clarify what "hope for the best" means here.
Done.
>> + printf_unfiltered (_("Using host libthread_db library \"%s\".\n"),
>> + library);
>> + last_loaded = td_ta_new_p;
>
> This message is currently only printed when verbose is on, but you
> changed it to be always printed. Please provide a rationale for the
> change.
It's an "internal support" issue: we want to know at a glance which
libthread_db got loaded.
I changed this back to "verbose on" only.
>> @@ -896,7 +993,10 @@ thread_db_wait (struct target_ops *ops,
>> {
>> remove_thread_event_breakpoints ();
>> unpush_target (&thread_db_ops);
>> + if (using_thread_db)
>> + dlclose (using_thread_db);
>> using_thread_db = 0;
>> + no_shared_libraries (NULL, 0);
>
> I don't know much about GDB's mourning process, so this is a genuine
> question: is this the appropriate place to call no_shared_libraries?
> Doesn't feel right to me.
This was a workaround for earlier bug, which (AFAICT) is fixed in current
source. I removed the call.
I've tried to add documentation changes for this patch, but can't seem to
find appropriate place for it :-(
I think "Debugging Programs with Multiple Threads" section is the most
appropriate place, but I am not sure.
Thanks,
--
Paul Pluzhnikov
-------------- next part --------------
Index: gdb_thread_db.h
===================================================================
RCS file: /cvs/src/src/gdb/gdb_thread_db.h,v
retrieving revision 1.12
diff -u -p -u -r1.12 gdb_thread_db.h
--- gdb_thread_db.h 18 Mar 2009 08:51:11 -0000 1.12
+++ gdb_thread_db.h 10 Apr 2009 00:28:27 -0000
@@ -1,5 +1,14 @@
#ifdef HAVE_THREAD_DB_H
#include <thread_db.h>
+
+#ifndef LIBTHREAD_DB_SO
+#define LIBTHREAD_DB_SO "libthread_db.so.1"
+#endif
+
+#ifndef LIBTHREAD_DB_SEARCH_PATH
+#define LIBTHREAD_DB_SEARCH_PATH ""
+#endif
+
#else
/* Copyright (C) 1999, 2000, 2007, 2008, 2009 Free Software Foundation, Inc.
Index: linux-thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-thread-db.c,v
retrieving revision 1.54
diff -u -p -u -r1.54 linux-thread-db.c
--- linux-thread-db.c 27 Feb 2009 20:34:41 -0000 1.54
+++ linux-thread-db.c 10 Apr 2009 00:28:28 -0000
@@ -26,13 +26,16 @@
#include "gdb_thread_db.h"
#include "bfd.h"
+#include "command.h"
#include "exceptions.h"
+#include "gdbcmd.h"
#include "gdbthread.h"
#include "inferior.h"
#include "symfile.h"
#include "objfiles.h"
#include "target.h"
#include "regcache.h"
+#include "solib.h"
#include "solib-svr4.h"
#include "gdbcore.h"
#include "observer.h"
@@ -44,10 +47,6 @@
#include <gnu/libc-version.h>
#endif
-#ifndef LIBTHREAD_DB_SO
-#define LIBTHREAD_DB_SO "libthread_db.so.1"
-#endif
-
/* GNU/Linux libthread_db support.
libthread_db is a library, provided along with libpthread.so, which
@@ -74,14 +73,17 @@
of the ptid_t prevents thread IDs changing when libpthread is
loaded or unloaded. */
+static char *libthread_db_search_path;
+
/* If we're running on GNU/Linux, we must explicitly attach to any new
threads. */
/* This module's target vector. */
static struct target_ops thread_db_ops;
-/* Non-zero if we're using this module's target vector. */
-static int using_thread_db;
+/* Handle from dlopen for libthread_db.so. Not NULL if we're using this
+ module's target vector. */
+static void *thread_db_handle;
/* Non-zero if we have determined the signals used by the threads
library. */
@@ -143,6 +145,9 @@ static void thread_db_find_new_threads_1
static void attach_thread (ptid_t ptid, const td_thrhandle_t *th_p,
const td_thrinfo_t *ti_p);
static void detach_thread (ptid_t ptid);
+static td_err_e enable_thread_event (td_thragent_t *thread_agent,
+ int event, CORE_ADDR *bp);
+static void enable_thread_event_reporting (void);
/* Use "struct private_thread_info" to cache thread state. This is
@@ -344,7 +349,7 @@ thread_db_attach_lwp (ptid_t ptid)
td_thrinfo_t ti;
td_err_e err;
- if (!using_thread_db)
+ if (thread_db_handle == NULL)
return 0;
/* This ptid comes from linux-nat.c, which should always fill in the
@@ -385,22 +390,17 @@ verbose_dlsym (void *handle, const char
return sym;
}
+/* Attempt to initialize dlopen()ed libthread_db, described by HANDLE.
+ Return 1 on success.
+ Failure could happen if libthread_db does not have symbols we expect,
+ or when it refuses to work with the current inferior (e.g. due to
+ version mismatch between libthread_db and libpthread). */
+
static int
-thread_db_load (void)
+try_thread_db_load_1(void *handle)
{
- void *handle;
td_err_e err;
- handle = dlopen (LIBTHREAD_DB_SO, RTLD_NOW);
- if (handle == NULL)
- {
- fprintf_filtered (gdb_stderr, "\n\ndlopen failed on '%s' - %s\n",
- LIBTHREAD_DB_SO, dlerror ());
- fprintf_filtered (gdb_stderr,
- "GDB will not be able to debug pthreads.\n\n");
- return 0;
- }
-
/* Initialize pointers to the dynamic library functions we will use.
Essential functions first. */
@@ -408,10 +408,45 @@ thread_db_load (void)
if (td_init_p == NULL)
return 0;
+ err = td_init_p ();
+ if (err != TD_OK)
+ {
+ warning (_("Cannot initialize libthread_db: %s"), thread_db_err_str (err));
+ return 0;
+ }
+
td_ta_new_p = verbose_dlsym (handle, "td_ta_new");
if (td_ta_new_p == NULL)
return 0;
+ /* Initialize the structure that identifies the child process. */
+ proc_handle.ptid = inferior_ptid;
+
+ /* Now attempt to open a connection to the thread library. */
+ err = td_ta_new_p (&proc_handle, &thread_agent);
+ if (err != TD_OK)
+ {
+ td_ta_new_p = NULL;
+ if (info_verbose)
+ printf_unfiltered (_("td_ta_new failed: %s\n"),
+ thread_db_err_str (err));
+ else
+ switch (err)
+ {
+ case TD_NOLIBTHREAD:
+#ifdef THREAD_DB_HAS_TD_VERSION
+ case TD_VERSION:
+#endif
+ /* The errors above are not unexpected and silently ignored:
+ they just mean we haven't found correct version of
+ libthread_db yet. */
+ break;
+ default:
+ warning (_("td_ta_new failed: %s"), thread_db_err_str (err));
+ }
+ return 0;
+ }
+
td_ta_map_id2thr_p = verbose_dlsym (handle, "td_ta_map_id2thr");
if (td_ta_map_id2thr_p == NULL)
return 0;
@@ -432,14 +467,6 @@ thread_db_load (void)
if (td_thr_get_info_p == NULL)
return 0;
- /* Initialize the library. */
- err = td_init_p ();
- if (err != TD_OK)
- {
- warning (_("Cannot initialize libthread_db: %s"), thread_db_err_str (err));
- return 0;
- }
-
/* These are not essential. */
td_ta_event_addr_p = dlsym (handle, "td_ta_event_addr");
td_ta_set_event_p = dlsym (handle, "td_ta_set_event");
@@ -447,9 +474,191 @@ thread_db_load (void)
td_thr_event_enable_p = dlsym (handle, "td_thr_event_enable");
td_thr_tls_get_addr_p = dlsym (handle, "td_thr_tls_get_addr");
+ printf_unfiltered (_("[Thread debugging using libthread_db enabled]\n"));
+
+ /* The thread library was detected. Activate the thread_db target. */
+ push_target (&thread_db_ops);
+ thread_db_handle = handle;
+
+ enable_thread_event_reporting ();
+ thread_db_find_new_threads_1 ();
return 1;
}
+/* Lookup a library in which given symbol resides.
+ Note: this is looking in GDB process, not in the inferior.
+ Returns library name, or NULL. */
+
+static const char *
+dladdr_to_soname (const void *addr)
+{
+ Dl_info info;
+
+ if (dladdr (addr, &info) != 0)
+ return info.dli_fname;
+ return NULL;
+}
+
+/* Attempt to use LIBRARY as libthread_db. LIBRARY could be absolute,
+ relative, or just LIBTHREAD_DB. */
+
+static int
+try_thread_db_load (const char *library)
+{
+ void *handle;
+
+ if (info_verbose)
+ printf_unfiltered (_("Trying host libthread_db library: %s.\n"),
+ library);
+ handle = dlopen (library, RTLD_NOW);
+ if (handle == NULL)
+ {
+ if (info_verbose)
+ printf_unfiltered (_("dlopen failed: %s.\n"), dlerror ());
+ return 0;
+ }
+
+ if (info_verbose && strchr(library, '/') == NULL)
+ {
+ void *td_init;
+
+ td_init = dlsym (handle, "td_init");
+ if (td_init != NULL)
+ {
+ const char *const libpath = dladdr_to_soname (td_init);
+
+ if (libpath != NULL)
+ printf_unfiltered (_("Host %s resolved to: %s.\n"),
+ library, libpath);
+ }
+ }
+
+ if (try_thread_db_load_1 (handle))
+ return 1;
+
+ /* This library "refused" to work on current inferior. */
+ dlclose (handle);
+ return 0;
+}
+
+
+/* Search libthread_db_search_path for libthread_db which "agrees"
+ to work on current inferior. */
+
+static int
+thread_db_load_search ()
+{
+ char path[PATH_MAX];
+ const char *search_path = libthread_db_search_path;
+ int rc = 0;
+
+ while (*search_path)
+ {
+ const char *end = strchr (search_path, ':');
+ if (end)
+ {
+ size_t len = end - search_path;
+ if (len + 1 + strlen (LIBTHREAD_DB_SO) + 1 > sizeof (path))
+ {
+ char *cp = xmalloc (len + 1);
+ memcpy (cp, search_path, len);
+ cp[len] = '\0';
+ warning (_("libthread_db_search_path component too long,"
+ " ignored: %s."), cp);
+ xfree (cp);
+ search_path += len + 1;
+ continue;
+ }
+ memcpy (path, search_path, len);
+ path[len] = '\0';
+ search_path += len + 1;
+ }
+ else
+ {
+ size_t len = strlen (search_path);
+
+ if (len + 1 + strlen (LIBTHREAD_DB_SO) + 1 > sizeof (path))
+ {
+ warning (_("libthread_db_search_path component too long,"
+ " ignored: %s."), search_path);
+ break;
+ }
+ memcpy (path, search_path, len + 1);
+ search_path += len;
+ }
+ strcat (path, "/");
+ strcat (path, LIBTHREAD_DB_SO);
+ if (try_thread_db_load (path))
+ {
+ rc = 1;
+ break;
+ }
+ }
+ if (rc == 0)
+ rc = try_thread_db_load (LIBTHREAD_DB_SO);
+ if (rc == 0)
+ warning (_("Unable to find libthread_db matching inferior's thread"
+ " library, thread debugging will not be available."));
+ return rc;
+}
+
+/* Attempt to load and initialize libthread_db.
+ Return 1 on success.
+ */
+
+static int
+thread_db_load (void)
+{
+ const char *soname;
+ struct minimal_symbol *msym;
+
+ if (thread_db_handle != NULL)
+ return 1;
+
+ /* Don't attempt to use thread_db on targets which can not run
+ (executables not running yet, core files) for now. */
+ if (!target_has_execution)
+ return 0;
+
+ /* Don't attempt to use thread_db for remote targets. */
+ if (!target_can_run (¤t_target))
+ return 0;
+
+ msym = lookup_minimal_symbol ("nptl_version", NULL, NULL);
+ if (!msym)
+ msym = lookup_minimal_symbol ("__linuxthreads_version", NULL, NULL);
+
+ /* Some really old libpthread versions do not have either of the above. */
+ if (!msym)
+ msym = lookup_minimal_symbol ("__pthread_threads_events", NULL, NULL);
+
+ if (!msym)
+ /* No threads yet */
+ return 0;
+
+ soname = solib_name_from_address (SYMBOL_VALUE_ADDRESS (msym));
+ if (soname)
+ {
+ /* Attempt to load libthread_db from the same directory. */
+ char path[PATH_MAX], *cp;
+ strcpy (path, soname);
+ cp = strrchr (path, '/');
+ if (cp == NULL)
+ {
+ /* Expected to get fully resolved pathname for libpthread,
+ but got something else. Search for matching libthread_db and
+ hope there is one that matches current libpthread. */
+ warning (_("Cannot obtain absolute path of thread library: %s."),
+ soname);
+ return thread_db_load_search ();
+ }
+ strcpy (cp + 1, LIBTHREAD_DB_SO);
+ if (try_thread_db_load (path))
+ return 1;
+ }
+ return thread_db_load_search ();
+}
+
static td_err_e
enable_thread_event (td_thragent_t *thread_agent, int event, CORE_ADDR *bp)
{
@@ -593,75 +802,34 @@ void
check_for_thread_db (void)
{
td_err_e err;
- static int already_loaded;
+ static void *last_loaded;
/* Do nothing if we couldn't load libthread_db.so.1. */
- if (td_ta_new_p == NULL)
+ if (!thread_db_load ())
return;
/* First time through, report that libthread_db was successfuly
loaded. Can't print this in in thread_db_load as, at that stage,
- the interpreter and it's console haven't started. */
+ the interpreter and it's console haven't started.
+ We track td_ta_new_p because the user may switch executables,
+ and as a result we may decide to use a different version of
+ libthread_db. */
- if (!already_loaded)
+ if (last_loaded != td_ta_new_p)
{
- Dl_info info;
- const char *library = NULL;
- if (dladdr ((*td_ta_new_p), &info) != 0)
- library = info.dli_fname;
-
- /* Try dlinfo? */
-
- if (library == NULL)
- /* Paranoid - don't let a NULL path slip through. */
- library = LIBTHREAD_DB_SO;
+ last_loaded = td_ta_new_p;
if (info_verbose)
- printf_unfiltered (_("Using host libthread_db library \"%s\".\n"),
- library);
- already_loaded = 1;
- }
-
- if (using_thread_db)
- /* Nothing to do. The thread library was already detected and the
- target vector was already activated. */
- return;
-
- /* Don't attempt to use thread_db on targets which can not run
- (executables not running yet, core files) for now. */
- if (!target_has_execution)
- return;
-
- /* Don't attempt to use thread_db for remote targets. */
- if (!target_can_run (¤t_target))
- return;
-
- /* Initialize the structure that identifies the child process. */
- proc_handle.ptid = inferior_ptid;
-
- /* Now attempt to open a connection to the thread library. */
- err = td_ta_new_p (&proc_handle, &thread_agent);
- switch (err)
- {
- case TD_NOLIBTHREAD:
- /* No thread library was detected. */
- break;
-
- case TD_OK:
- printf_unfiltered (_("[Thread debugging using libthread_db enabled]\n"));
-
- /* The thread library was detected. Activate the thread_db target. */
- push_target (&thread_db_ops);
- using_thread_db = 1;
+ {
+ const char *library;
- enable_thread_event_reporting ();
- thread_db_find_new_threads_1 ();
- break;
-
- default:
- warning (_("Cannot initialize thread debugging library: %s"),
- thread_db_err_str (err));
- break;
+ library = dladdr_to_soname (*td_ta_new_p);
+ if (library == NULL)
+ library = LIBTHREAD_DB_SO;
+
+ printf_unfiltered (_("Using host libthread_db library \"%s\".\n"),
+ library);
+ }
}
}
@@ -783,7 +951,9 @@ thread_db_detach (struct target_ops *ops
/* Detach thread_db target ops. */
unpush_target (&thread_db_ops);
- using_thread_db = 0;
+ if (thread_db_handle)
+ dlclose (thread_db_handle);
+ thread_db_handle = NULL;
target_beneath->to_detach (target_beneath, args, from_tty);
}
@@ -896,7 +1066,9 @@ thread_db_wait (struct target_ops *ops,
{
remove_thread_event_breakpoints ();
unpush_target (&thread_db_ops);
- using_thread_db = 0;
+ if (thread_db_handle)
+ dlclose (thread_db_handle);
+ thread_db_handle = NULL;
return ptid;
}
@@ -944,7 +1116,9 @@ thread_db_mourn_inferior (struct target_
/* Detach thread_db target ops. */
unpush_target (ops);
- using_thread_db = 0;
+ if (thread_db_handle)
+ dlclose (thread_db_handle);
+ thread_db_handle = NULL;
}
static int
@@ -1187,13 +1361,27 @@ extern initialize_file_ftype _initialize
void
_initialize_thread_db (void)
{
- /* Only initialize the module if we can load libthread_db. */
- if (thread_db_load ())
- {
- init_thread_db_ops ();
- add_target (&thread_db_ops);
+ init_thread_db_ops ();
+ add_target (&thread_db_ops);
- /* Add ourselves to objfile event chain. */
- observer_attach_new_objfile (thread_db_new_objfile);
- }
+ /* Defer loading of libthread_db.so until inferior is running.
+ This allows gdb to load correct libthread_db for a given
+ executable -- there could be mutiple versions of glibc,
+ compiled with LinuxThreads or NPTL, and until there is
+ a running inferior, we can't tell which libthread_db is
+ the correct one to load. */
+
+ libthread_db_search_path = xstrdup(LIBTHREAD_DB_SEARCH_PATH);
+
+ add_setshow_filename_cmd ("libthread-db-search-path", class_support,
+ &libthread_db_search_path, _("\
+Set search path for libthread_db."), _("\
+Show the current search path or libthread_db."), _("\
+This path is used to search for libthread_db to be loaded into \
+gdb itself."),
+ NULL,
+ NULL,
+ &setlist, &showlist);
+ /* Add ourselves to objfile event chain. */
+ observer_attach_new_objfile (thread_db_new_objfile);
}
More information about the Gdb-patches
mailing list