diff mbox

[6/8] Use std::unordered_map instead of htab_t.

Message ID 20180503184153.31183-7-keiths@redhat.com
State New, archived
Headers

Commit Message

Keith Seitz May 3, 2018, 6:41 p.m. UTC 
  This patch simply removes the use of libiberty's hashtab facility in favor
of C++'s unordered_map.

gdb/ChangeLog:

	* compile/compile-c-symbols.c (symbol_error, hash_symbol_error)
	(eq_symbol_error, del_symbol_error): Remove.
	(compile_instance::insert_symbol_error)
	(compile_instance::error_symbol_once): Update to use
	std::unordered_map instead of htab_t.
	* compile/compile-c-types.c (type_map_instance, hash_type_map_instance)
	(eq_type_map_instance, compile_instance::compile_instance): Remove.
	(compile_instance::insert_type, compile_instance::convert_type):
	Update to use std::unordered_map.
	* compile/compile-internal.h: Include string and unordered_map.
	(compile_instance::compile_instance): Don't call htab_delete.
	<type_map_t, symbol_err_map_t>: Make std::unordered_map.
---
 gdb/compile/compile-c-symbols.c | 90 +++++------------------------------------
 gdb/compile/compile-c-types.c   | 86 +++++++--------------------------------
 gdb/compile/compile-internal.h  | 20 ++++-----
 3 files changed, 34 insertions(+), 162 deletions(-)

Comments

Pedro Alves June 6, 2018, 2:35 p.m. UTC | #1 
On 05/03/2018 07:41 PM, Keith Seitz wrote:
> This patch simply removes the use of libiberty's hashtab facility in favor
> of C++'s unordered_map.

Do you know whether these hash tables end up with just a few entries, or
many entries?  I hope the open vs closed hashing, different hashing, and
increased memory usage aren't a real concern here.

> -
>  /* See compile-internal.h.  */
>  
>  void
>  compile_instance::insert_symbol_error (const struct symbol *sym,
> -				       const char *text)
> +				       std::string text)

Is this passing by value/copy on purpose?

>  {
> -  struct symbol_error e;
> -  void **slot;
> +  symbol_err_map_t::iterator pos = m_symbol_err_map.find (sym);
>  
> -  if (m_symbol_err_map == NULL)
> -    {
> -      m_symbol_err_map = htab_create_alloc (10,
> -					    hash_symbol_error,
> -					    eq_symbol_error,
> -					    del_symbol_error,
> -					    xcalloc,
> -					    xfree);
> -    }
> -
> -  e.sym = sym;
> -  slot = htab_find_slot (m_symbol_err_map, &e, INSERT);
> -  if (*slot == NULL)
> -    {
> -      struct symbol_error *e = XNEW (struct symbol_error);
> -
> -      e->sym = sym;
> -      e->message = xstrdup (text);
> -      *slot = e;
> -    }
> +  if (pos == m_symbol_err_map.end ())
> +    m_symbol_err_map.insert (std::make_pair (sym, text));

Move 'text' ?

>  }
>  
>  /* See compile-internal.h.  */
> @@ -115,20 +50,13 @@ compile_instance::insert_symbol_error (const struct symbol *sym,
>  void
>  compile_instance::error_symbol_once (const struct symbol *sym)
>  {
> -  struct symbol_error search;
> -  struct symbol_error *err;
> -
> -  if (m_symbol_err_map == NULL)
> -    return;
> -
> -  search.sym = sym;
> -  err = (struct symbol_error *) htab_find (m_symbol_err_map, &search);
> -  if (err == NULL || err->message == NULL)
> +  symbol_err_map_t::iterator pos = m_symbol_err_map.find (sym);
> +  if (pos == m_symbol_err_map.end () || pos->second.length () == 0)
>      return;
>  
> -  gdb::unique_xmalloc_ptr<char> message (err->message);
> -  err->message = NULL;
> -  error (_("%s"), message.get ());
> +  std::string message (pos->second);
> +  pos->second.clear ();

Move pos->second instead of copying?


> +  ::error (_("%s"), message.c_str ());
>  }
>  

Thanks,
Pedro Alves
Keith Seitz July 10, 2018, 5:05 p.m. UTC | #2 
On 06/06/2018 07:35 AM, Pedro Alves wrote:
> On 05/03/2018 07:41 PM, Keith Seitz wrote:
> Do you know whether these hash tables end up with just a few entries, or
> many entries?  I hope the open vs closed hashing, different hashing, and
> increased memory usage aren't a real concern here.

insert_symbol_error is an exceptional case. It is only ever used when there's been an exception "converting" a local variable. In fact, AFAICT, we have /no/ test suite coverage for this yet.

The type map is different, though. It is used as a cache (which is currently tossed every time a compile command runs). So I guess it is possible for the type conversion map to grow large. While I'm not particularly concerned with it at this time, if you'd like me to revert that to using an htab_up, just let me know. It's almost trivial.

> 
>> -
>>  /* See compile-internal.h.  */
>>  
>>  void
>>  compile_instance::insert_symbol_error (const struct symbol *sym,
>> -				       const char *text)
>> +				       std::string text)
> 
> Is this passing by value/copy on purpose?

