This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] Allow gdbserver to dynamically lookup libthread_db.so.1
On Wednesday 07 October 2009 01:26:58, Paul Pluzhnikov wrote:
> doc/
> 2009-10-06 Paul Pluzhnikov <ppluzhnikov@google.com>
>
> * gdb.texinfo (Server): Document libthread-db-search-path.
>
> gdbserver/
> 2009-10-06 Paul Pluzhnikov <ppluzhnikov@google.com>
>
>
> * acinclude.m4: (SRV_CHECK_THREAD_DB, SRV_CHECK_TLS_GET_ADDR): Remove.
> * configure.ac: Adjust.
> * linux-low.h (struct process_info_private): Move members to struct
> thread_db.
> (thread_db_free): New prototype.
> * linux-low.c (linux_remove_process): Adjust.
> (linux_wait_for_event_1, linux_look_up_symbols): Likewise.
> * server.c (handle_query): Move code ...
> (handle_monitor_command): ... here. New function.
> * target.h (struct target_ops): New member.
> * thread-db.c (struct thread_db): New.
> (libthread_db_search_path): New variable.
> (thread_db_create_event, thread_db_enable_reporting)
> (find_one_thread, maybe_attach_thread, find_new_threads_callback)
> (thread_db_find_new_threads, (thread_db_get_tls_address): Adjust.
> (try_thread_db_load_1, dladdr_to_soname): New functions.
> (try_thread_db_load, thread_db_load_search): New functions.
> (thread_db_init): Search for libthread_db.
> (thread_db_free): New function.
> (thread_db_handle_monitor_command): Likewise.
'* configure: Regenerate.' or so is missing. Please always
add that to the changelog even if not posting the regenerated
files.
(Opinions diverge on whether or not to include regenerated files in
patch submissions. Personaly, I prefer them to be included,
as it makes it easier for someone else to try the patch, and
makes it easy to spot if the contributer is using the correct
versions of the autotools. When I include them in my patches, I
tend to put the regenerated files hunk as last hunks in
the patch, so that they don't make review any harder.)
> Index: gdbserver/linux-low.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
> retrieving revision 1.112
> diff -u -p -u -r1.112 linux-low.c
> --- gdbserver/linux-low.c 31 Jul 2009 15:25:22 -0000 1.112
> +++ gdbserver/linux-low.c 7 Oct 2009 00:09:37 -0000
> @@ -42,6 +42,7 @@
> #include <dirent.h>
> #include <sys/stat.h>
> #include <sys/vfs.h>
> +#include <dlfcn.h>
This shouldn't be needed anymore here.
>
> #ifndef SPUFS_MAGIC
> #define SPUFS_MAGIC 0x23c9b64e
> @@ -118,6 +119,11 @@ int stopping_threads;
> /* FIXME make into a target method? */
> int using_threads = 1;
>
> +#ifdef USE_THREAD_DB
> +/* From thread-db.c */
> +extern int thread_db_handle_monitor_command (char *);
> +#endif
This can go to linux-low.h unconditionally, next to...
>
> int thread_db_init (int use_events);
> +void thread_db_free (struct process_info *);
> int thread_db_get_tls_address (struct thread_info *thread, CORE_ADDR offset,
> CORE_ADDR load_module, CORE_ADDR *address);
... these.
> +
> /* This flag is true iff we've just created or attached to our first
> inferior but it has not stopped yet. As soon as it does, we need
> to call the low target's arch_setup callback. Doing this only on
> @@ -261,8 +267,11 @@ linux_add_process (int pid, int attached
> static void
> linux_remove_process (struct process_info *process)
> {
> - free (process->private->arch_private);
> - free (process->private);
> + struct process_info_private *priv = process->private;
> +
> + thread_db_free (process);
> + free (priv->arch_private);
> + free (priv);
> remove_process (process);
> }
That thread_db_free call should be wrapped in USE_THREAD_DB.
> +static char *libthread_db_search_path;
>
> static int find_one_thread (ptid_t);
> static int find_new_threads_callback (const td_thrhandle_t *th_p, void *data);
> @@ -130,7 +168,11 @@ thread_db_create_event (CORE_ADDR where)
> td_event_msg_t msg;
> td_err_e err;
> struct lwp_info *lwp;
> - struct process_info_private *proc = current_process()->private;
> + struct thread_db *thread_db = current_process ()->private->thread_db;
> +
> + if (thread_db->td_ta_event_getmsg_p == NULL)
> + /* We shouldn't have ever be here in the first place. */
> + return TD_ERR;
Is this necessary? Looks like dead code. It would be a bad
programming error to reach here with thread_db->td_ta_event_getmsg_p it
seems. If we had gdb_assert or internal_error it would be more
appropriate here. We have `fatal' though. Until someone adds
an internal_error to gdbserver, I think a `fatal' here would be
better. Or just nothing at all.
> +static int
> +try_thread_db_load_1 (void *handle)
> +{
> + td_err_e err;
> + struct thread_db tdb;
> + struct process_info *proc = current_process ();
> +
> + if (proc->private->thread_db != NULL)
> + {
> + fprintf (stderr, "Internal error: thread_db != NULL in %s:%d\n",
> + __FILE__, __LINE__);
> + return 0;
> + }
> +
Odd. Did you ever actually see this happen? Same comment
about internal errors applies here.
> +
> + proc->private->thread_db = malloc (sizeof (tdb));
xmalloc.
> +static int
> +thread_db_load_search (void)
> +{
> + char path[PATH_MAX];
> + const char *search_path;
> + int rc = 0;
> +
> +
One blank line too much.
> + 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;
> + }
> + 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;
> }
Please fix tabs vs spaces issues in this function. I
noticed similar issues in try_thread_db_load as well.
>
> int
> thread_db_init (int use_events)
> {
> - int err;
> struct process_info *proc = current_process ();
> - struct process_info_private *priv = proc->private;
>
> /* FIXME drow/2004-10-16: This is the "overall process ID", which
> GNU/Linux calls tgid, "thread group ID". When we support
> @@ -433,27 +637,64 @@ thread_db_init (int use_events)
>
> thread_db_use_events = use_events;
>
> - err = td_ta_new (&priv->proc_handle, &priv->thread_agent);
> - switch (err)
> + if (thread_db_load_search ())
> {
> - case TD_NOLIBTHREAD:
> - /* No thread library was detected. */
> - return 0;
> -
> - case TD_OK:
> - /* The thread library was detected. */
> -
> if (use_events && thread_db_enable_reporting () == 0)
> return 0;
I notice that this return 0 here meant that
linux-low.c:linux_look_up_symbols still retry thread_db_init,
and that after your patch it won't. I'm inclined to think this
was a buglet and that following thread_db_init retries
wouldn't work anyway. Do you agree, or is this something
that we still need to keep addressing?
> thread_db_find_new_threads ();
> thread_db_look_up_symbols ();
> proc->all_symbols_looked_up = 1;
> return 1;
> + }
>
> - default:
> - warning ("error initializing thread_db library: %s",
> - thread_db_err_str (err));
> + return 0;
> +}
> +
Other than that, this looks OK to me.
--
Pedro Alves