Patchwork [v3,4/8] Lock the demangled hash table

login
register
mail settings
Submitter Tom Tromey
Date May 29, 2019, 9:29 p.m.
Message ID <20190529212916.23721-5-tom@tromey.com>
Download mbox | patch
Permalink /patch/32909/
State New
Headers show

Comments

Tom Tromey - May 29, 2019, 9:29 p.m.
This introduces a lock that is used when modifying the demangled hash
table.

gdb/ChangeLog
2019-05-29  Tom Tromey  <tom@tromey.com>

	* symtab.c (demangled_mutex): New global.
	(symbol_set_names): Use a lock_guard.
---
 gdb/ChangeLog |   5 ++
 gdb/symtab.c  | 134 ++++++++++++++++++++++++++++----------------------
 2 files changed, 81 insertions(+), 58 deletions(-)
Pedro Alves - May 30, 2019, 12:58 p.m.
On 5/29/19 10:29 PM, Tom Tromey wrote:
> This introduces a lock that is used when modifying the demangled hash
> table.
> 
> gdb/ChangeLog
> 2019-05-29  Tom Tromey  <tom@tromey.com>
> 
> 	* symtab.c (demangled_mutex): New global.
> 	(symbol_set_names): Use a lock_guard.
> ---
>  gdb/ChangeLog |   5 ++
>  gdb/symtab.c  | 134 ++++++++++++++++++++++++++++----------------------
>  2 files changed, 81 insertions(+), 58 deletions(-)
> 
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 130d5cd48ff..6ad024a8a29 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -69,6 +69,9 @@
>  #include "arch-utils.h"
>  #include <algorithm>
>  #include "common/pathstuff.h"
> +#if CXX_STD_THREAD
> +#include <mutex>
> +#endif
>  
>  /* Forward declarations for local functions.  */
>  
> @@ -709,6 +712,11 @@ symbol_set_language (struct general_symbol_info *gsymbol,
>  
>  /* Functions to initialize a symbol's mangled name.  */
>  
> +#if CXX_STD_THREAD
> +/* Mutex that is used when modifying the demangled hash table.  */
> +static std::mutex demangled_mutex;
> +#endif

Not just modifying but when accessing as well.  

There's only a single mutex, and the comment says _THE_ demangled
hash table, but AFAICS, each per_bfd has its own table, right?

A single lock for every table compared to a lock per table might be
a good approach, but I'd welcome extended comments to explain
that design choice.

Thanks,
Pedro Alves
Pedro Alves - May 30, 2019, 2:03 p.m.
On 5/30/19 1:58 PM, Pedro Alves wrote:
> A single lock for every table compared to a lock per table might be
> a good approach, but I'd welcome extended comments to explain
> that design choice.

Just to be sure something wasn't lost in translation here:
by "extended" above, I just meant that I'd like to see comments
extended in that direction, not that I'd like to see super mega long
comments.  :-)

Thanks,
Pedro Alves
Tom Tromey - May 30, 2019, 9:58 p.m.
>> This introduces a lock that is used when modifying the demangled hash
>> table.

Pedro> There's only a single mutex, and the comment says _THE_ demangled
Pedro> hash table, but AFAICS, each per_bfd has its own table, right?

Yes, that's correct.  I'll fix the comment.

Pedro> A single lock for every table compared to a lock per table might be
Pedro> a good approach, but I'd welcome extended comments to explain
Pedro> that design choice.

I didn't want to put a mutex into the per-bfd object, mostly because it
uses space for the entire lifetime of that, while the mutex is only used
while demangling minsym names; and that can (currently) only be done for
one objfile at a time.

Tom

Patch

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 130d5cd48ff..6ad024a8a29 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -69,6 +69,9 @@ 
 #include "arch-utils.h"
 #include <algorithm>
 #include "common/pathstuff.h"
+#if CXX_STD_THREAD
+#include <mutex>
+#endif
 
 /* Forward declarations for local functions.  */
 
