Re: [patch] Allow gdbserver to dynamically lookup libthread_db.so.1

[Pedro Alves <pedro at codesourcery dot com>]

On Saturday 03 October 2009 00:51:22, Paul Pluzhnikov wrote:
> On Wed, Sep 2, 2009 at 10:15 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> > On Wed, Sep 2, 2009 at 9:47 AM, Pedro Alves<pedro@codesourcery.com> wrote:
> >
> >> Since you're touching this, how about loading a thread_db per-process
> >> like gdb/linux-thread-db.c already does?
> >
> > Oh, I've had this patch locally for so long, I didn't realize gdbserver
> > is now multi-process as well. Will fix.
> 
> Here is the fix.

Thanks much.  This is close.

I take it you only care for extended-remote?  How is the user
supposed to tweak the new setting with plain remote?
"tar remote; monitor foo; c" ?  I don't think that would work
with "gdbserver --attach".

> Index: gdbserver/acinclude.m4
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/acinclude.m4,v
> retrieving revision 1.7
> diff -u -p -u -r1.7 acinclude.m4
> --- gdbserver/acinclude.m4??????5 Jun 2008 22:36:57 -0000???????1.7
> +++ gdbserver/acinclude.m4??????2 Oct 2009 23:49:31 -0000
> @@ -22,7 +22,7 @@ AC_DEFUN([SRV_CHECK_THREAD_DB],
> ? ? void ps_get_thread_area() {}
> ? ? void ps_getpid() {}],
> ? ?[td_ta_new();],
> - ?[srv_cv_thread_db="-lthread_db"],
> + ?[srv_cv_thread_db="-ldl"],
> ? ?[srv_cv_thread_db=no
> ?
> ? if test "$prefix" = "/usr" || test "$prefix" = "NONE"; then
> @@ -42,28 +42,9 @@ AC_DEFUN([SRV_CHECK_THREAD_DB],
> ? ? void ps_get_thread_area() {}
> ? ? void ps_getpid() {}],
> ? ?[td_ta_new();],
> - ?[srv_cv_thread_db="$thread_db"],
> + ?[srv_cv_thread_db="-ldl"],
> ? ?[srv_cv_thread_db=no])
> ? ?])
> ? LIBS="$old_LIBS"

This doesn't make sense.

> Index: gdbserver/linux-low.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/linux-low.h,v
> retrieving revision 1.32
> diff -u -p -u -r1.32 linux-low.h
> --- gdbserver/linux-low.h???????30 Jun 2009 16:35:25 -0000??????1.32
> +++ gdbserver/linux-low.h???????2 Oct 2009 23:49:31 -0000
> @@ -57,6 +57,32 @@ struct process_info_private
> ? ?/* Connection to the libthread_db library. ?*/
> ? ?td_thragent_t *thread_agent;
> ?
> + ?/* Handle of the libthread_db from dlopen. ?*/
> + ?void *handle;
> +
> + ?/* Addresses of libthread_db functions. ?*/
> + ?td_err_e (*td_ta_new_p) (struct ps_prochandle * ps, td_thragent_t **ta);
> + ?td_err_e (*td_ta_event_getmsg_p) (const td_thragent_t *ta,
> +??????????????????????????????? ? ?td_event_msg_t *msg);
> + ?td_err_e (*td_ta_set_event_p) (const td_thragent_t *ta,
> +??????????????????????????????? td_thr_events_t *event);
> + ?td_err_e (*td_ta_event_addr_p) (const td_thragent_t *ta,
> +??????????????????????????????? ?td_event_e event, td_notify_t *ptr);
> + ?td_err_e (*td_ta_map_lwp2thr_p) (const td_thragent_t *ta, lwpid_t lwpid,
> +??????????????????????????????? ? td_thrhandle_t *th);
> + ?td_err_e (*td_thr_get_info_p) (const td_thrhandle_t *th,
> +??????????????????????????????? td_thrinfo_t *infop);
> + ?td_err_e (*td_thr_event_enable_p) (const td_thrhandle_t *th, int event);
> + ?td_err_e (*td_ta_thr_iter_p) (const td_thragent_t *ta,
> +???????????????????????????????td_thr_iter_f *callback, void *cbdata_p,
> +???????????????????????????????td_thr_state_e state, int ti_pri,
> +???????????????????????????????sigset_t *ti_sigmask_p,
> +???????????????????????????????unsigned int ti_user_flags);
> + ?td_err_e (*td_thr_tls_get_addr_p) (const td_thrhandle_t *th,
> +??????????????????????????????? ? ? void *map_address,
> +??????????????????????????????? ? ? size_t offset, void **address);
> + ?const char ** (*td_symbol_list_p) (void);
> +

