Re: [patch][rfc] Allow GDB to search for the right libthread_db.so.1

[Thiago Jung Bauermann <bauerman at br dot ibm dot com>]

Hi Paul,

El lun, 06-04-2009 a las 13:39 -0700, Paul Pluzhnikov escribiÃ:
> We have perhaps uncommon setup here, where we have several installed
> versions of glibc, and need to debug executables which are compiled
> and linked against them (using -rpath).

Many thanks for posting this patch! This is an issue for glibc
developers too, and I already considered working on exactly this
problem. I'm glad you did it first. :-)

> If this looks reasonable, I'll work on documentation next.
> There is also a matching change to gdbserver, which I am postponing until
> a decision on this patch is made.

Looks reasonable to me in general, but I have some comments. I wasn't
familiar with linux-thread-db.c before reviewing this patch, so I'd
appreciate if someone with more familiarity with this code would verify
that I'm not talking nonsense. :-)

> 2009-04-06  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
> 	    * gdb_thread_db.h (LIBTHREAD_DB_SEARCH_PATH): New define.
> 	    (LIBTHREAD_DB_SO): Moved from linux-thread-db.c
> 
> 	    * linux-thread-db.c (try_thread_db_load_1): New function.
> 	    (try_thread_db_load, thread_db_load_search): Likewise.
> 	    (thread_db_load): Iterate over possibly multiple libthread_db's.

I think the ChangeLog should mention that thread_db_load is now called
from check_for_thread_db instead of _initialize_thread_db. Also, there
are changes in the patch that this ChangeLog doesn't mention.

>  /* Non-zero if we're using this module's target vector.  */
> -static int using_thread_db;
> +static void *using_thread_db;

The comment for this variable needs to be updated. It's not a binary
flag anymore, so it should mention what the void * value is. The
variable should have a different name too, to reflect its new meaning.
 
>  static int
> -thread_db_load (void)
> +try_thread_db_load_1(void *handle)
>  {

I know that this function was undocumented already, but would you mind
adding a brief comment describing what it does, and what its return
value means?

Since you are changing a bit its behaviour and purpose, it seems fair
that you get to (briefly) document it. :-)

> +  /* Now attempt to open a connection to the thread library.  */
> +  err = td_ta_new_p (&proc_handle, &thread_agent);
> +  if (err != TD_OK)
> +    {
> +      td_ta_new_p = NULL;
> +      if (info_verbose)
> +	printf_unfiltered (_("td_ta_new(): %s.\n"),
> +			   thread_db_err_str (err));
> +      return 0;
> +    }

If err != TD_OK && err != TD_NOLIBTHREAD, a warning should be emitted,
like in the current version of the code:

      warning (_("Cannot initialize thread debugging library: %s"),
	       thread_db_err_str (err));

In your version of the code, this message is gone.

> @@ -447,9 +450,141 @@ thread_db_load (void)
>    td_thr_event_enable_p = dlsym (handle, "td_thr_event_enable");
>    td_thr_tls_get_addr_p = dlsym (handle, "td_thr_tls_get_addr");
>  
> +  printf_unfiltered (_("[Thread debugging using libthread_db enabled]\n"));
> +
> +  init_thread_db_ops ();
> +  add_target (&thread_db_ops);

Why can't you keep these calls in _initialize_thread_db? Isn't it wrong
to call add_target whenever a libthread_db is loaded (I admit I don't
know much about the target infrastructure in GDB)?

> +  handle = dlopen (library, RTLD_NOW);
> +  if (handle == NULL)
> +    {
> +      if (info_verbose)
> +	printf_unfiltered (_("dlopen(): %s.\n"), dlerror ());

The parenthesis shouldn't be in the message. Also, suggest being a bit
more descriptive: "dlopen failed: %s.\n"

The other error and information messages which currently have
parenthesis in them should also be fixed.

> +static int
> +thread_db_load_search ()
> +{
> +  char path[PATH_MAX];

This function can overflow path. Serious problem, IMHO.

> +      Dl_info info;
> +      const char *library = NULL;
> +      if (dladdr ((*td_ta_new_p), &info) != 0)
> +	library = info.dli_fname;
> +      if (library == NULL)
> +	library = LIBTHREAD_DB_SO;
> +      printf_unfiltered (_("Warning: guessed host libthread_db "
> +			   "library \"%s\".\n"), library);

Not sure this should be a warning. It's possible that the guessed
libthread_db is correct. Or is it more likely that it's not?

Also, I think it's clearer to rephrase it as:

"Searched libthread_db library to use, selected %s"

That is, avoid the (IMHO confusing) term "guessed library".

> +    }
> +  else
> +      printf_unfiltered (_("Warning: unable to guess libthread_db, "
> +			   "thread debugging will not be available.\n"));

Again, printf_unfiltered vs warning. I'd appreciate other opinions on
this topic. Also, suggest rewording to avoid "guess":

"Unable to find libthread_db matching inferior's thread library, thread
debugging will not be available.\n".

> +static int
> +thread_db_load (void)
> +{

Please add a comment describing the function and its return value.
Please also do this to the other functions which you introduced.

> +  msym = lookup_minimal_symbol ("nptl_version", NULL, NULL);
> +  if (!msym)
> +    msym = lookup_minimal_symbol ("__linuxthreads_version", NULL, NULL);
> +
> +  if (!msym)
> +    /* No threads yet */
> +    return 0;

Clever way to detect if a thread library is present. I can't comment on
its correctness and reliability though. Perhaps others will have more
insight. My only comment is that an alternative would be to search
through the inferior's link map. Don't know if it would be better or
worse.

Also, I assume you tested your patch in both NPTL systems and
LinuxThread systems, right?

> +  soname = solib_name_from_address (SYMBOL_VALUE_ADDRESS (msym));
> +  if (soname)
> +    {
> +      /* Attempt to load libthread_db from the same directory. */
> +      char path[PATH_MAX], *cp;
> +      strcpy (path, soname);
> +      cp = strrchr (path, '/');
> +      if (cp == NULL)
> +	{
> +	  /* Expected to get fully resolved pathname, but got
> +	     something else. Hope for the best.  */
> +	  printf_unfiltered (_("warning: (Internal error: "
> +			       "solib_name_from_address() returned \"%s\".\n"),
> +			     soname);
> +	  return thread_db_load_search ();
> +	}

"Internal error" has a specific meaning in GDB already, and it's not
what you use it for here. I suggest rewording to:

warning (_("Cannot obtain absolute path of thread library: %s"));

Note that I changed from calling printf_unfiltered to warning. I'm not
sure, but I gather the latter is preferred, right? (/me looks nervously
at other GDB developers, seeking a nod.)

Also, please clarify what "hope for the best" means here.

> +      printf_unfiltered (_("Using host libthread_db library \"%s\".\n"),
> +                         library);
> +      last_loaded = td_ta_new_p;

This message is currently only printed when verbose is on, but you
changed it to be always printed. Please provide a rationale for the
change.

> @@ -896,7 +993,10 @@ thread_db_wait (struct target_ops *ops,
>      {
>        remove_thread_event_breakpoints ();
>        unpush_target (&thread_db_ops);
> +      if (using_thread_db)
> +	dlclose (using_thread_db);
>        using_thread_db = 0;
> +      no_shared_libraries (NULL, 0);

I don't know much about GDB's mourning process, so this is a genuine
question: is this the appropriate place to call no_shared_libraries?
Doesn't feel right to me.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center