Re: [RFC] Make solib_add regex-free

Sergio Durigan Junior
2011-08-09T18:10:19Z

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.