Although gdbserver doesn't have a target stack concept, let's try to keep the
layers a bit separate.  Could you please make this a new (private) structure
in thread-db.c, and then have a new pointer here, say
process_info_private->thread_db into such an object?


> ? ?/* Arch-specific additions. ?*/
> ? ?struct arch_process_info *arch_private;
> ?};
> Index: gdbserver/server.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
> retrieving revision 1.102
> diff -u -p -u -r1.102 server.c
> --- gdbserver/server.c??30 Jun 2009 16:35:25 -0000??????1.102
> +++ gdbserver/server.c??2 Oct 2009 23:49:31 -0000
> @@ -32,6 +32,8 @@
> ?#include <malloc.h>
> ?#endif
> ?
> +#include <ctype.h>
> +
> ?ptid_t cont_thread;
> ?ptid_t general_thread;
> ?ptid_t step_thread;
> @@ -51,6 +53,10 @@ static char **program_argv, **wrapper_ar
> ? ? was originally used to debug LinuxThreads support. ?*/
> ?int debug_threads;
> ?
> +/* If not NULL, a colon-separated list of paths to use while looking for
> + ? libthread_db. ?*/
> +char *libthread_db_search_path;

We're now leaking target specific code into gdbserver's
common bits.  This will definitely not make sense to have on a
Windows build of gdbserver.  Can I convince you to add a target hook
to handle monitor commands, and move this handling to thread-db.c?
That's the simplest.  Even better would be to have some way
to register monitor commands with a callback, similar to gdb
commands, but much simpler.  But I'd be happy with the target
method for now.


> Index: gdbserver/thread-db.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/thread-db.c,v
> retrieving revision 1.23
> diff -u -p -u -r1.23 thread-db.c
> --- gdbserver/thread-db.c???????3 Apr 2009 20:15:51 -0000???????1.23
> +++ gdbserver/thread-db.c???????2 Oct 2009 23:49:31 -0000

<snip>

> @@ -233,7 +222,7 @@ find_one_thread (ptid_t ptid)
> ? ?td_err_e err;
> ? ?struct thread_info *inferior;
> ? ?struct lwp_info *lwp;
> - ?struct process_info_private *proc;
> + ?struct process_info_private *proc = current_process()->private;

Missing space before '()'.  There are other instances of this.

> ? ?/* If the thread layer is not (yet) initialized, fail. ?*/
> - ?if (!get_thread_process (thread)->all_symbols_looked_up)
> + ?if (proc->all_symbols_looked_up == 0)
> + ? ?return TD_ERR;

Please keep using ! for boolean's.

> +
> + ?if (priv->td_thr_tls_get_addr_p == NULL)
> ? ? ?return TD_ERR;

Shouldn't this be -1 ?  As in ...

> -#else
> - ?return -1;
> -#endif

... this?  < 0 here means unsupported, see server.c.

