[RFA,01/10] Remove some cleanups from search_minsyms_for_name

Message ID 20180401163539.15314-2-tom@tromey.com
State New
Headers show
Series
  • Remove some cleanups from linespec.c
Related show

Commit Message

Tom Tromey April 1, 2018, 4:35 p.m.
This changes struct collect_minsyms to use a std::vector, which
enables the removal of a cleanup from search_minsyms_for_name.  This
also changes iterate_over_minimal_symbols to take a
gdb::function_view, which makes a function in linespec.c more
type-safe.

gdb/ChangeLog
2018-03-31  Tom Tromey  <tom@tromey.com>

	* minsyms.h (iterate_over_minimal_symbols): Update.
	* minsyms.c (iterate_over_minimal_symbols): Take a
	gdb::function_view.
	* linespec.c (struct collect_minsyms): Add constructor and
	initializers.
	<msyms>: Now a std::vector.
	(compare_msyms): Now a std::sort comparator.
	(add_minsym): Change type of second parameter.
	(search_minsyms_for_name): Update.
---
 gdb/ChangeLog  |  12 +++++++
 gdb/linespec.c | 112 ++++++++++++++++++++++++++++-----------------------------
 gdb/minsyms.c  |   9 ++---
 gdb/minsyms.h  |   8 ++---
 4 files changed, 72 insertions(+), 69 deletions(-)

-- 
2.13.6

Comments

Simon Marchi April 2, 2018, 1:39 a.m. | #1
On 2018-04-01 12:35 PM, Tom Tromey wrote:
> This changes struct collect_minsyms to use a std::vector, which

> enables the removal of a cleanup from search_minsyms_for_name.  This

> also changes iterate_over_minimal_symbols to take a

> gdb::function_view, which makes a function in linespec.c more

> type-safe.

> 

> gdb/ChangeLog

> 2018-03-31  Tom Tromey  <tom@tromey.com>

> 

> 	* minsyms.h (iterate_over_minimal_symbols): Update.

> 	* minsyms.c (iterate_over_minimal_symbols): Take a

> 	gdb::function_view.

> 	* linespec.c (struct collect_minsyms): Add constructor and

> 	initializers.

> 	<msyms>: Now a std::vector.

> 	(compare_msyms): Now a std::sort comparator.

> 	(add_minsym): Change type of second parameter.

> 	(search_minsyms_for_name): Update.

> ---

>  gdb/ChangeLog  |  12 +++++++

>  gdb/linespec.c | 112 ++++++++++++++++++++++++++++-----------------------------

>  gdb/minsyms.c  |   9 ++---

>  gdb/minsyms.h  |   8 ++---

>  4 files changed, 72 insertions(+), 69 deletions(-)

> 

> diff --git a/gdb/linespec.c b/gdb/linespec.c

> index 1236b3f475..bd09f57b05 100644

> --- a/gdb/linespec.c

> +++ b/gdb/linespec.c

> @@ -46,6 +46,7 @@

>  #include "location.h"

>  #include "common/function-view.h"

>  #include "common/def-vector.h"

> +#include <algorithm>

>  

>  /* An enumeration of the various things a user might attempt to

>     complete for a linespec location.  */

> @@ -4375,8 +4376,15 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,

>  

>  struct collect_minsyms

>  {

> +  collect_minsyms (int funfirstline_, int list_mode_, struct symtab *symtab_)

> +    : symtab (symtab_),

> +      funfirstline (funfirstline_),


Note, funfirstline is unused, so you might as well remove it.

> +      list_mode (list_mode_)

> +  {

> +  }

> +

>    /* The objfile we're examining.  */

> -  struct objfile *objfile;

> +  struct objfile *objfile = nullptr;

>  

>    /* Only search the given symtab, or NULL to search for all symbols.  */

>    struct symtab *symtab;

> @@ -4388,7 +4396,7 @@ struct collect_minsyms

>    int list_mode;

>  

>    /* The resulting symbols.  */

> -  VEC (bound_minimal_symbol_d) *msyms;

> +  std::vector<struct bound_minimal_symbol> msyms;

>  };


I would lean towards removing the collect_minsyms structure and just pass anything
that's required by parameter to add_minsym.  It's easier to follow the flow of
execution when parameters are passed explicitly than through a structure.

>  

>  /* A helper function to classify a minimal_symbol_type according to

> @@ -4415,47 +4423,43 @@ classify_mtype (enum minimal_symbol_type t)

>      }

>  }

>  

> -/* Callback for qsort that sorts symbols by priority.  */

> +/* Callback for std::sort that sorts symbols by priority.  */

