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

[Paul Pluzhnikov <ppluzhnikov at google dot com>]

On Wed, Apr 8, 2009 at 2:22 PM, Thiago Jung Bauermann
<bauerman@br.ibm.com> wrote:

Thank you for comments and interest in this patch :-)

Attached is a revised version, which I believe addresses all the issues
you have raised.

>> ?/* 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.

Done.

>> ?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. :-)

Done.

>> + ?/* 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));

That's not quite right: you could also see TD_VERSION here (if you try the
"wrong" libthread_db first). I've changed the code to emit a warning for
other cases.

>> @@ -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)?

I believe you are quite correct. Fixed.

>> + ?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"

Fixed.

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

Fixed.

>> +static int
>> +thread_db_load_search ()
>> +{
>> + ?char path[PATH_MAX];
>
> This function can overflow path. Serious problem, IMHO.

Fixed.

>> + ? ? ?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?

With our Google-specific setting of LIBTHREAD_DB_SEARCH_PATH, we expect
correct libthread_db to always be found on that path; hence the warning.

But with empty search path (as in this patch) it definitely should not be.
Fixed accordingly.

> 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".

Done.

>> + ? ?}
>> + ?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".

Done.

>> +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.

Done.

>> + ?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.

I added one more symbol: __pthread_threads_events; really old versions of
LinuxThreads libpthread lacked the __linuxthreads_version.

> 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.

Looking through link_map fails for statically-linked executables.

It is desireable to have a single GDB that "just works", regardless of
whether the executable was built statically or dynamically.

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

Yes: we have a mixture and I tested both.

>> + ?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"));

Fixed.

> 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.

Done.

>> + ? ? ?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.

It's an "internal support" issue: we want to know at a glance which
libthread_db got loaded.

I changed this back to "verbose on" only.

>> @@ -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.

This was a workaround for earlier bug, which (AFAICT) is fixed in current
source. I removed the call.

I've tried to add documentation changes for this patch, but can't seem to
find appropriate place for it :-(

I think "Debugging Programs with Multiple Threads" section is the most
appropriate place, but I am not sure.

Thanks,
-- 
Paul Pluzhnikov

Attachment: gdb-thread-db.20090409.txt
Description: Text document