> +}
> +
> +static int
> +try_thread_db_load_1 (void *handle)
> +{
> + ?td_err_e err;
> + ?struct process_info *proc = current_process ();
> + ?struct process_info_private *priv = proc->private;
> +
> + ?/* Initialize pointers to the dynamic library functions we will use.
> + ? ? Essential functions first. ?*/
> +
> +#define CHK(required, a)???????????????????????????????\
> + ?if ((a) == NULL) {???????????????????????????????????\
> + ? ?if (debug_threads) {???????????????????????????????\
> + ? ? ?fprintf (stderr, "dlsym: %s\n", dlerror ());?????\
> + ? ?}??????????????????????????????????????????????????\
> + ? ?if (required)??????????????????????????????????????\
> + ? ? ?return 0;????????????????????????????????????????????????\
> + ?}

Even though people shouldn't stare at this for too
long (it may cause rare forms of brain rash), please make this
follow the code standards.  '{' on new line.  I'd prefer to see
this wrapped on do {} while (0).  Dangling if's raise eyebrows.

> +
> + ?CHK (1, priv->td_ta_new_p = dlsym (handle, "td_ta_new"));
> +
> + ?/* Attempt to open a connection to the thread library. ?*/
> + ?err = priv->td_ta_new_p (&priv->proc_handle, &priv->thread_agent);
> + ?if (err != TD_OK)
> + ? ?{
> + ? ? ?priv->td_ta_new_p = NULL;
> + ? ? ?if (debug_threads)
> +???????fprintf (stderr, "td_ta_new(): %s\n", thread_db_err_str (err));
> + ? ? ?return 0;
> + ? ?}
> +
> + ?CHK (1, priv->td_ta_map_lwp2thr_p = dlsym (handle, "td_ta_map_lwp2thr"));
> + ?CHK (1, priv->td_thr_get_info_p = dlsym (handle, "td_thr_get_info"));
> + ?CHK (1, priv->td_ta_thr_iter_p = dlsym (handle, "td_ta_thr_iter"));
> + ?CHK (1, priv->td_symbol_list_p = dlsym (handle, "td_symbol_list"));
> +
> + ?/* This is required only when thread_db_use_events is on. ?*/
> + ?CHK (thread_db_use_events,
> + ? ? ? priv->td_thr_event_enable_p = dlsym (handle, "td_thr_event_enable"));
> +
> + ?/* These are not essential. ?*/
> + ?CHK (0, priv->td_ta_event_addr_p = dlsym (handle, "td_ta_event_addr"));
> + ?CHK (0, priv->td_ta_set_event_p = dlsym (handle, "td_ta_set_event"));
> + ?CHK (0, priv->td_ta_event_getmsg_p = dlsym (handle, "td_ta_event_getmsg"));
> + ?CHK (0, priv->td_thr_tls_get_addr_p = dlsym (handle, "td_thr_tls_get_addr"));
> +
> +#undef CHK

Looking at the equivalent code in gdb/linux-thread-db.c, I don't
know that it's less readable there anyway, and I don't think the case
of finding just a couple of the required symbols (and dumping that to
debug output) is something we expect to find.  Just MHO.

> +
> + ?return 1;
> +}
> +

<snip>

> +
> +static int
> +thread_db_load_search ()

                         ^ (void) please.

> +{
> + ?char path[PATH_MAX];
> + ?const char *search_path;
> + ?int rc = 0;
> +
> +
> + ?if (libthread_db_search_path == NULL)
> + ? ?libthread_db_search_path = xstrdup (LIBTHREAD_DB_SEARCH_PATH);
> +
> + ?search_path = libthread_db_search_path;
> + ?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);
> + ? ? ? ? ? ? ?free (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;

Something fishy with the alignment here?  Mind tab vs spaces.

> + ? ? ? ? ? ?}
> +??????? ?memcpy (path, search_path, len + 1);
> +??????? ?search_path += len;
> +???????}
> + ? ? ?strcat (path, "/");
> + ? ? ?strcat (path, LIBTHREAD_DB_SO);
> + ? ? ?if (debug_threads)
> + ? ? ? ?fprintf (stderr, "thread_db_load_search trying %s\n", path);
> + ? ? ?if (try_thread_db_load (path))
> +???????{
> +??????? ?rc = 1;
> +??????? ?break;
> +???????}
> + ? ?}
> + ?if (rc == 0)
> + ? ?rc = try_thread_db_load (LIBTHREAD_DB_SO);
> +
> + ?if (debug_threads)
> + ? ?fprintf (stderr, "thread_db_load_search returning %d\n", rc);
> + ?return rc;
> ?}
> ?

<snip>

-- 
Pedro Alves