Re: [RFC] Make solib_add regex-free

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

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