Nope, it was an oversight/mistake. Since this method is only called from one place, I've changed this to accept a const char*.

>>  }
>>  
>>  /* See compile-internal.h.  */
>> @@ -115,20 +50,13 @@ compile_instance::insert_symbol_error (const struct symbol *sym,
>>  void
>>  compile_instance::error_symbol_once (const struct symbol *sym)
>>  {
>> -  struct symbol_error search;
>> -  struct symbol_error *err;
>> -
>> -  if (m_symbol_err_map == NULL)
>> -    return;
>> -
>> -  search.sym = sym;
>> -  err = (struct symbol_error *) htab_find (m_symbol_err_map, &search);
>> -  if (err == NULL || err->message == NULL)
>> +  symbol_err_map_t::iterator pos = m_symbol_err_map.find (sym);
>> +  if (pos == m_symbol_err_map.end () || pos->second.length () == 0)
>>      return;
>>  
>> -  gdb::unique_xmalloc_ptr<char> message (err->message);
>> -  err->message = NULL;
>> -  error (_("%s"), message.get ());
>> +  std::string message (pos->second);
>> +  pos->second.clear ();
> 
> Move pos->second instead of copying?

Yes, indeed. Fixed.

Thank you,
Keith
Pedro Alves July 11, 2018, 11:21 a.m. UTC | #3 
On 07/10/2018 06:05 PM, Keith Seitz wrote:
> On 06/06/2018 07:35 AM, Pedro Alves wrote:
>> On 05/03/2018 07:41 PM, Keith Seitz wrote:
>> Do you know whether these hash tables end up with just a few entries, or
>> many entries?  I hope the open vs closed hashing, different hashing, and
>> increased memory usage aren't a real concern here.
> 
> insert_symbol_error is an exceptional case. It is only ever used when there's been an exception "converting" a local variable. In fact, AFAICT, we have /no/ test suite coverage for this yet.
> 
> The type map is different, though. It is used as a cache (which is currently tossed every time a compile command runs). So I guess it is possible for the type conversion map to grow large. While I'm not particularly concerned with it at this time, if you'd like me to revert that to using an htab_up, just let me know. It's almost trivial.

Yeah, it's not whether or not I _want_ you to revert.  Instead, data rules.
I really have no awareness of whether that map ends up with a hundred entries
or a hundred million entries, and thus whether the difference ends up
significant.  So if after considering that you're not concerned, then
neither am I.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/compile/compile-c-symbols.c b/gdb/compile/compile-c-symbols.c
index 7bc30a5b3d..0b74e4b4cd 100644
--- a/gdb/compile/compile-c-symbols.c
+++ b/gdb/compile/compile-c-symbols.c
@@ -33,81 +33,16 @@ 
 
 
 
-/* Object of this type are stored in the compiler's symbol_err_map.  */
-
-struct symbol_error
-{
-  /* The symbol.  */
-
-  const struct symbol *sym;
-
-  /* The error message to emit.  This is malloc'd and owned by the
-     hash table.  */
-
-  char *message;
-};
-
-/* Hash function for struct symbol_error.  */
-
-static hashval_t
-hash_symbol_error (const void *a)
-{
-  const struct symbol_error *se = (const struct symbol_error *) a;
-
-  return htab_hash_pointer (se->sym);
-}
-
-/* Equality function for struct symbol_error.  */
-
-static int
-eq_symbol_error (const void *a, const void *b)
-{
-  const struct symbol_error *sea = (const struct symbol_error *) a;
-  const struct symbol_error *seb = (const struct symbol_error *) b;
-
-  return sea->sym == seb->sym;
-}
-
-/* Deletion function for struct symbol_error.  */
-
-static void
-del_symbol_error (void *a)
-{
-  struct symbol_error *se = (struct symbol_error *) a;
-
-  xfree (se->message);
-  xfree (se);
-}
-
 /* See compile-internal.h.  */
 
 void
 compile_instance::insert_symbol_error (const struct symbol *sym,
-				       const char *text)
+				       std::string text)
 {
-  struct symbol_error e;
-  void **slot;
+  symbol_err_map_t::iterator pos = m_symbol_err_map.find (sym);
 
-  if (m_symbol_err_map == NULL)
-    {
-      m_symbol_err_map = htab_create_alloc (10,
-					    hash_symbol_error,
-					    eq_symbol_error,
-					    del_symbol_error,
-					    xcalloc,
-					    xfree);
-    }
-
-  e.sym = sym;
-  slot = htab_find_slot (m_symbol_err_map, &e, INSERT);
-  if (*slot == NULL)
-    {
-      struct symbol_error *e = XNEW (struct symbol_error);
-
-      e->sym = sym;
-      e->message = xstrdup (text);
-      *slot = e;
-    }
+  if (pos == m_symbol_err_map.end ())
+    m_symbol_err_map.insert (std::make_pair (sym, text));
 }
 
 /* See compile-internal.h.  */
@@ -115,20 +50,13 @@  compile_instance::insert_symbol_error (const struct symbol *sym,
 void
 compile_instance::error_symbol_once (const struct symbol *sym)
 {
-  struct symbol_error search;
-  struct symbol_error *err;
-
-  if (m_symbol_err_map == NULL)
-    return;
-
-  search.sym = sym;
-  err = (struct symbol_error *) htab_find (m_symbol_err_map, &search);
-  if (err == NULL || err->message == NULL)
+  symbol_err_map_t::iterator pos = m_symbol_err_map.find (sym);
+  if (pos == m_symbol_err_map.end () || pos->second.length () == 0)
     return;
 
-  gdb::unique_xmalloc_ptr<char> message (err->message);
-  err->message = NULL;
-  error (_("%s"), message.get ());
+  std::string message (pos->second);
+  pos->second.clear ();
+  ::error (_("%s"), message.c_str ());
 }
 
 
diff --git a/gdb/compile/compile-c-types.c b/gdb/compile/compile-c-types.c
index 44b8317739..1ad6c2a4da 100644
--- a/gdb/compile/compile-c-types.c
+++ b/gdb/compile/compile-c-types.c
@@ -24,78 +24,24 @@ 
 #include "compile-c.h"
 #include "objfiles.h"
 
-/* An object that maps a gdb type to a gcc type.  */
-
-struct type_map_instance
-{
-  /* The gdb type.  */
-
-  struct type *type;
-
-  /* The corresponding gcc type handle.  */
-
-  gcc_type gcc_type_handle;
-};
-
-/* Hash a type_map_instance.  */
-
-static hashval_t
-hash_type_map_instance (const void *p)
-{
-  const struct type_map_instance *inst = (const struct type_map_instance *) p;
-
-  return htab_hash_pointer (inst->type);
-}
-
-/* Check two type_map_instance objects for equality.  */
-
-static int
-eq_type_map_instance (const void *a, const void *b)
-{
-  const struct type_map_instance *insta = (const struct type_map_instance *) a;
-  const struct type_map_instance *instb = (const struct type_map_instance *) b;
-
-  return insta->type == instb->type;
-}
-
-/* Constructor for compile_instance.  */
-
-compile_instance::compile_instance (struct gcc_base_context *gcc_fe,
-				    const char *options)
-  : m_gcc_fe (gcc_fe), m_gcc_target_options (options),
-    m_symbol_err_map (NULL)
-{
-  m_type_map = htab_create_alloc (10, hash_type_map_instance,
-				  eq_type_map_instance,
-				  xfree, xcalloc, xfree);
-}
-
-
-
 /* See compile-internal.h.  */
 
 void
 compile_instance::insert_type (struct type *type, gcc_type gcc_type)
 {
-  struct type_map_instance inst, *add;
-  void **slot;
-
-  inst.type = type;
-  inst.gcc_type_handle = gcc_type;
-  slot = htab_find_slot (m_type_map, &inst, INSERT);
+  type_map_t::iterator pos = m_type_map.find (type);
 
-  add = (struct type_map_instance *) *slot;
-  /* The type might have already been inserted in order to handle
-     recursive types.  */
-  if (add != NULL && add->gcc_type_handle != gcc_type)
-    error (_("Unexpected type id from GCC, check you use recent enough GCC."));
-
-  if (add == NULL)
+  if (pos != m_type_map.end ())
     {
-      add = XNEW (struct type_map_instance);
-      *add = inst;
-      *slot = add;
+      /* The type might have already been inserted in order to handle
+         recursive types.  */
+      if (pos->second != gcc_type)
+        error (_("Unexpected type id from GCC, check for recent "
+                 "enough GCC."));
     }
+  else
+    m_type_map.insert (std::make_pair (type, gcc_type));
+
 }
 
 /* Convert a pointer type to its gcc representation.  */
@@ -422,19 +368,15 @@  const char *compile_c_instance::m_default_cflags = "-std=gnu11"
 gcc_type
 compile_c_instance::convert_type (struct type *type)
 {
-  struct type_map_instance inst, *found;
-  gcc_type result;
-
   /* We don't ever have to deal with typedefs in this code, because
      those are only needed as symbols by the C compiler.  */
   type = check_typedef (type);
 
-  inst.type = type;
-  found = (struct type_map_instance *) htab_find (m_type_map, &inst);
-  if (found != NULL)
-    return found->gcc_type_handle;
+  type_map_t::iterator pos = m_type_map.find (type);
+  if (pos != m_type_map.end ())
+    return pos->second;
 
-  result = convert_type_basic (this, type);
+  gcc_type result = convert_type_basic (this, type);
   insert_type (type, result);
   return result;
 }
diff --git a/gdb/compile/compile-internal.h b/gdb/compile/compile-internal.h
index 8cfb214c02..e7b428da3f 100644
--- a/gdb/compile/compile-internal.h
+++ b/gdb/compile/compile-internal.h
@@ -19,6 +19,9 @@ 
 
 #include "gcc-c-interface.h"
 
+#include <string>
+#include <unordered_map>
+
 /* Debugging flag for the "compile" family of commands.  */
 
 extern int compile_debug;
@@ -31,15 +34,13 @@  struct block;
 class compile_instance
 {
 public:
-  compile_instance (struct gcc_base_context *gcc_fe, const char *options);
-
-  virtual ~compile_instance ()
+  compile_instance (struct gcc_base_context *gcc_fe, const char *options)
+      : m_gcc_fe (gcc_fe), m_gcc_target_options (options)
   {
-    htab_delete (m_type_map);
-    if (m_symbol_err_map != NULL)
-      htab_delete (m_symbol_err_map);
   }
 
+  virtual ~compile_instance () = default;
+
   /* Returns the GCC options to be passed during compilation.  */
   const std::string &gcc_target_options () const
   {
@@ -53,7 +54,7 @@  public:
   void insert_type (struct type *type, gcc_type gcc_type);
 
   /* Associate SYMBOL with some error text.  */
-  void insert_symbol_error (const struct symbol *sym, const char *text);
+  void insert_symbol_error (const struct symbol *sym, std::string text);
 
   /* Emit the error message corresponding to SYM, if one exists, and
      arrange for it not to be emitted again.  */
@@ -117,8 +118,9 @@  public:
 protected:
   /* Map types used by the compile instance for caching type conversions.
      and error tracking.  */
-  typedef htab_t type_map_t;
-  typedef htab_t symbol_err_map_t;
+  typedef std::unordered_map<const struct type *, gcc_type> type_map_t;
+  typedef std::unordered_map<const struct symbol *, std::string>
+    symbol_err_map_t;
 
   /* The GCC front end.  */
   struct gcc_base_context *m_gcc_fe;