>  

> -static int

> -compare_msyms (const void *a, const void *b)

> +static bool

> +compare_msyms (const bound_minimal_symbol &a, const bound_minimal_symbol &b)

>  {

> -  const bound_minimal_symbol_d *moa = (const bound_minimal_symbol_d *) a;

> -  const bound_minimal_symbol_d *mob = (const bound_minimal_symbol_d *) b;

> -  enum minimal_symbol_type ta = MSYMBOL_TYPE (moa->minsym);

> -  enum minimal_symbol_type tb = MSYMBOL_TYPE (mob->minsym);

> +  enum minimal_symbol_type ta = MSYMBOL_TYPE (a.minsym);

> +  enum minimal_symbol_type tb = MSYMBOL_TYPE (b.minsym);

>  

> -  return classify_mtype (ta) - classify_mtype (tb);

> +  return classify_mtype (ta) < classify_mtype (tb);

>  }

>  

>  /* Callback for iterate_over_minimal_symbols that adds the symbol to

>     the result.  */


This comment should probably updated.  It is not really a callback for
iterate_over_minimal_symbols anymore (at least not directly).

>  

>  static void

> -add_minsym (struct minimal_symbol *minsym, void *d)

> +add_minsym (struct minimal_symbol *minsym, struct collect_minsyms &info)


...

> diff --git a/gdb/minsyms.c b/gdb/minsyms.c

> index 72969b7778..08efa1dc7e 100644

> --- a/gdb/minsyms.c

> +++ b/gdb/minsyms.c

> @@ -471,11 +471,8 @@ linkage_name_str (const lookup_name_info &lookup_name)

>  void

>  iterate_over_minimal_symbols (struct objfile *objf,

>  			      const lookup_name_info &lookup_name,

> -			      void (*callback) (struct minimal_symbol *,

> -						void *),

> -			      void *user_data)

> +			      gdb::function_view<void (struct minimal_symbol *)> callback)


This line is too long, you should probably wrap the whole parameter list.

>  {

> -

>    /* The first pass is over the ordinary hash table.  */

>      {

>        const char *name = linkage_name_str (lookup_name);

> @@ -490,7 +487,7 @@ iterate_over_minimal_symbols (struct objfile *objf,

>  	   iter = iter->hash_next)

>  	{

>  	  if (mangled_cmp (MSYMBOL_LINKAGE_NAME (iter), name) == 0)

> -	    (*callback) (iter, user_data);

> +	    callback (iter);

>  	}

>      }

>  

> @@ -509,7 +506,7 @@ iterate_over_minimal_symbols (struct objfile *objf,

>  	   iter != NULL;

>  	   iter = iter->demangled_hash_next)

>  	if (name_match (MSYMBOL_SEARCH_NAME (iter), lookup_name, NULL))

> -	  (*callback) (iter, user_data);

> +	  callback (iter);

>      }

>  }

>  

> diff --git a/gdb/minsyms.h b/gdb/minsyms.h

> index 11a202025d..b05f717575 100644

> --- a/gdb/minsyms.h

> +++ b/gdb/minsyms.h

> @@ -268,11 +268,9 @@ struct bound_minimal_symbol lookup_minimal_symbol_by_pc (CORE_ADDR);

>     For each matching symbol, CALLBACK is called with the symbol and

>     USER_DATA as arguments.  */


This comment should be updated.

>  

> -void iterate_over_minimal_symbols (struct objfile *objf,

> -				   const lookup_name_info &name,

> -				   void (*callback) (struct minimal_symbol *,

> -						     void *),

> -				   void *user_data);

> +void iterate_over_minimal_symbols

> +    (struct objfile *objf, const lookup_name_info &name,

> +     gdb::function_view<void (struct minimal_symbol *)> callback);

>  

>  /* Compute the upper bound of MINSYM.  The upper bound is the last

>     address thought to be part of the symbol.  If the symbol has a

> 


Thanks,

Simon

Patch

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 1236b3f475..bd09f57b05 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -46,6 +46,7 @@ 
 #include "location.h"
 #include "common/function-view.h"
 #include "common/def-vector.h"
+#include <algorithm>
 
 /* An enumeration of the various things a user might attempt to
    complete for a linespec location.  */