@@ -709,6 +712,11 @@  symbol_set_language (struct general_symbol_info *gsymbol,
 
 /* Functions to initialize a symbol's mangled name.  */
 
+#if CXX_STD_THREAD
+/* Mutex that is used when modifying the demangled hash table.  */
+static std::mutex demangled_mutex;
+#endif
+
 /* Objects of this type are stored in the demangled name hash table.  */
 struct demangled_name_entry
 {
@@ -837,9 +845,6 @@  symbol_set_names (struct general_symbol_info *gsymbol,
       return;
     }
 
-  if (per_bfd->demangled_names_hash == NULL)
-    create_demangled_names_hash (per_bfd);
-
   if (linkage_name[len] != '\0')
     {
       char *alloc_name;
@@ -858,64 +863,77 @@  symbol_set_names (struct general_symbol_info *gsymbol,
     = symbol_find_demangled_name (gsymbol, linkage_name_copy);
   gdb::unique_xmalloc_ptr<char> demangled_name (demangled_name_ptr);
 
-  entry.mangled = linkage_name_copy;
-  slot = ((struct demangled_name_entry **)
-	  htab_find_slot (per_bfd->demangled_names_hash.get (),
-			  &entry, INSERT));
-
-  /* If this name is not in the hash table, add it.  */
-  if (*slot == NULL
-      /* A C version of the symbol may have already snuck into the table.
-	 This happens to, e.g., main.init (__go_init_main).  Cope.  */
-      || (gsymbol->language == language_go
-	  && (*slot)->demangled[0] == '\0'))
-    {
-      int demangled_len = demangled_name ? strlen (demangled_name.get ()) : 0;
-
-      /* Suppose we have demangled_name==NULL, copy_name==0, and
-	 linkage_name_copy==linkage_name.  In this case, we already have the
-	 mangled name saved, and we don't have a demangled name.  So,
-	 you might think we could save a little space by not recording
-	 this in the hash table at all.
+  struct demangled_name_entry *found_entry;
+
+  {
+#if CXX_STD_THREAD
+    std::lock_guard<std::mutex> guard (demangled_mutex);
+#endif
+
+    if (per_bfd->demangled_names_hash == NULL)
+      create_demangled_names_hash (per_bfd);
+
+    entry.mangled = linkage_name_copy;
+    slot = ((struct demangled_name_entry **)
+	    htab_find_slot (per_bfd->demangled_names_hash.get (),
+			    &entry, INSERT));
+
+    /* If this name is not in the hash table, add it.  */
+    if (*slot == NULL
+	/* A C version of the symbol may have already snuck into the table.
+	   This happens to, e.g., main.init (__go_init_main).  Cope.  */
+	|| (gsymbol->language == language_go
+	    && (*slot)->demangled[0] == '\0'))
+      {
+	int demangled_len = demangled_name ? strlen (demangled_name.get ()) : 0;
+
+	/* Suppose we have demangled_name==NULL, copy_name==0, and
+	   linkage_name_copy==linkage_name.  In this case, we already have the
+	   mangled name saved, and we don't have a demangled name.  So,
+	   you might think we could save a little space by not recording
+	   this in the hash table at all.
 	 
-	 It turns out that it is actually important to still save such
-	 an entry in the hash table, because storing this name gives
-	 us better bcache hit rates for partial symbols.  */
-      if (!copy_name && linkage_name_copy == linkage_name)
-	{
-	  *slot
-	    = ((struct demangled_name_entry *)
-	       obstack_alloc (&per_bfd->storage_obstack,
-			      offsetof (struct demangled_name_entry, demangled)
-			      + demangled_len + 1));
-	  (*slot)->mangled = linkage_name;
-	}
-      else
-	{
-	  char *mangled_ptr;
-
-	  /* If we must copy the mangled name, put it directly after
-	     the demangled name so we can have a single
-	     allocation.  */
-	  *slot
-	    = ((struct demangled_name_entry *)
-	       obstack_alloc (&per_bfd->storage_obstack,
-			      offsetof (struct demangled_name_entry, demangled)
-			      + len + demangled_len + 2));
-	  mangled_ptr = &((*slot)->demangled[demangled_len + 1]);
-	  strcpy (mangled_ptr, linkage_name_copy);
-	  (*slot)->mangled = mangled_ptr;
-	}
+	   It turns out that it is actually important to still save such
+	   an entry in the hash table, because storing this name gives
+	   us better bcache hit rates for partial symbols.  */
+	if (!copy_name && linkage_name_copy == linkage_name)
+	  {
+	    *slot
+	      = ((struct demangled_name_entry *)
+		 obstack_alloc (&per_bfd->storage_obstack,
+				offsetof (struct demangled_name_entry, demangled)
+				+ demangled_len + 1));
+	    (*slot)->mangled = linkage_name;
+	  }
+	else
+	  {
+	    char *mangled_ptr;
+
+	    /* If we must copy the mangled name, put it directly after
+	       the demangled name so we can have a single
+	       allocation.  */
+	    *slot
+	      = ((struct demangled_name_entry *)
+		 obstack_alloc (&per_bfd->storage_obstack,
+				offsetof (struct demangled_name_entry, demangled)
+				+ len + demangled_len + 2));
+	    mangled_ptr = &((*slot)->demangled[demangled_len + 1]);
+	    strcpy (mangled_ptr, linkage_name_copy);
+	    (*slot)->mangled = mangled_ptr;
+	  }
 
-      if (demangled_name != NULL)
-	strcpy ((*slot)->demangled, demangled_name.get());
-      else
-	(*slot)->demangled[0] = '\0';
-    }
+	if (demangled_name != NULL)
+	  strcpy ((*slot)->demangled, demangled_name.get());
+	else
+	  (*slot)->demangled[0] = '\0';
+      }
+
+    found_entry = *slot;
+  }
 
-  gsymbol->name = (*slot)->mangled;
-  if ((*slot)->demangled[0] != '\0')
-    symbol_set_demangled_name (gsymbol, (*slot)->demangled,
+  gsymbol->name = found_entry->mangled;
+  if (found_entry->demangled[0] != '\0')
+    symbol_set_demangled_name (gsymbol, found_entry->demangled,
 			       &per_bfd->storage_obstack);
   else
     symbol_set_demangled_name (gsymbol, NULL, &per_bfd->storage_obstack);