From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5165 invoked by alias); 9 Aug 2011 09:59:03 -0000 Received: (qmail 5156 invoked by uid 22791); 9 Aug 2011 09:59:02 -0000 X-SWARE-Spam-Status: No, hits=-6.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 09 Aug 2011 09:58:43 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p799whYM012099 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 9 Aug 2011 05:58:43 -0400 Received: from host1.jankratochvil.net (ovpn-116-17.ams2.redhat.com [10.36.116.17]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p799weAQ016723 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 9 Aug 2011 05:58:42 -0400 Received: from host1.jankratochvil.net (localhost [127.0.0.1]) by host1.jankratochvil.net (8.14.4/8.14.4) with ESMTP id p799wdkd003110; Tue, 9 Aug 2011 11:58:39 +0200 Received: (from jkratoch@localhost) by host1.jankratochvil.net (8.14.4/8.14.4/Submit) id p799wd9t003109; Tue, 9 Aug 2011 11:58:39 +0200 Date: Tue, 09 Aug 2011 09:59:00 -0000 From: Jan Kratochvil To: Sergio Durigan Junior Cc: gdb-patches@sourceware.org Subject: Re: [RFC] Make solib_add regex-free Message-ID: <20110809095838.GA32172@host1.jankratochvil.net> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) 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: 2011-08/txt/msg00173.txt.bz2 On Tue, 09 Aug 2011 02:53:04 +0200, Sergio Durigan Junior wrote: > While working on the lazy debuginfo reading patch, I started to dislike > the way `solib_add' handles the list of shared libraries to have their > symbols read. It basically takes a regex as a first argument, and > matches the current so_list against it. As I am aware of your lazy debuginfo reading patch I understand the reasons for this patch. But I do not consider this patch as a cleanup on its own, in fact it makes IMO the code a bit more complicated. I find it a valid part of the lazy debuginfo reading patch series but as so it should be checked in only with that series. (I also do not commit parts of series which are not an improvement on their own). > --- a/gdb/solib.c > +++ b/gdb/solib.c > @@ -888,91 +888,101 @@ libpthread_solib_p (struct so_list *so) > return libpthread_name_p (so->so_name); > } > > -/* GLOBAL FUNCTION > +/* LOCAL FUNCTION > > - solib_add -- read in symbol info for newly added shared libraries > + solib_add_1 -- read in symbol info for newly added shared libraries, > + without calling `update_solib_list'. > > SYNOPSIS > > - void solib_add (char *pattern, int from_tty, struct target_ops > - *TARGET, int readsyms) > + static void solib_add_1 (VEC(so_list_p) *so_list, > + int from_tty, int readsyms) > > DESCRIPTION > One can drop this comment heading, discussed below. > - Read in symbolic information for any shared objects whose names > - match PATTERN. (If we've already read a shared object's symbol > - info, leave it alone.) If PATTERN is zero, read them all. > - > - If READSYMS is 0, defer reading symbolic information until later > - but still do any needed low level processing. > - > - FROM_TTY and TARGET are as described for update_solib_list, above. */ > + Helper function for `solib_add' below. This function does all the > + hard job described in `solib_add' comments, but does not call > + `update_solib_list'. */ > > -void > -solib_add (char *pattern, int from_tty, > - struct target_ops *target, int readsyms) > +static void > +solib_add_1 (VEC(so_list_p) *so_list, int from_tty, int readsyms) > { > struct so_list *gdb; > + struct target_so_ops *ops = solib_ops (target_gdbarch); > > current_program_space->solib_add_generation++; > > - if (pattern) > - { > - char *re_err = re_comp (pattern); > - > - if (re_err) > - error (_("Invalid regexp: %s"), re_err); > - } > - > - update_solib_list (from_tty, target); > - > /* Walk the list of currently loaded shared libraries, and read > symbols for any that match the pattern --- or any whose symbols > aren't already loaded, if no pattern was given. */ > { > - int any_matches = 0; Removal of this variable/functionality is a regression, discussed below. > int loaded_any_symbols = 0; > const int flags = > SYMFILE_DEFER_BP_RESET | (from_tty ? SYMFILE_VERBOSE : 0); > > for (gdb = so_list_head; gdb; gdb = gdb->next) > - if (! pattern || re_exec (gdb->so_name)) > - { > - /* Normally, we would read the symbols from that library > - only if READSYMS is set. However, we're making a small > - exception for the pthread library, because we sometimes > - need the library symbols to be loaded in order to provide > - thread support (x86-linux for instance). */ > - const int add_this_solib = > - (readsyms || libpthread_solib_p (gdb)); > - > - any_matches = 1; > - if (add_this_solib) > - { > - if (gdb->symbols_loaded) > - { > - /* If no pattern was given, be quiet for shared > - libraries we have already loaded. */ > - if (pattern && (from_tty || info_verbose)) > - printf_unfiltered (_("Symbols already loaded for %s\n"), > - gdb->so_name); > - } > - else if (solib_read_symbols (gdb, flags)) > - loaded_any_symbols = 1; > - } > - } > + { > + int add_this_solib; > + > + if (so_list) > + { > + int iter, found = 0; > + struct so_list *cur_so; > + > + for (iter = 0; > + VEC_iterate (so_list_p, so_list, iter, cur_so); > + iter++) > + { > + if (ops->same) > + { > + if (ops->same (gdb, cur_so)) > + { > + found = 1; > + break; > + } > + } > + else > + { > + if (!filename_cmp (gdb->so_original_name, > + cur_so->so_original_name)) > + { > + found = 1; > + break; > + } > + } I do not see why to use any content comparison for SO_LIST. Neither of the lists can change between being generated and compared, one could just use if (gdb == cur_so) > + } > + > + if (!found) > + /* We are not adding this shared library's symbols. */ > + continue; > + } > + /* Normally, we would read the symbols from that library > + only if READSYMS is set. However, we're making a small > + exception for the pthread library, because we sometimes > + need the library symbols to be loaded in order to provide > + thread support (x86-linux for instance). */ > + add_this_solib = (readsyms || libpthread_solib_p (gdb)); > + > + if (add_this_solib) > + { > + if (gdb->symbols_loaded) > + { > + /* If no so_list was given, be quiet for shared > + libraries we have already loaded. */ > + if (so_list && (from_tty || info_verbose)) > + printf_unfiltered (_("Symbols already loaded for %s\n"), > + gdb->so_name); > + } > + else if (solib_read_symbols (gdb, flags)) > + loaded_any_symbols = 1; > + } > + } > > if (loaded_any_symbols) > breakpoint_re_set (); > > - if (from_tty && pattern && ! any_matches) > - printf_unfiltered > - ("No loaded shared libraries match the pattern `%s'.\n", pattern); This useful error message is no longer ever produced, maybe solib_add_1 could have some return value. > - > if (loaded_any_symbols) > { > - struct target_so_ops *ops = solib_ops (target_gdbarch); > - > /* Getting new symbols may change our opinion about what is > frameless. */ > reinit_frame_cache (); > @@ -982,6 +992,34 @@ solib_add (char *pattern, int from_tty, > } > } > > +/* GLOBAL FUNCTION > + > + solib_add -- read in symbol info for newly added shared libraries > + > + SYNOPSIS > + > + void solib_add (VEC(so_list_p) *so_list, int from_tty, > + struct target_ops *target, int readsyms) > + > + DESCRIPTION > + While it was in the GDB code the new code no longer uses so large+duplicate headers, this part could be better dropped in new code I think. > + Read in symbolic information for any shared objects listed in > + SO_LIST. ((If we've already read a shared object's symbol info, > + leave it alone.) If SO_LIST is NULL, read them all. > + > + If READSYMS is 0, defer reading symbolic information until later > + but still do any needed low level processing inside `solib_add_1'. > + > + FROM_TTY and TARGET are as described for update_solib_list, above. */ > + > +void > +solib_add (VEC(so_list_p) *so_list, int from_tty, There is currently no caller which would use the SO_LIST parameter and I guess there will never be one. Please drop it. > + struct target_ops *target, int readsyms) > +{ > + update_solib_list (from_tty, target); > + solib_add_1 (so_list, from_tty, readsyms); > +} > + > > /* > > @@ -1272,6 +1310,54 @@ in_solib_dynsym_resolve_code (CORE_ADDR pc) > > LOCAL FUNCTION > > + solib_match_regex_solist -- compile a regex and match shared libraries > + against it. > + > + SYNOPSIS > + > + static VEC(so_list_p) *solib_match_regex_solist (const char *pattern) > + > + DESCRIPTION > + Likewise, I find it hypercorrect. > + This function compiles a regex represented by PATTERN, and returns a list > + of shared libraries matching it. Returns NULL if PATTERN is NULL or if > + no match was found. > + > + This function calls `update_solib_list' in order to refresh shared the > + shlib list. If you are going to call `solib_add' after calling this > + function, call `solib_add_1' instead, because this version does not > + call `update_solib_list', thus saving time. > + > + */ > + > +static VEC(so_list_p) * > +solib_match_regex_solist (const char *pattern, > + struct target_ops *target, int from_tty) > +{ > + char *err; > + struct so_list *iter; > + VEC(so_list_p) *solist = NULL; > + > + if (!pattern) > + return NULL; > + > + err = re_comp (pattern); > + > + if (err) > + error (_("Invalid regexp: `%s'"), err); > + > + update_solib_list (from_tty, target); > + for (iter = so_list_head; iter; iter = iter->next) > + if (re_exec (iter->so_name)) > + VEC_safe_push (so_list_p, solist, iter); > + > + return solist; > +} > + > +/* > + > + LOCAL FUNCTION > + > sharedlibrary_command -- handle command to explicitly add library > > SYNOPSIS > @@ -1285,8 +1371,19 @@ in_solib_dynsym_resolve_code (CORE_ADDR pc) > static void > sharedlibrary_command (char *args, int from_tty) > { > + struct cleanup *c; > + VEC(so_list_p) *solist; > + > dont_repeat (); > - solib_add (args, from_tty, (struct target_ops *) 0, 1); > + > + solist = solib_match_regex_solist (args, (struct target_ops *) 0, > + from_tty); > + if (!solist) > + error (_("No shared library matched the pattern `%s'."), args); If you do with FSF GDB `nosharedlibrary' symbols for libraries are unloaded, then `sharedlibrary' loads symbols for all the libraries. Your patches GDB writes an error message. Also `args' can be NULL which prints: No shared library matched the pattern `(null)'. But NULL %s is not portable. > + > + c = make_cleanup (VEC_cleanup (so_list_p), &solist); > + solib_add_1 (solist, from_tty, 1); > + do_cleanups (c); > } > > /* LOCAL FUNCTION Thanks, Jan