@@ -4375,8 +4376,15 @@  minsym_found (struct linespec_state *self, struct objfile *objfile,
 
 struct collect_minsyms
 {
+  collect_minsyms (int funfirstline_, int list_mode_, struct symtab *symtab_)
+    : symtab (symtab_),
+      funfirstline (funfirstline_),
+      list_mode (list_mode_)
+  {
+  }
+
   /* The objfile we're examining.  */
-  struct objfile *objfile;
+  struct objfile *objfile = nullptr;
 
   /* Only search the given symtab, or NULL to search for all symbols.  */
   struct symtab *symtab;
@@ -4388,7 +4396,7 @@  struct collect_minsyms
   int list_mode;
 
   /* The resulting symbols.  */
-  VEC (bound_minimal_symbol_d) *msyms;
+  std::vector<struct bound_minimal_symbol> msyms;
 };
 
 /* A helper function to classify a minimal_symbol_type according to
@@ -4415,47 +4423,43 @@  classify_mtype (enum minimal_symbol_type t)
     }
 }
 
-/* Callback for qsort that sorts symbols by priority.  */
+/* Callback for std::sort that sorts symbols by priority.  */
 
-static int
-compare_msyms (const void *a, const void *b)
+static bool
+compare_msyms (const bound_minimal_symbol &a, const bound_minimal_symbol &b)
 {
-  const bound_minimal_symbol_d *moa = (const bound_minimal_symbol_d *) a;
-  const bound_minimal_symbol_d *mob = (const bound_minimal_symbol_d *) b;
-  enum minimal_symbol_type ta = MSYMBOL_TYPE (moa->minsym);
-  enum minimal_symbol_type tb = MSYMBOL_TYPE (mob->minsym);
+  enum minimal_symbol_type ta = MSYMBOL_TYPE (a.minsym);
+  enum minimal_symbol_type tb = MSYMBOL_TYPE (b.minsym);
 
-  return classify_mtype (ta) - classify_mtype (tb);
+  return classify_mtype (ta) < classify_mtype (tb);
 }
 
 /* Callback for iterate_over_minimal_symbols that adds the symbol to
    the result.  */
 
 static void
-add_minsym (struct minimal_symbol *minsym, void *d)
+add_minsym (struct minimal_symbol *minsym, struct collect_minsyms &info)
 {
-  struct collect_minsyms *info = (struct collect_minsyms *) d;
-
-  if (info->symtab != NULL)
+  if (info.symtab != NULL)
     {
       /* We're looking for a label for which we don't have debug
 	 info.  */
       CORE_ADDR func_addr;
-      if (msymbol_is_function (info->objfile, minsym, &func_addr))
+      if (msymbol_is_function (info.objfile, minsym, &func_addr))
 	{
 	  symtab_and_line sal = find_pc_sect_line (func_addr, NULL, 0);
 
-	  if (info->symtab != sal.symtab)
+	  if (info.symtab != sal.symtab)
 	    return;
 	}
     }
 
   /* Exclude data symbols when looking for breakpoint locations.  */
-  if (!info->list_mode && !msymbol_is_function (info->objfile, minsym))
+  if (!info.list_mode && !msymbol_is_function (info.objfile, minsym))
     return;
 
-  bound_minimal_symbol_d mo = {minsym, info->objfile};
-  VEC_safe_push (bound_minimal_symbol_d, info->msyms, &mo);
+  struct bound_minimal_symbol mo = {minsym, info.objfile};
+  info.msyms.push_back (mo);
 }
 
 /* Search for minimal symbols called NAME.  If SEARCH_PSPACE
@@ -4471,15 +4475,9 @@  search_minsyms_for_name (struct collect_info *info,
 			 struct program_space *search_pspace,
 			 struct symtab *symtab)
 {
-  struct collect_minsyms local;
-  struct cleanup *cleanup;
-
-  memset (&local, 0, sizeof (local));
-  local.funfirstline = info->state->funfirstline;
-  local.list_mode = info->state->list_mode;
-  local.symtab = symtab;
-
-  cleanup = make_cleanup (VEC_cleanup (bound_minimal_symbol_d), &local.msyms);
+  struct collect_minsyms local (info->state->funfirstline,
+				info->state->list_mode,
+				symtab);
 
   if (symtab == NULL)
     {
@@ -4499,7 +4497,11 @@  search_minsyms_for_name (struct collect_info *info,
 	ALL_OBJFILES (objfile)
 	{
 	  local.objfile = objfile;
-	  iterate_over_minimal_symbols (objfile, name, add_minsym, &local);
+	  iterate_over_minimal_symbols (objfile, name,
+					[&] (struct minimal_symbol *msym)
+					  {
+					    add_minsym (msym, local);
+					  });
 	}
       }
     }
@@ -4509,40 +4511,34 @@  search_minsyms_for_name (struct collect_info *info,
 	{
 	  set_current_program_space (SYMTAB_PSPACE (symtab));
 	  local.objfile = SYMTAB_OBJFILE(symtab);
-	  iterate_over_minimal_symbols (local.objfile, name, add_minsym, &local);
+	  iterate_over_minimal_symbols (local.objfile, name,
+					[&] (struct minimal_symbol *msym)
+					  {
+					    add_minsym (msym, local);
+					  });
 	}
     }
 
-    if (!VEC_empty (bound_minimal_symbol_d, local.msyms))
-      {
-	int classification;
-	int ix;
-	bound_minimal_symbol_d *item;
-
-	qsort (VEC_address (bound_minimal_symbol_d, local.msyms),
-	       VEC_length (bound_minimal_symbol_d, local.msyms),
-	       sizeof (bound_minimal_symbol_d),
-	       compare_msyms);
-
-	/* Now the minsyms are in classification order.  So, we walk
-	   over them and process just the minsyms with the same
-	   classification as the very first minsym in the list.  */
-	item = VEC_index (bound_minimal_symbol_d, local.msyms, 0);
-	classification = classify_mtype (MSYMBOL_TYPE (item->minsym));
-
-	for (ix = 0;
-	     VEC_iterate (bound_minimal_symbol_d, local.msyms, ix, item);
-	     ++ix)
-	  {
-	    if (classify_mtype (MSYMBOL_TYPE (item->minsym)) != classification)
-	      break;
+  if (!local.msyms.empty ())
+    {
+      int classification;
 
-	    VEC_safe_push (bound_minimal_symbol_d,
-			   info->result.minimal_symbols, item);
-	  }
-      }
+      std::sort (local.msyms.begin (), local.msyms.end (), compare_msyms);
 
-    do_cleanups (cleanup);
+      /* Now the minsyms are in classification order.  So, we walk
+	 over them and process just the minsyms with the same
+	 classification as the very first minsym in the list.  */
+      classification = classify_mtype (MSYMBOL_TYPE (local.msyms[0].minsym));
+
+      for (const struct bound_minimal_symbol &item : local.msyms)
+	{
+	  if (classify_mtype (MSYMBOL_TYPE (item.minsym)) != classification)
+	    break;
+
+	  VEC_safe_push (bound_minimal_symbol_d,
+			 info->result.minimal_symbols, &item);
+	}
+    }
 }
 
 /* A helper function to add all symbols matching NAME to INFO.  If
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 72969b7778..08efa1dc7e 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -471,11 +471,8 @@  linkage_name_str (const lookup_name_info &lookup_name)
 void
 iterate_over_minimal_symbols (struct objfile *objf,
 			      const lookup_name_info &lookup_name,
-			      void (*callback) (struct minimal_symbol *,
-						void *),
-			      void *user_data)
+			      gdb::function_view<void (struct minimal_symbol *)> callback)
 {
-
   /* The first pass is over the ordinary hash table.  */
     {
       const char *name = linkage_name_str (lookup_name);
@@ -490,7 +487,7 @@  iterate_over_minimal_symbols (struct objfile *objf,
 	   iter = iter->hash_next)
 	{
 	  if (mangled_cmp (MSYMBOL_LINKAGE_NAME (iter), name) == 0)
-	    (*callback) (iter, user_data);
+	    callback (iter);
 	}
     }
 
