From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10862 invoked by alias); 4 Oct 2009 20:34:21 -0000 Received: (qmail 10850 invoked by uid 22791); 4 Oct 2009 20:34:19 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 04 Oct 2009 20:34:12 +0000 Received: (qmail 6627 invoked from network); 4 Oct 2009 20:34:09 -0000 Received: from unknown (HELO orlando) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 4 Oct 2009 20:34:09 -0000 From: Pedro Alves To: Paul Pluzhnikov Subject: Re: [patch] Allow gdbserver to dynamically lookup libthread_db.so.1 Date: Sun, 04 Oct 2009 20:34:00 -0000 User-Agent: KMail/1.9.10 Cc: gdb-patches@sourceware.org, dje@google.com References: <20090902163344.833F476568@localhost> <8ac60eac0909021015u37d1a6e2u1ae88dd35d00d2b9@mail.gmail.com> <8ac60eac0910021651o38990564ue7a65a870c14d00b@mail.gmail.com> In-Reply-To: <8ac60eac0910021651o38990564ue7a65a870c14d00b@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200910042134.08577.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-10/txt/msg00103.txt.bz2 On Saturday 03 October 2009 00:51:22, Paul Pluzhnikov wrote: > On Wed, Sep 2, 2009 at 10:15 AM, Paul Pluzhnikov wrote: > > On Wed, Sep 2, 2009 at 9:47 AM, Pedro Alves 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 > #endif > > +#include > + > 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 > @@ -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; > +} > + > + > +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; > } > -- Pedro Alves