[PATCH 0/9] Minor minimal symbol improvements

[PATCH 0/9] Minor minimal symbol improvements

[Tom Tromey]

While working on an experiment, I found a number of small improvements
that could be made in the minsym code.  This series is the result.

Tested by the buildbot.  Let me know what you think.

Tom

[PATCH 8/9] Use memcpy in minimal_symbol_reader::install

[Tom Tromey]

minimal_symbol_reader::install copies minsyms from the msym_bunch
objects into the allocated memory.  It seemed better to me to do this
via memcpy, as that is frequently optimized in libc.

gdb/ChangeLog
2019-03-07  Tom Tromey  <tom@tromey.com>

	* minsyms.c (minimal_symbol_reader::install): Use memcpy.
---
 gdb/ChangeLog | 4 ++++
 gdb/minsyms.c | 6 +++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index f86105db7ee..4f3c929cd72 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -1330,7 +1330,6 @@ build_minimal_symbol_hash_tables (struct objfile *objfile)
 void
 minimal_symbol_reader::install ()
 {
-  int bindex;
   int mcount;
   struct msym_bunch *bunch;
   struct minimal_symbol *msymbols;
@@ -1377,8 +1376,9 @@ minimal_symbol_reader::install ()
 
       for (bunch = m_msym_bunch; bunch != NULL; bunch = bunch->next)
 	{
-	  for (bindex = 0; bindex < m_msym_bunch_index; bindex++, mcount++)
-	    msymbols[mcount] = bunch->contents[bindex];
+	  memcpy (&msymbols[mcount], &bunch->contents[0],
+		  m_msym_bunch_index * sizeof (struct minimal_symbol));
+	  mcount += m_msym_bunch_index;
 	  m_msym_bunch_index = BUNCH_SIZE;
 	}
 
-- 
2.17.2

[PATCH 6/9] Use htab_up for demangled hash

[Tom Tromey]

This changes objfile_per_bfd_storage::demangled_names_hash to be an
htab_up.  This lets us remove some manual management code from the
objfile_per_bfd_storage destructor.

gdb/ChangeLog
2019-03-07  Tom Tromey  <tom@tromey.com>

	* symtab.c (create_demangled_names_hash): Update.
	(symbol_set_names): Update.
	* objfiles.h (struct objfile_per_bfd_storage)
	<demangled_names_hash>: Now an htab_up.
	* objfiles.c (objfile_per_bfd_storage): Simplify.
---
 gdb/ChangeLog  | 8 ++++++++
 gdb/objfiles.c | 2 --
 gdb/objfiles.h | 2 +-
 gdb/symtab.c   | 6 +++---
 4 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index ff8b6fc72cf..7d36a2a7114 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -119,8 +119,6 @@ static const struct bfd_data *objfiles_bfd_data;
 
 objfile_per_bfd_storage::~objfile_per_bfd_storage ()
 {
-  if (demangled_names_hash)
-    htab_delete (demangled_names_hash);
 }
 
 /* Create the per-BFD storage object for OBJFILE.  If ABFD is not
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index 9db212a1d13..709525ae5af 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -261,7 +261,7 @@ struct objfile_per_bfd_storage
      name, and the second is the demangled name or just a zero byte
      if the name doesn't demangle.  */
 
-  htab *demangled_names_hash = NULL;
+  htab_up demangled_names_hash;
 
   /* The per-objfile information about the entry point, the scope (file/func)
      containing the entry point, and the scope of the user's main() func.  */
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 005ea23da8d..449bc4cd2b3 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -740,9 +740,9 @@ create_demangled_names_hash (struct objfile_per_bfd_storage *per_bfd)
      Choosing a much larger table size wastes memory, and saves only about
      1% in symbol reading.  */
 
-  per_bfd->demangled_names_hash = htab_create_alloc
+  per_bfd->demangled_names_hash.reset (htab_create_alloc
     (256, hash_demangled_name_entry, eq_demangled_name_entry,
-     NULL, xcalloc, xfree);
+     NULL, xcalloc, xfree));
 }
 
 /* Try to determine the demangled name for a symbol, based on the
@@ -848,7 +848,7 @@ symbol_set_names (struct general_symbol_info *gsymbol,
 
   entry.mangled = linkage_name_copy;
   slot = ((struct demangled_name_entry **)
-	  htab_find_slot (per_bfd->demangled_names_hash,
+	  htab_find_slot (per_bfd->demangled_names_hash.get (),
 			  &entry, INSERT));
 
   /* If this name is not in the hash table, add it.  */
-- 
2.17.2

[PATCH 4/9] Remove minsym termination

[Tom Tromey]

I was curious what used the terminating "null" minimal symbol; and
after looking I could not find anything.  This patch removes
terminate_minimal_symbol_table and the extra minimal symbol that is
allocated for it.

gdb/ChangeLog
2019-03-07  Tom Tromey  <tom@tromey.com>

	* symfile.c (reread_symbols): Update.
	* objfiles.c (objfile::objfile): Update.
	* minsyms.h (terminate_minimal_symbol_table): Don't declare.
	* minsyms.c (lookup_minimal_symbol_by_pc_section): Update
	comment.
	(minimal_symbol_reader::install): Update.
	(terminate_minimal_symbol_table): Remove.
	* jit.c (jit_object_close_impl): Update.
---
 gdb/ChangeLog  | 11 +++++++++++
 gdb/jit.c      |  2 --
 gdb/minsyms.c  | 40 +++-------------------------------------
 gdb/minsyms.h  |  7 -------
 gdb/objfiles.c |  2 --
 gdb/symfile.c  |  1 -
 6 files changed, 14 insertions(+), 49 deletions(-)

