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