From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12940 invoked by alias); 9 Aug 2011 18:10:19 -0000 Received: (qmail 12927 invoked by uid 22791); 9 Aug 2011 18:10:18 -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 18:10:01 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p79IA1CS003255 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 9 Aug 2011 14:10:01 -0400 Received: from psique ([10.3.112.5]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p79I9tFX032185; Tue, 9 Aug 2011 14:09:58 -0400 From: Sergio Durigan Junior To: Jan Kratochvil Cc: gdb-patches@sourceware.org Subject: Re: [RFC] Make solib_add regex-free References: <20110809095838.GA32172@host1.jankratochvil.net> Date: Tue, 09 Aug 2011 18:10:00 -0000 In-Reply-To: <20110809095838.GA32172@host1.jankratochvil.net> (Jan Kratochvil's message of "Tue, 9 Aug 2011 11:58:38 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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/msg00206.txt.bz2 Jan Kratochvil writes: > 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. Ok, makes sense, thanks for the review. I will submit it as a part of the lazy debuginfo patch. My intention was more to receive comments regarding my intention of making `solib_add' regex-free, and if you think this is the right way to do it. >> --- 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. Ok. >> /* 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. I'm not sure I understood why, after reading the entire message. Is it because of the `(null)' string being printed in the error message? >> + { >> + 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) Ok, I was just studying how `update_solib_list' does, and decided to do the same. I will update it then. >> - 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. I decided to drop this error message because, the way it's written, it only makes sense when using regex. Also, I couldn't come up with a decent replacement for it. Now `solib_add' uses a so_list in order to decided what to load. So IMO the error message should contain which shared libraries are present in the so_list. I don't know if I'm overcomplicating things here... >> +/* 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. Ok, I really didn't know what to do here, so I decided to keep the pattern. >> +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. Actually the lazy debuginfo reading patch uses it, but I agree, with the current code nobody uses it. >> @@ -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. Ok, I'll fix that. Thanks.