From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5724 invoked by alias); 8 Apr 2009 21:23:48 -0000 Received: (qmail 5714 invoked by uid 22791); 8 Apr 2009 21:23:47 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,SPF_PASS X-Spam-Check-By: sourceware.org Received: from e24smtp02.br.ibm.com (HELO e24smtp02.br.ibm.com) (32.104.18.86) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 08 Apr 2009 21:23:38 +0000 Received: from mailhub1.br.ibm.com (mailhub1.br.ibm.com [9.18.232.109]) by e24smtp02.br.ibm.com (8.13.1/8.13.1) with ESMTP id n38Leu62024744 for ; Wed, 8 Apr 2009 18:40:56 -0300 Received: from d24av01.br.ibm.com (d24av01.br.ibm.com [9.18.232.46]) by mailhub1.br.ibm.com (8.13.8/8.13.8/NCO v9.2) with ESMTP id n38LNqi01298746 for ; Wed, 8 Apr 2009 18:23:52 -0300 Received: from d24av01.br.ibm.com (loopback [127.0.0.1]) by d24av01.br.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id n38LNVjS017834 for ; Wed, 8 Apr 2009 18:23:32 -0300 Received: from [9.8.10.215] ([9.8.10.215]) by d24av01.br.ibm.com (8.12.11.20060308/8.12.11) with ESMTP id n38LNQej017224; Wed, 8 Apr 2009 18:23:27 -0300 Subject: Re: [patch][rfc] Allow GDB to search for the right libthread_db.so.1 From: Thiago Jung Bauermann To: Paul Pluzhnikov Cc: gdb-patches ml In-Reply-To: <20090406203920.CCD6F19C4EC@localhost> References: <20090406203920.CCD6F19C4EC@localhost> Content-Type: text/plain; charset=UTF-8 Date: Wed, 08 Apr 2009 21:23:00 -0000 Message-Id: <1239225742.8871.145.camel@localhost.localdomain> Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-04/txt/msg00158.txt.bz2 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 > > * 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