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