[RFC] Make solib_add regex-free

[RFC] Make solib_add regex-free

[Sergio Durigan Junior]

Hi,

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.

Well, this is not elegant IMO, especially because I believe the only
place which uses this regex feature is the `sharedlibrary' command.

Now suppose you're not the `sharedlibrary' command, and you already have
a `struct so_list *' that you want to load symbols from (happened to
me).  What you would do?  Call `solib_add' with the so->name, right?
Well, I did that, and it worked, but Jan has pointed me to a problem
that might happen:  in order to call `solib_add' correctly, you would
have to treat the so->name as a regex, i.e., escape symbols and
whatnot.  This is a pain and, IMO, error-prone.

That is why I decided to make `solib_add' take a list of `struct so_list
*' (stored in a VEC).  I believe this is much more elegant and will
probably help clarify the code.

As for the `sharedlibrary' command, I decided to make a new function
which compiles a regex, matches it against the current so_list, and
return a list containing all the shlibs which will have their symbols
loaded.  In order to get an up-to-date list of shared libs, I had to
call `update_solib_list' inside this new function, which made me break
`solib_add' in two functions to avoid calling `update_solib_list'
twice.

I tested this patch on the compile farm, without regressions.

Is this OK to check-in?

2011-08-08  Sergio Durigan Junior  <sergiodj@redhat.com>

	* solib-irix.c (irix_solib_create_inferior_hook): Uncast `solib_add'
	first argument.
	* solib-osf.c (osf_solib_create_inferior_hook): Likewise.
	* solib-sunos.c (sunos_solib_create_inferior_hook): Likewise.
	* solib.c (solib_add_1): New function which does not call
	`update_solib_list'.
	(solib_add): Split.  Call `update_solib_list' and then call
	`solib_add_1' to do the hard work.  Receive new argument `so_list'.
	Delete `pattern' argument.
	(solib_match_regex_solist): New function.
	(sharedlibrary_command): Call `solib_match_regex_solist' to obtain
	a list of shared libraries to have their symbols loaded.
	* solib.h: Include "vec.h".
	(so_list_p): New type.
	(solib_add): Update declaration; receive `VEC(so_list_p) *' as first
	argument.


diff --git a/gdb/solib-irix.c b/gdb/solib-irix.c
index 7bb528a..982cf51 100644
--- a/gdb/solib-irix.c
+++ b/gdb/solib-irix.c
@@ -495,7 +495,7 @@ irix_solib_create_inferior_hook (int from_tty)
      and will put out an annoying warning.
      Delaying the resetting of stop_soon until after symbol loading
      suppresses the warning.  */
-  solib_add ((char *) 0, 0, (struct target_ops *) 0, auto_solib_add);
+  solib_add (NULL, 0, (struct target_ops *) 0, auto_solib_add);
   inf->control.stop_soon = NO_STOP_QUIETLY;
 }
 
diff --git a/gdb/solib-osf.c b/gdb/solib-osf.c
index 0905eb6..0258404 100644
--- a/gdb/solib-osf.c
+++ b/gdb/solib-osf.c
@@ -356,7 +356,7 @@ osf_solib_create_inferior_hook (int from_tty)
      and will put out an annoying warning.
      Delaying the resetting of stop_soon until after symbol loading
      suppresses the warning.  */
-  solib_add ((char *) 0, 0, (struct target_ops *) 0, auto_solib_add);
+  solib_add (NULL, 0, (struct target_ops *) 0, auto_solib_add);
   inf->control.stop_soon = NO_STOP_QUIETLY;
 }
 
diff --git a/gdb/solib-sunos.c b/gdb/solib-sunos.c
index 9936038..41c95c6 100644
--- a/gdb/solib-sunos.c
+++ b/gdb/solib-sunos.c
@@ -805,7 +805,7 @@ sunos_solib_create_inferior_hook (int from_tty)
       warning (_("shared library handler failed to disable breakpoint"));
     }
 
-  solib_add ((char *) 0, 0, (struct target_ops *) 0, auto_solib_add);
+  solib_add (NULL, 0, (struct target_ops *) 0, auto_solib_add);
 }
 
 static void
diff --git a/gdb/solib.c b/gdb/solib.c
index 3296ed4..5a1049d 100644
--- 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
 
-   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;
     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;
+		      }
+		  }
+	      }
+
+	    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);
-
     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
+
+   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,
+	   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
+
+   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);
+
+  c = make_cleanup (VEC_cleanup (so_list_p), &solist);
+  solib_add_1 (solist, from_tty, 1);
+  do_cleanups (c);
 }
 
 /* LOCAL FUNCTION
diff --git a/gdb/solib.h b/gdb/solib.h
index c473d85..f2055e7 100644
--- a/gdb/solib.h
+++ b/gdb/solib.h
@@ -21,12 +21,17 @@
 #ifndef SOLIB_H
 #define SOLIB_H
 
+#include "vec.h"
+
 /* Forward decl's for prototypes */
 struct so_list;
 struct target_ops;
 struct target_so_ops;
 struct program_space;
 
+typedef struct so_list *so_list_p;
+DEF_VEC_P(so_list_p);
+
 /* Called when we free all symtabs, to free the shared library information
    as well.  */
 
@@ -34,7 +39,7 @@ extern void clear_solib (void);
 
 /* Called to add symbols from a shared library to gdb's symbol table.  */
 
-extern void solib_add (char *, int, struct target_ops *, int);
+extern void solib_add (VEC(so_list_p) *, int, struct target_ops *, int);
 extern int solib_read_symbols (struct so_list *, int);
 
 /* Function to be called when the inferior starts up, to discover the

Re: [RFC] Make solib_add regex-free

[Jan Kratochvil]

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

Re: [RFC] Make solib_add regex-free

[Pedro Alves]

On Tuesday 09 August 2011 10:58:38, Jan Kratochvil wrote:
> > +/* 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.

Agreed.  This came up just recently in the tic6x patches review too.
I think we should rework all such comments in the current codebase
to follow the standard, to avoid proliferation.  There aren't that
many.  I will do this.

-- 
Pedro Alves

Re: [RFC] Make solib_add regex-free

[Sergio Durigan Junior]

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.

Re: [RFC] Make solib_add regex-free

[Jan Kratochvil]

On Tue, 09 Aug 2011 20:09:54 +0200, Sergio Durigan Junior wrote:
> Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> >>    /* 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?

Yes, it is related to the error message with `(null)' (although it is not
related to the `(null)' problem itself), I think one can skip this variable,
the problem was described enough with the error message.


> Ok, I was just studying how `update_solib_list' does, and decided to do
> the same.  I will update it then.

Yes but this is really a different case.


> >> -    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.

That may be true but still the final GDB as whole should print it IMO.


> 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...

I guess I suggest it in the last paragraph.


> >> @@ -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.

Just changing it to:

if (from_tty && args && !solist)
  error (_("No shared library matched the pattern `%s'."), args);

should do the right - no behavior change (I hope).


Thanks,
Jan