diff --git a/gdb/jit.c b/gdb/jit.c
index 81c4af40f92..1f87bf2538d 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -805,8 +805,6 @@ jit_object_close_impl (struct gdb_symbol_callbacks *cb,
 				OBJF_NOT_FILENAME);
   objfile->per_bfd->gdbarch = target_gdbarch ();
 
-  terminate_minimal_symbol_table (objfile);
-
   j = NULL;
   for (i = obj->symtabs; i; i = j)
     {
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 3c91853e780..d937e5d2a99 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -728,11 +728,8 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
     {
       CORE_ADDR pc = pc_in;
 
-      /* If this objfile has a minimal symbol table, go search it using
-         a binary search.  Note that a minimal symbol table always consists
-         of at least two symbols, a "real" symbol and the terminating
-         "null symbol".  If there are no real symbols, then there is no
-         minimal symbol table at all.  */
+      /* If this objfile has a minimal symbol table, go search it
+         using a binary search.  */
 
       if (objfile->per_bfd->minimal_symbol_count > 0)
 	{
@@ -1356,7 +1353,7 @@ minimal_symbol_reader::install ()
          compact out the duplicate entries.  Once we have a final table,
          we will give back the excess space.  */
 
-      alloc_count = m_msym_count + m_objfile->per_bfd->minimal_symbol_count + 1;
+      alloc_count = m_msym_count + m_objfile->per_bfd->minimal_symbol_count;
       obstack_blank (&m_objfile->per_bfd->storage_obstack,
 		     alloc_count * sizeof (struct minimal_symbol));
       msymbols = (struct minimal_symbol *)
@@ -1399,16 +1396,6 @@ minimal_symbol_reader::install ()
       msymbols = (struct minimal_symbol *)
 	obstack_finish (&m_objfile->per_bfd->storage_obstack);
 
-      /* We also terminate the minimal symbol table with a "null symbol",
-         which is *not* included in the size of the table.  This makes it
-         easier to find the end of the table when we are handed a pointer
-         to some symbol in the middle of it.  Zero out the fields in the
-         "null symbol" allocated at the end of the array.  Note that the
-         symbol count does *not* include this null symbol, which is why it
-         is indexed by mcount and not mcount-1.  */
-
-      memset (&msymbols[mcount], 0, sizeof (struct minimal_symbol));
-
       /* Attach the minimal symbol table to the specified objfile.
          The strings themselves are also located in the storage_obstack
          of this objfile.  */
@@ -1424,27 +1411,6 @@ minimal_symbol_reader::install ()
     }
 }
 
-/* See minsyms.h.  */
-
-void
-terminate_minimal_symbol_table (struct objfile *objfile)
-{
-  if (! objfile->per_bfd->msymbols)
-    objfile->per_bfd->msymbols = XOBNEW (&objfile->per_bfd->storage_obstack,
-					 minimal_symbol);
-
-  {
-    struct minimal_symbol *m
-      = &objfile->per_bfd->msymbols[objfile->per_bfd->minimal_symbol_count];
-
-    memset (m, 0, sizeof (*m));
-    /* Don't rely on these enumeration values being 0's.  */
-    MSYMBOL_TYPE (m) = mst_unknown;
-    MSYMBOL_SET_LANGUAGE (m, language_unknown,
-			  &objfile->per_bfd->storage_obstack);
-  }
-}
-
 /* Check if PC is in a shared library trampoline code stub.
    Return minimal symbol for the trampoline entry or NULL if PC is not
    in a trampoline code stub.  */
diff --git a/gdb/minsyms.h b/gdb/minsyms.h
index 532436c16c1..3e414f6ff7b 100644
--- a/gdb/minsyms.h
+++ b/gdb/minsyms.h
@@ -148,13 +148,6 @@ class minimal_symbol_reader
   int m_msym_count;
 };
 
-/* Create the terminating entry of OBJFILE's minimal symbol table.
-   If OBJFILE->msymbols is zero, allocate a single entry from
-   OBJFILE->objfile_obstack; otherwise, just initialize
-   OBJFILE->msymbols[OBJFILE->minimal_symbol_count].  */
-
-void terminate_minimal_symbol_table (struct objfile *objfile);
-
 \f
 
 /* Return whether MSYMBOL is a function/method.  If FUNC_ADDRESS_P is
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 4091b42dbf1..2468ca7a3f7 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -411,8 +411,6 @@ objfile::objfile (bfd *abfd, const char *name, objfile_flags flags_)
 
   per_bfd = get_objfile_bfd_data (this, abfd);
 
-  terminate_minimal_symbol_table (this);
-
   /* Add this file onto the tail of the linked list of other such files.  */
 
   if (object_files == NULL)
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 2214f16b431..68ec491d357 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2580,7 +2580,6 @@ reread_symbols (void)
 	  objfile_set_sym_fns (objfile, find_sym_fns (objfile->obfd));
 
 	  build_objfile_section_table (objfile);
-	  terminate_minimal_symbol_table (objfile);
 
 	  /* We use the same section offsets as from last time.  I'm not
 	     sure whether that is always correct for shared libraries.  */
-- 
2.17.2

[PATCH 3/9] Remove some unneeded initializations in minimal_symbol_reader

[Tom Tromey]

minimal_symbol_reader::record_full does not need to initialize any
minsym fields to 0, because that was already done implicitly via the
use of XCNEW when allocating the msym_bunch.

gdb/ChangeLog
2019-03-07  Tom Tromey  <tom@tromey.com>

	* minsyms.c (minimal_symbol_reader::record_full): Remove some
	initializations.
---
 gdb/ChangeLog |  5 +++++
 gdb/minsyms.c | 10 ----------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 6395cc4ccab..3c91853e780 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -1156,16 +1156,6 @@ minimal_symbol_reader::record_full (const char *name, int name_len,
   MSYMBOL_SECTION (msymbol) = section;
 
   MSYMBOL_TYPE (msymbol) = ms_type;
-  MSYMBOL_TARGET_FLAG_1 (msymbol) = 0;
-  MSYMBOL_TARGET_FLAG_2 (msymbol) = 0;
-  /* Do not use the SET_MSYMBOL_SIZE macro to initialize the size,
-     as it would also set the has_size flag.  */
-  msymbol->size = 0;
-
-  /* The hash pointers must be cleared! If they're not,
-     add_minsym_to_hash_table will NOT add this msymbol to the hash table.  */
-  msymbol->hash_next = NULL;
-  msymbol->demangled_hash_next = NULL;
 
   /* If we already read minimal symbols for this objfile, then don't
      ever allocate a new one.  */
-- 
2.17.2

[PATCH 2/9] Introduce and use language_set

[Tom Tromey]

I noticed that objfile_per_bfd_storage::demangled_hash_languages is a
std::vector, which seemed quite large for something that,
fundamentally, can be represented as a bit-set.  This patch
reimplements it as a bit-set.

gdb/ChangeLog
2019-03-07  Tom Tromey  <tom@tromey.com>

	* objfiles.h (struct objfile_per_bfd_storage)
	<demangled_hash_languages>: Now a language_set.
	* minsyms.c (add_minsym_to_demangled_hash_table): Update.
	* language.h (class language_set): New.
---
 gdb/ChangeLog  |  7 +++++
 gdb/language.h | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/minsyms.c  |  7 ++---
 gdb/objfiles.h |  6 ++--
 4 files changed, 95 insertions(+), 9 deletions(-)

diff --git a/gdb/language.h b/gdb/language.h
index d56ec200208..d57eee96a12 100644
--- a/gdb/language.h
+++ b/gdb/language.h
@@ -747,4 +747,88 @@ private:
   enum language m_lang;
 };
 