@@ -509,7 +506,7 @@  iterate_over_minimal_symbols (struct objfile *objf,
 	   iter != NULL;
 	   iter = iter->demangled_hash_next)
 	if (name_match (MSYMBOL_SEARCH_NAME (iter), lookup_name, NULL))
-	  (*callback) (iter, user_data);
+	  callback (iter);
     }
 }
 
diff --git a/gdb/minsyms.h b/gdb/minsyms.h
index 11a202025d..b05f717575 100644
--- a/gdb/minsyms.h
+++ b/gdb/minsyms.h
@@ -268,11 +268,9 @@  struct bound_minimal_symbol lookup_minimal_symbol_by_pc (CORE_ADDR);
    For each matching symbol, CALLBACK is called with the symbol and
    USER_DATA as arguments.  */
 
-void iterate_over_minimal_symbols (struct objfile *objf,
-				   const lookup_name_info &name,
-				   void (*callback) (struct minimal_symbol *,
-						     void *),
-				   void *user_data);
+void iterate_over_minimal_symbols
+    (struct objfile *objf, const lookup_name_info &name,
+     gdb::function_view<void (struct minimal_symbol *)> callback);
 
 /* Compute the upper bound of MINSYM.  The upper bound is the last
    address thought to be part of the symbol.  If the symbol has a