[2/6] Use gdb::unordered_set in linespec_state

Message ID 20250108-linespec-state-cxx-v1-2-a721e95ee050@tromey.com
State New
Headers
Series More linespec cleanups and C++-ification |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 fail Patch failed to apply

Commit Message

Tom Tromey Jan. 8, 2025, 7:23 a.m. UTC
  This patch changes linespec_state to use gdb::unordered_set.  This
simplifies the code a little and removes some manual management.
---
 gdb/linespec.c | 82 +++++++++++++++++++++++-----------------------------------
 1 file changed, 33 insertions(+), 49 deletions(-)
  

Comments

Simon Marchi Jan. 9, 2025, 3:20 a.m. UTC | #1
> @@ -186,10 +183,17 @@ struct linespec_state
>  
>    ~linespec_state ()
>    {
> -    htab_delete (addr_set);
>      xfree (canonical_names);
>    }
>  
> +  /* Add ADDR to the address set.  Return true if this is a new
> +     entry.  */
> +  bool add_address (program_space *pspace, CORE_ADDR addr)

I would keep the old name (maybe_add_address), I think it was clearer
(but making it a method is good).

> +  {
> +    address_entry entry { pspace, addr };
> +    return addr_set.insert (entry).second;

To be more concise, this could use:

  return addr_set.emplace (pspace, addr).second;

> @@ -219,12 +223,15 @@ struct linespec_state
>    /* Canonical strings that mirror the std::vector<symtab_and_line> result.  */
>    struct linespec_canonical_name *canonical_names = nullptr;
>  
> -  /* This is a set of address_entry objects which is used to prevent
> -     duplicate symbols from being entered into the result.  */
> -  htab_t addr_set;
> -
>    /* Are we building a linespec?  */
>    int is_linespec = 0;
> +
> +private:
> +
> +  /* This is a set of address_entry objects which is used to prevent
> +     duplicate symbols from being entered into the result.  */
> +  gdb::unordered_set<address_entry, address_entry_hash,
> +		     address_entry_eq> addr_set;

I don't really like using std::pair usually because `first` and `second`
make unclear code, but since here we never actually read back the values
(we only care about the presence or not of a certain value in the set),
we could perhaps simplify things (avoid having user-defined hash and eq
functions) by using:

  using address_entry = std::pair<struct program_space, CORE_ADDR>;

Simon
  

Patch

diff --git a/gdb/linespec.c b/gdb/linespec.c
index b9c918b048eb0a4404104cddb9b161ac7c09a709..689a8791d8561098a435262a32cc6883bfd38abf 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -134,26 +134,25 @@  struct linespec_canonical_name
 
 /* A hash function for address_entry.  */
 
-static hashval_t
-hash_address_entry (const void *p)
+struct address_entry_hash
 {
-  const struct address_entry *aep = (const struct address_entry *) p;
-  hashval_t hash;
-
-  hash = iterative_hash_object (aep->pspace, 0);
-  return iterative_hash_object (aep->addr, hash);
-}
+  std::size_t operator() (const address_entry &aep) const noexcept
+  {
+    hashval_t hash = iterative_hash_object (aep.pspace, 0);
+    return iterative_hash_object (aep.addr, hash);
+  }
+};
 
 /* An equality function for address_entry.  */
 
