Re: [RFC] Make solib_add regex-free

[Sergio Durigan Junior <sergiodj at redhat dot com>]

Jan Kratochvil <jan.kratochvil@redhat.com> 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.