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

Pedro Alves
2009-10-04T20:34:21Z

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