-static int
-eq_address_entry (const void *a, const void *b)
+struct address_entry_eq
 {
-  const struct address_entry *aea = (const struct address_entry *) a;
-  const struct address_entry *aeb = (const struct address_entry *) b;
-
-  return aea->pspace == aeb->pspace && aea->addr == aeb->addr;
-}
+  bool operator() (const address_entry &aea, const address_entry &aeb)
+    const noexcept
+  {
+    return aea.pspace == aeb.pspace && aea.addr == aeb.addr;
+  }
+};
 
 /* An instance of this is used to keep all state while linespec
    operates.  This instance is passed around as a 'this' pointer to
@@ -173,9 +172,7 @@  struct linespec_state
       default_line (default_line),
       funfirstline ((flags & DECODE_LINE_FUNFIRSTLINE) ? 1 : 0),
       list_mode ((flags & DECODE_LINE_LIST_MODE) ? 1 : 0),
-      canonical (canonical),
-      addr_set (htab_create_alloc (10, hash_address_entry, eq_address_entry,
-				   xfree, xcalloc, xfree))
+      canonical (canonical)
   {
   }
 
@@ -186,10 +183,17 @@  struct linespec_state
 
   ~linespec_state ()
   {
-    htab_delete (addr_set);
     xfree (canonical_names);
   }
 
+  /* Add ADDR to the address set.  Return true if this is a new
+     entry.  */
+  bool add_address (program_space *pspace, CORE_ADDR addr)
+  {
+    address_entry entry { pspace, addr };
+    return addr_set.insert (entry).second;
+  }
+
   /* The language in use during linespec processing.  */
   const struct language_defn *language;
 
@@ -219,12 +223,15 @@  struct linespec_state
   /* Canonical strings that mirror the std::vector<symtab_and_line> result.  */
   struct linespec_canonical_name *canonical_names = nullptr;
 
-  /* This is a set of address_entry objects which is used to prevent
-     duplicate symbols from being entered into the result.  */
-  htab_t addr_set;
-
   /* Are we building a linespec?  */
   int is_linespec = 0;
+
+private:
+
+  /* This is a set of address_entry objects which is used to prevent
+     duplicate symbols from being entered into the result.  */
+  gdb::unordered_set<address_entry, address_entry_hash,
+		     address_entry_eq> addr_set;
 };
 
 /* This is a helper object that is used when collecting symbols into a
@@ -1136,29 +1143,6 @@  add_sal_to_sals (struct linespec_state *self,
     }
 }
 
-/* Check whether the address, represented by PSPACE and ADDR, is
-   already in the set.  If so, return 0.  Otherwise, add it and return
-   1.  */
-
-static int
-maybe_add_address (htab_t set, struct program_space *pspace, CORE_ADDR addr)
-{
-  struct address_entry e, *p;
-  void **slot;
-
-  e.pspace = pspace;
-  e.addr = addr;
-  slot = htab_find_slot (set, &e, INSERT);
-  if (*slot)
-    return 0;
-
-  p = XNEW (struct address_entry);
-  memcpy (p, &e, sizeof (struct address_entry));
-  *slot = p;
-
-  return 1;
-}
-
 /* A helper that walks over all matching symtabs in all objfiles and
    calls CALLBACK for each symbol matching NAME.  If SEARCH_PSPACE is
    not NULL, then the search is restricted to just that program
@@ -2240,7 +2224,7 @@  convert_linespec_to_sals (struct linespec_state *state, linespec *ls)
 	    = sym.symbol->symtab ()->compunit ()->objfile ()->pspace ();
 
 	  if (symbol_to_sal (&sal, state->funfirstline, sym.symbol)
-	      && maybe_add_address (state->addr_set, pspace, sal.pc))
+	      && state->add_address (pspace, sal.pc))
 	    add_sal_to_sals (state, &sals, &sal,
 			     sym.symbol->natural_name (), 0);
 	}
@@ -2305,7 +2289,7 @@  convert_linespec_to_sals (struct linespec_state *state, linespec *ls)
 		{
 		  symtab_and_line sal;
 		  if (symbol_to_sal (&sal, state->funfirstline, sym.symbol)
-		      && maybe_add_address (state->addr_set, pspace, sal.pc))
+		      && state->add_address (pspace, sal.pc))
 		    add_sal_to_sals (state, &sals, &sal,
 				     sym.symbol->natural_name (), 0);
 		}
@@ -4154,7 +4138,7 @@  minsym_found (struct linespec_state *self, struct objfile *objfile,
 
   sal.section = msymbol->obj_section (objfile);
 
-  if (maybe_add_address (self->addr_set, objfile->pspace (), sal.pc))
+  if (self->add_address (objfile->pspace (), sal.pc))
     add_sal_to_sals (self, result, &sal, msymbol->natural_name (), 0);
 }