+/* A set object specialized for keeping track of languages.  */
+
+class language_set
+{
+public:
+
+  language_set ()
+  {
+  }
+
+  /* Insert LANG into this set.  */
+  void insert (enum language lang)
+  {
+    m_mask |= 1 << lang;
+  }
+
+  /* The underlying type used to represent the languages.  */
+  typedef unsigned mask_type;
+
+  /* An iterator that iterates over languages contained in a language
+     set.  */
+  class iterator
+  {
+  public:
+
+    iterator (unsigned int n, mask_type mask)
+      : m_index (n),
+	m_mask (mask)
+    {
+      gdb_assert (m_index <= nr_languages);
+      update ();
+    }
+
+    enum language operator* () const
+    {
+      return (enum language) m_index;
+    }
+
+    iterator &operator++ ()
+    {
+      ++m_index;
+      update ();
+      return *this;
+    }
+
+    bool operator!= (const iterator &other) const
+    {
+      return m_index != other.m_index;
+    }
+
+  private:
+
+    /* A helper function that will advance M_INDEX to the next
+       language in the set.  */
+    void update ()
+    {
+      while (m_index < nr_languages && (m_mask & (1 << m_index)) == 0)
+	++m_index;
+    }
+
+    /* The current index of the iterator.  */
+    unsigned int m_index;
+    /* The mask from the underlying set.  */
+    mask_type m_mask;
+  };
+
+  iterator begin () const
+  {
+    return iterator (0, m_mask);
+  }
+
+  iterator end () const
+  {
+    return iterator (nr_languages, m_mask);
+  }
+
+private:
+
+  /* A bit mask of languages which are included in this set.  */
+  mask_type m_mask = 0;
+
+  gdb_static_assert (nr_languages <= (sizeof (mask_type) * CHAR_BIT));
+};
+
 #endif /* defined (LANGUAGE_H) */
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 0513cfe69f4..6395cc4ccab 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -160,11 +160,8 @@ add_minsym_to_demangled_hash_table (struct minimal_symbol *sym,
       unsigned int hash = search_name_hash (MSYMBOL_LANGUAGE (sym),
 					    MSYMBOL_SEARCH_NAME (sym));
 
-      auto &vec = objfile->per_bfd->demangled_hash_languages;
-      auto it = std::lower_bound (vec.begin (), vec.end (),
-				  MSYMBOL_LANGUAGE (sym));
-      if (it == vec.end () || *it != MSYMBOL_LANGUAGE (sym))
-	vec.insert (it, MSYMBOL_LANGUAGE (sym));
+      objfile->per_bfd->demangled_hash_languages.insert
+	(MSYMBOL_LANGUAGE (sym));
 
       struct minimal_symbol **table
 	= objfile->per_bfd->msymbol_demangled_hash;
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index c5ce9eec955..843c44e1d24 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -313,10 +313,8 @@ struct objfile_per_bfd_storage
   minimal_symbol *msymbol_demangled_hash[MINIMAL_SYMBOL_HASH_SIZE] {};
 
   /* All the different languages of symbols found in the demangled
-     hash table.  A flat/vector-based map is more efficient than a map
-     or hash table here, since this will only usually contain zero or
-     one entries.  */
-  std::vector<enum language> demangled_hash_languages;
+     hash table.  */
+  language_set demangled_hash_languages;
 };
 
 /* Master structure for keeping track of each file from which
-- 
2.17.2

Re: [PATCH 2/9] Introduce and use language_set

[John Baldwin]

On 3/7/19 12:57 PM, Tom Tromey wrote:
> I noticed that objfile_per_bfd_storage::demangled_hash_languages is a
> std::vector, which seemed quite large for something that,
> fundamentally, can be represented as a bit-set.  This patch
> reimplements it as a bit-set.

Hmm, did you consider using std::bitset<nr_languages> for langauge_set?  You'd
still have to write your own iterator class, so it wouldn't save much in terms
of lines of code.  It just seems a bit odd to write a bitset class from scratch?

-- 
John Baldwin

Re: [PATCH 2/9] Introduce and use language_set

[Tom Tromey]

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> On 3/7/19 12:57 PM, Tom Tromey wrote:
>> I noticed that objfile_per_bfd_storage::demangled_hash_languages is a
>> std::vector, which seemed quite large for something that,
>> fundamentally, can be represented as a bit-set.  This patch
>> reimplements it as a bit-set.

John> Hmm, did you consider using std::bitset<nr_languages> for langauge_set?  You'd
John> still have to write your own iterator class, so it wouldn't save much in terms
John> of lines of code.  It just seems a bit odd to write a bitset class from scratch?

I didn't really consider it.

I looked now and on my machine, std::bitset<nr_languages> is 8 bytes,
whereas langauge_set is just 4.  So, with the custom implementation, I
could pack it into a hole in objfile_per_bfd_storage.

That said, due to the minsym arrays, objfile_per_bfd_storage is
enormous, so saving 4 bytes probably isn't important.  So, I'll redo
this.

Tom

Re: [PATCH 2/9] Introduce and use language_set

[Tom Tromey]

John> Hmm, did you consider using std::bitset<nr_languages> for langauge_set?  You'd
John> still have to write your own iterator class, so it wouldn't save much in terms
John> of lines of code.

In this case, because there are just two spots that iterate over this
set, and because we'd need to introduce somewhat ugly casts back to
"enum language" anyway, it seemed just as good to not write an iterator
and just do this in a simpler way.

Tom

commit d52ac096d7ae33144eb3a691a78e061d32833444
Author: Tom Tromey <tom@tromey.com>
Date:   Fri Mar 1 19:55:46 2019 -0700

    Use bitset for demangled_hash_languages
    
    I noticed that objfile_per_bfd_storage::demangled_hash_languages is a
    std::vector, which seemed quite large for something that,
    fundamentally, can be represented as a bitset.  This patch
    reimplements it as a std::bitset.
    
    gdb/ChangeLog
    2019-03-08  Tom Tromey  <tom@tromey.com>
    
            * objfiles.h (struct objfile_per_bfd_storage)
            <demangled_hash_languages>: Now a bitset.
            * minsyms.c (add_minsym_to_demangled_hash_table): Update.
            (lookup_minimal_symbol): Update.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7ec81d36571..4c952b0a445 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2019-03-08  Tom Tromey  <tom@tromey.com>
+
+	* objfiles.h (struct objfile_per_bfd_storage)
+	<demangled_hash_languages>: Now a bitset.
+	* minsyms.c (add_minsym_to_demangled_hash_table): Update.
+	(lookup_minimal_symbol): Update.
+
 2019-03-07  Tom Tromey  <tom@tromey.com>
 
 	* minsyms.h (class minimal_symbol_reader) <record_with_info>:
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 0513cfe69f4..cbb45f141c9 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -160,11 +160,7 @@ add_minsym_to_demangled_hash_table (struct minimal_symbol *sym,
       unsigned int hash = search_name_hash (MSYMBOL_LANGUAGE (sym),
 					    MSYMBOL_SEARCH_NAME (sym));
 
-      auto &vec = objfile->per_bfd->demangled_hash_languages;
-      auto it = std::lower_bound (vec.begin (), vec.end (),
-				  MSYMBOL_LANGUAGE (sym));
-      if (it == vec.end () || *it != MSYMBOL_LANGUAGE (sym))
-	vec.insert (it, MSYMBOL_LANGUAGE (sym));
+      objfile->per_bfd->demangled_hash_languages.set (MSYMBOL_LANGUAGE (sym));
 
       struct minimal_symbol **table
 	= objfile->per_bfd->msymbol_demangled_hash;
@@ -354,8 +350,12 @@ lookup_minimal_symbol (const char *name, const char *sfile,
 	    {
 	      /* Once for each language in the demangled hash names
 		 table (usually just zero or one languages).  */
-	      for (auto lang : objfile->per_bfd->demangled_hash_languages)
+	      for (unsigned iter = 0; iter < nr_languages; ++iter)
 		{
+		  if (!objfile->per_bfd->demangled_hash_languages.test (iter))
+		    continue;
+		  enum language lang = (enum language) iter;
+
 		  unsigned int hash
 		    = (lookup_name.search_name_hash (lang)
 		       % MINIMAL_SYMBOL_HASH_SIZE);
@@ -497,8 +497,12 @@ iterate_over_minimal_symbols
   /* The second pass is over the demangled table.  Once for each
      language in the demangled hash names table (usually just zero or
      one).  */
-  for (auto lang : objf->per_bfd->demangled_hash_languages)
+  for (unsigned liter = 0; liter < nr_languages; ++liter)
     {
+      if (!objf->per_bfd->demangled_hash_languages.test (liter))
+	continue;
+
+      enum language lang = (enum language) liter;
       const language_defn *lang_def = language_def (lang);
       symbol_name_matcher_ftype *name_match
 	= get_symbol_name_matcher (lang_def, lookup_name);
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index c5ce9eec955..47df0023dcc 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -28,6 +28,7 @@
 #include "registry.h"
 #include "gdb_bfd.h"
 #include "psymtab.h"
+#include <bitset>
 #include <vector>
 #include "common/next-iterator.h"
 #include "common/safe-iterator.h"
@@ -313,10 +314,8 @@ struct objfile_per_bfd_storage
   minimal_symbol *msymbol_demangled_hash[MINIMAL_SYMBOL_HASH_SIZE] {};
 
   /* All the different languages of symbols found in the demangled
-     hash table.  A flat/vector-based map is more efficient than a map
-     or hash table here, since this will only usually contain zero or
-     one entries.  */
-  std::vector<enum language> demangled_hash_languages;
+     hash table.  */
+  std::bitset<nr_languages> demangled_hash_languages;
 };
 
 /* Master structure for keeping track of each file from which

Re: [PATCH 2/9] Introduce and use language_set

[John Baldwin]

On 3/8/19 2:43 PM, Tom Tromey wrote:
> John> Hmm, did you consider using std::bitset<nr_languages> for langauge_set?  You'd
> John> still have to write your own iterator class, so it wouldn't save much in terms
> John> of lines of code.
> 
> In this case, because there are just two spots that iterate over this
> set, and because we'd need to introduce somewhat ugly casts back to
> "enum language" anyway, it seemed just as good to not write an iterator
> and just do this in a simpler way.

Looks good to me.

-- 
John Baldwin

[PATCH 7/9] Allocate minimal symbols with malloc

[Tom Tromey]

Currently, minimal symbols are allocated on the per-BFD obstack.
However, it is also possible for multiple symbol readers to create
minimal symbols for a given objfile.  In this case, the minimal
symbols will be reallocated on the obstack, leading to some waste of
storage.

This is a memory leak, but I think it won't be caught by tools like
valgrind, because valgrind doesn't know about obstacks.

This patch fixes the problem by using malloc to allocate the storage
for minimal symbols.

gdb/ChangeLog
2019-03-07  Tom Tromey  <tom@tromey.com>

	* objfiles.h (struct objfile_per_bfd_storage) <msymbols>: Now a
	unique_xmalloc_ptr.
	(objfile::msymbols_range::begin, objfile::msymbols_range::end):
	Update.
	* minsyms.c (lookup_minimal_symbol_by_pc_section)
	(build_minimal_symbol_hash_tables)
	(minimal_symbol_reader::install): Update.
---
 gdb/ChangeLog  | 10 ++++++++++
 gdb/minsyms.c  | 25 ++++++++++++-------------
 gdb/objfiles.h | 10 ++++------
 3 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index d937e5d2a99..f86105db7ee 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -735,7 +735,7 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio
 	{
 	  int best_zero_sized = -1;
 
-          msymbol = objfile->per_bfd->msymbols;
+          msymbol = objfile->per_bfd->msymbols.get ();
 	  lo = 0;
 	  hi = objfile->per_bfd->minimal_symbol_count - 1;
 
@@ -1291,7 +1291,7 @@ build_minimal_symbol_hash_tables (struct objfile *objfile)
 
   /* Now, (re)insert the actual entries.  */
   for ((i = objfile->per_bfd->minimal_symbol_count,
-	msym = objfile->per_bfd->msymbols);
+	msym = objfile->per_bfd->msymbols.get ());
        i > 0;
        i--, msym++)
     {
@@ -1356,14 +1356,16 @@ minimal_symbol_reader::install ()
       alloc_count = m_msym_count + m_objfile->per_bfd->minimal_symbol_count;
       obstack_blank (&m_objfile->per_bfd->storage_obstack,
 		     alloc_count * sizeof (struct minimal_symbol));
-      msymbols = (struct minimal_symbol *)
-	obstack_base (&m_objfile->per_bfd->storage_obstack);
+      gdb::unique_xmalloc_ptr<minimal_symbol>
+	msym_holder (XNEWVEC (minimal_symbol, alloc_count));
+      msymbols = msym_holder.get ();
 
       /* Copy in the existing minimal symbols, if there are any.  */
 
       if (m_objfile->per_bfd->minimal_symbol_count)
-	memcpy ((char *) msymbols, (char *) m_objfile->per_bfd->msymbols,
-	    m_objfile->per_bfd->minimal_symbol_count * sizeof (struct minimal_symbol));
+	memcpy (msymbols, m_objfile->per_bfd->msymbols.get (),
+		m_objfile->per_bfd->minimal_symbol_count
+		* sizeof (struct minimal_symbol));
 
       /* Walk through the list of minimal symbol bunches, adding each symbol
          to the new contiguous array of symbols.  Note that we start with the
@@ -1389,19 +1391,16 @@ minimal_symbol_reader::install ()
          no longer using.  */
 
       mcount = compact_minimal_symbols (msymbols, mcount, m_objfile);
-
-      ssize_t shrink_bytes
-	= (mcount + 1 - alloc_count) * sizeof (struct minimal_symbol);
-      obstack_blank_fast (&m_objfile->per_bfd->storage_obstack, shrink_bytes);
-      msymbols = (struct minimal_symbol *)
-	obstack_finish (&m_objfile->per_bfd->storage_obstack);
+      msym_holder.reset (XRESIZEVEC (struct minimal_symbol,
+				     msym_holder.release (),
+				     mcount));
 
       /* Attach the minimal symbol table to the specified objfile.
          The strings themselves are also located in the storage_obstack
          of this objfile.  */
 
       m_objfile->per_bfd->minimal_symbol_count = mcount;
-      m_objfile->per_bfd->msymbols = msymbols;
+      m_objfile->per_bfd->msymbols = std::move (msym_holder);
 
       /* Now build the hash tables; we can't do this incrementally
          at an earlier point since we weren't finished with the obstack
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index 709525ae5af..d03a8b608b3 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -281,11 +281,9 @@ struct objfile_per_bfd_storage
      name and a zero value for the address.  This makes it easy to walk
      through the array when passed a pointer to somewhere in the middle
      of it.  There is also a count of the number of symbols, which does
-     not include the terminating null symbol.  The array itself, as well
-     as all the data that it points to, should be allocated on the
-     objfile_obstack for this file.  */
+     not include the terminating null symbol.  */
 
-  minimal_symbol *msymbols = NULL;
+  gdb::unique_xmalloc_ptr<minimal_symbol> msymbols;
   int minimal_symbol_count = 0;
 
   /* The number of minimal symbols read, before any minimal symbol
@@ -374,13 +372,13 @@ struct objfile
 
     minimal_symbol_iterator begin () const
     {
-      return minimal_symbol_iterator (m_objfile->per_bfd->msymbols);
+      return minimal_symbol_iterator (m_objfile->per_bfd->msymbols.get ());
     }
 
     minimal_symbol_iterator end () const
     {
       return minimal_symbol_iterator
-	(m_objfile->per_bfd->msymbols
+	(m_objfile->per_bfd->msymbols.get ()
 	 + m_objfile->per_bfd->minimal_symbol_count);
     }
 
-- 
2.17.2

[PATCH 1/9] Slightly simplify minsym creation

[Tom Tromey]

Only one caller of minimal_symbol_reader::record_with_info was using
the return value, so this patch simplifies this code by having it
return void and changing that caller to use record_full instead.

gdb/ChangeLog
2019-03-07  Tom Tromey  <tom@tromey.com>

	* minsyms.h (class minimal_symbol_reader) <record_with_info>:
	Don't return the symbol.
	* coffread.c (record_minimal_symbol): Use record_full.
---
 gdb/ChangeLog  |  6 ++++++
 gdb/coffread.c |  3 ++-
 gdb/minsyms.h  | 13 +++++++------
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/gdb/coffread.c b/gdb/coffread.c
index b89c0e2e803..8c8e0784789 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -463,7 +463,8 @@ record_minimal_symbol (minimal_symbol_reader &reader,
       return NULL;
     }
 
-  return reader.record_with_info (cs->c_name, address, type, section);
+  return reader.record_full (cs->c_name, strlen (cs->c_name), true, address,
+			     type, section);
 }
 \f
 /* coff_symfile_init ()
diff --git a/gdb/minsyms.h b/gdb/minsyms.h
index 729d3946075..532436c16c1 100644
--- a/gdb/minsyms.h
+++ b/gdb/minsyms.h
@@ -116,14 +116,15 @@ class minimal_symbol_reader
 
   /* Like record_full, but:
      - uses strlen to compute NAME_LEN,
-     - passes COPY_NAME = true.  */
+     - passes COPY_NAME = true.
 
-  struct minimal_symbol *record_with_info (const char *name,
-					   CORE_ADDR address,
-					   enum minimal_symbol_type ms_type,
-					   int section)
+     This variant does not return the new symbol.  */
+
+  void record_with_info (const char *name, CORE_ADDR address,
+			 enum minimal_symbol_type ms_type,
+			 int section)
   {
-    return record_full (name, strlen (name), true, address, ms_type, section);
+    record_full (name, strlen (name), true, address, ms_type, section);
   }
 
  private:
-- 
2.17.2

[PATCH 9/9] Change minimal_symbol inheritance

[Tom Tromey]

This changes struct minimal_symbol to inherit from general_symbol_info
and updates various macros to cope.

Because MSYMBOL_SET_LANGUAGE and MSYMBOL_SET_NAMES were only used from
a single spot, this patch removes them in favor of simply inlining
their definitions.  I consider this to be somewhat cleaner, not least
because the "phony polymorphism" provided by such macros is not useful
in practice.

gdb/ChangeLog
2019-03-07  Tom Tromey  <tom@tromey.com>

	* symtab.h (struct minimal_symbol): Derive from
	general_symbol_info.
	(MSYMBOL_VALUE, MSYMBOL_VALUE_RAW_ADDRESS)
	(MSYMBOL_VALUE_ADDRESS, MSYMBOL_VALUE_BYTES)
	(MSYMBOL_BLOCK_VALUE, MSYMBOL_VALUE_CHAIN, MSYMBOL_LANGUAGE)
	(MSYMBOL_SECTION, MSYMBOL_OBJ_SECTION, MSYMBOL_NATURAL_NAME)
	(MSYMBOL_LINKAGE_NAME, MSYMBOL_DEMANGLED_NAME)
	(MSYMBOL_SEARCH_NAME): Update.
	(MSYMBOL_SET_LANGUAGE, MSYMBOL_SET_NAMES): Remove.
	* solib.c (gdb_bfd_lookup_symbol_from_symtab): Don't use memset.
	* minsyms.c (minimal_symbol_reader::record_full): Update.
---
 gdb/ChangeLog | 14 ++++++++++++++
 gdb/minsyms.c |  6 +++---
 gdb/solib.c   |  3 +--
 gdb/symtab.h  | 49 ++++++++++++++++++-------------------------------
 4 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 4f3c929cd72..669c1d13928 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -1145,9 +1145,9 @@ minimal_symbol_reader::record_full (const char *name, int name_len,
       m_msym_bunch = newobj;
     }
   msymbol = &m_msym_bunch->contents[m_msym_bunch_index];
-  MSYMBOL_SET_LANGUAGE (msymbol, language_auto,
-			&m_objfile->per_bfd->storage_obstack);
-  MSYMBOL_SET_NAMES (msymbol, name, name_len, copy_name, m_objfile);
+  symbol_set_language (msymbol, language_auto,
+		       &m_objfile->per_bfd->storage_obstack);
+  symbol_set_names (msymbol, name, name_len, copy_name, m_objfile->per_bfd);
 
   SET_MSYMBOL_VALUE_ADDRESS (msymbol, address);
   MSYMBOL_SECTION (msymbol) = section;
diff --git a/gdb/solib.c b/gdb/solib.c
index 791b94bc93a..017cd62eb77 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -1501,9 +1501,8 @@ gdb_bfd_lookup_symbol_from_symtab (bfd *abfd,
 	      if (bfd_get_flavour (abfd) == bfd_target_elf_flavour
 		  && gdbarch_elf_make_msymbol_special_p (gdbarch))
 		{
-		  struct minimal_symbol msym;
+		  struct minimal_symbol msym {};
 
-		  memset (&msym, 0, sizeof (msym));
 		  SET_MSYMBOL_VALUE_ADDRESS (&msym, symaddr);
 		  gdbarch_elf_make_msymbol_special (gdbarch, sym, &msym);
 		  symaddr = MSYMBOL_VALUE_RAW_ADDRESS (&msym);
diff --git a/gdb/symtab.h b/gdb/symtab.h
index d354c95800d..85dc3710483 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -641,16 +641,8 @@ gdb_static_assert (nr_minsym_types <= (1 << MINSYM_TYPE_BITS));
    between names and addresses, and vice versa.  They are also sometimes
    used to figure out what full symbol table entries need to be read in.  */
 
-struct minimal_symbol
+struct minimal_symbol : public general_symbol_info
 {
-
-  /* The general symbol info required for all types of symbols.
-
-     The SYMBOL_VALUE_ADDRESS contains the address that this symbol
-     corresponds to.  */
-
-  struct general_symbol_info mginfo;
-
   /* Size of this symbol.  dbx_end_psymtab in dbxread.c uses this
      information to calculate the end of the partial symtab based on the
      address of the last symbol plus the size of the last symbol.  */
@@ -687,7 +679,7 @@ struct minimal_symbol
 
   struct minimal_symbol *demangled_hash_next;
 
-/* True if this symbol is of some data type.  */
+  /* True if this symbol is of some data type.  */
 
   bool data_p () const;
 
@@ -708,43 +700,38 @@ struct minimal_symbol
 #define MSYMBOL_HAS_SIZE(msymbol)	((msymbol)->has_size + 0)
 #define MSYMBOL_TYPE(msymbol)		(msymbol)->type
 
-#define MSYMBOL_VALUE(symbol)		(symbol)->mginfo.value.ivalue
+#define MSYMBOL_VALUE(symbol)		(symbol)->value.ivalue
 /* The unrelocated address of the minimal symbol.  */
-#define MSYMBOL_VALUE_RAW_ADDRESS(symbol) ((symbol)->mginfo.value.address + 0)
+#define MSYMBOL_VALUE_RAW_ADDRESS(symbol) ((symbol)->value.address + 0)
 /* The relocated address of the minimal symbol, using the section
    offsets from OBJFILE.  */
 #define MSYMBOL_VALUE_ADDRESS(objfile, symbol)				\
-  ((symbol)->mginfo.value.address					\
-   + ANOFFSET ((objfile)->section_offsets, ((symbol)->mginfo.section)))
+  ((symbol)->value.address					\
+   + ANOFFSET ((objfile)->section_offsets, ((symbol)->section)))
 /* For a bound minsym, we can easily compute the address directly.  */
 #define BMSYMBOL_VALUE_ADDRESS(symbol) \
   MSYMBOL_VALUE_ADDRESS ((symbol).objfile, (symbol).minsym)
 #define SET_MSYMBOL_VALUE_ADDRESS(symbol, new_value)	\
-  ((symbol)->mginfo.value.address = (new_value))
-#define MSYMBOL_VALUE_BYTES(symbol)	(symbol)->mginfo.value.bytes
-#define MSYMBOL_BLOCK_VALUE(symbol)	(symbol)->mginfo.value.block
-#define MSYMBOL_VALUE_CHAIN(symbol)	(symbol)->mginfo.value.chain
-#define MSYMBOL_LANGUAGE(symbol)	(symbol)->mginfo.language
-#define MSYMBOL_SECTION(symbol)		(symbol)->mginfo.section
+  ((symbol)->value.address = (new_value))
+#define MSYMBOL_VALUE_BYTES(symbol)	(symbol)->value.bytes
+#define MSYMBOL_BLOCK_VALUE(symbol)	(symbol)->value.block
+#define MSYMBOL_VALUE_CHAIN(symbol)	(symbol)->value.chain
+#define MSYMBOL_LANGUAGE(symbol)	(symbol)->language
+#define MSYMBOL_SECTION(symbol)		(symbol)->section
 #define MSYMBOL_OBJ_SECTION(objfile, symbol)			\
-  (((symbol)->mginfo.section >= 0)				\
-   ? (&(((objfile)->sections)[(symbol)->mginfo.section]))	\
+  (((symbol)->section >= 0)				\
+   ? (&(((objfile)->sections)[(symbol)->section]))	\
    : NULL)
 
 #define MSYMBOL_NATURAL_NAME(symbol) \
-  (symbol_natural_name (&(symbol)->mginfo))
-#define MSYMBOL_LINKAGE_NAME(symbol)	(symbol)->mginfo.name
+  (symbol_natural_name (symbol))
+#define MSYMBOL_LINKAGE_NAME(symbol)	(symbol)->name
 #define MSYMBOL_PRINT_NAME(symbol)					\
   (demangle ? MSYMBOL_NATURAL_NAME (symbol) : MSYMBOL_LINKAGE_NAME (symbol))
 #define MSYMBOL_DEMANGLED_NAME(symbol) \
-  (symbol_demangled_name (&(symbol)->mginfo))
-#define MSYMBOL_SET_LANGUAGE(symbol,language,obstack)	\
-  (symbol_set_language (&(symbol)->mginfo, (language), (obstack)))
+  (symbol_demangled_name (symbol))
 #define MSYMBOL_SEARCH_NAME(symbol)					 \
-   (symbol_search_name (&(symbol)->mginfo))
-#define MSYMBOL_SET_NAMES(symbol,linkage_name,len,copy_name,objfile)	\
-  symbol_set_names (&(symbol)->mginfo, linkage_name, len, copy_name, \
-		    (objfile)->per_bfd)
+   (symbol_search_name (symbol))
 
 #include "minsyms.h"
 
-- 
2.17.2

[PATCH 5/9] Simplify per-BFD storage management

[Tom Tromey]

There's no reason that the objfile_per_bfd_storage must be allocated
via bfd_alloc.  This patch changes objfile_per_bfd_storage to be
managed more simply, via ordinary new and delete; and moves some code
into its (new) destructor.

While doing this I also noticed an extra initialization of
language_of_main, and removed it.

gdb/ChangeLog
2019-03-07  Tom Tromey  <tom@tromey.com>

	* objfiles.h (struct objfile_per_bfd_storage): Declare
	destructor.
	* objfiles.c (objfile_per_bfd_storage::~objfile_per_bfd_storage):
	New.
	(get_objfile_bfd_data): Use new.  Don't initialize
	language_of_main.
	(free_objfile_per_bfd_storage): Remove.
	(objfile_bfd_data_free, objfile::~objfile): Use delete.
---
 gdb/ChangeLog  | 11 +++++++++++
 gdb/objfiles.c | 46 ++++++++++++++--------------------------------
 gdb/objfiles.h |  2 ++
 3 files changed, 27 insertions(+), 32 deletions(-)

diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 2468ca7a3f7..ff8b6fc72cf 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -117,13 +117,17 @@ get_objfile_pspace_data (struct program_space *pspace)
 
 static const struct bfd_data *objfiles_bfd_data;
 
+objfile_per_bfd_storage::~objfile_per_bfd_storage ()
+{
+  if (demangled_names_hash)
+    htab_delete (demangled_names_hash);
+}
+
 /* Create the per-BFD storage object for OBJFILE.  If ABFD is not
    NULL, and it already has a per-BFD storage object, use that.
-   Otherwise, allocate a new per-BFD storage object.  If ABFD is not
-   NULL, the object is allocated on the BFD; otherwise it is allocated
-   on OBJFILE's obstack.  Note that it is not safe to call this
-   multiple times for a given OBJFILE -- it can only be called when
-   allocating or re-initializing OBJFILE.  */
+   Otherwise, allocate a new per-BFD storage object.  Note that it is
+   not safe to call this multiple times for a given OBJFILE -- it can
+   only be called when allocating or re-initializing OBJFILE.  */
 
 static struct objfile_per_bfd_storage *
 get_objfile_bfd_data (struct objfile *objfile, struct bfd *abfd)
@@ -136,50 +140,28 @@ get_objfile_bfd_data (struct objfile *objfile, struct bfd *abfd)
 
   if (storage == NULL)
     {
+      storage = new objfile_per_bfd_storage;
       /* If the object requires gdb to do relocations, we simply fall
 	 back to not sharing data across users.  These cases are rare
 	 enough that this seems reasonable.  */
       if (abfd != NULL && !gdb_bfd_requires_relocations (abfd))
-	{
-	  storage
-	    = ((struct objfile_per_bfd_storage *)
-	       bfd_alloc (abfd, sizeof (struct objfile_per_bfd_storage)));
-	  /* objfile_per_bfd_storage is not trivially constructible, must
-	     call the ctor manually.  */
-	  storage = new (storage) objfile_per_bfd_storage ();
-	  set_bfd_data (abfd, objfiles_bfd_data, storage);
-	}
-      else
-	storage
-	  = obstack_new<objfile_per_bfd_storage> (&objfile->objfile_obstack);
+	set_bfd_data (abfd, objfiles_bfd_data, storage);
 
       /* Look up the gdbarch associated with the BFD.  */
       if (abfd != NULL)
 	storage->gdbarch = gdbarch_from_bfd (abfd);
-
-      storage->language_of_main = language_unknown;
     }
 
   return storage;
 }
 
-/* Free STORAGE.  */
-
-static void
-free_objfile_per_bfd_storage (struct objfile_per_bfd_storage *storage)
-{
-  if (storage->demangled_names_hash)
-    htab_delete (storage->demangled_names_hash);
-  storage->~objfile_per_bfd_storage ();
-}
-
-/* A wrapper for free_objfile_per_bfd_storage that can be passed as a
+/* A deleter for objfile_per_bfd_storage that can be passed as a
    cleanup function to the BFD registry.  */
 
 static void
 objfile_bfd_data_free (struct bfd *unused, void *d)
 {
-  free_objfile_per_bfd_storage ((struct objfile_per_bfd_storage *) d);
+  delete (struct objfile_per_bfd_storage *) d;
 }
 
 /* See objfiles.h.  */
@@ -670,7 +652,7 @@ objfile::~objfile ()
   if (obfd)
     gdb_bfd_unref (obfd);
   else
-    free_objfile_per_bfd_storage (per_bfd);
+    delete per_bfd;
 
   /* Remove it from the chain of all objfiles.  */
 
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index 843c44e1d24..9db212a1d13 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -234,6 +234,8 @@ struct objfile_per_bfd_storage
     : minsyms_read (false)
   {}
 
+  ~objfile_per_bfd_storage ();
+
   /* The storage has an obstack of its own.  */
 
   auto_obstack storage_obstack;
-- 
2.17.2

Re: [PATCH 0/9] Minor minimal symbol improvements

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> While working on an experiment, I found a number of small improvements
Tom> that could be made in the minsym code.  This series is the result.

Tom> Tested by the buildbot.  Let me know what you think.

I'm checking this in now.

Tom