This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
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
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |