gdb: replace msym_bunch with deque

This patch replaces the msym_bunch implementation with an std::deque.

I initially tried to replace it with a vector.  However, that doesn't
work, because minimal_symbol references need to stay valid across calls
to minimal_symbol_reader::record_full in at least one spot.
elf_symtab_read calls minimal_symbol_reader::record_full (through
function record_minimal_symbol) to record a minimal symbol for a PLT
entry and then uses a previously added minimal symbol to set the size of
the PLT minimal symbol.  If we used a vector, a re-allocation could
happen which would invalidate the reference to the previous minimal
symbol (luckily this was caught by ASan).

An std::deque implementation typically uses a sequence of fixed-sized
arrays, much like our current msym_bunch implementation.  So adding an
item at the end will not invalidate existing references.  But unlike our
msym_bunch, we don't have to worry about memory management.

I was a bit puzzled about this code in
minimal_symbol_reader::record_full:

  /* If we already read minimal symbols for this objfile, then don't
     ever allocate a new one.  */
  if (!m_objfile->per_bfd->minsyms_read)
    {
      m_msym_bunch_index++;
      m_objfile->per_bfd->n_minsyms++;
    }

From what I understand, this "feature" gets used when you have
ECOFF debug info in an ELF executable.  We first parse the ELF minimal
symbols in elf_symfile_read, then go into elfmdebug_build_psymtabs.
elfmdebug_build_psymtabs has the capability to generate minimal symbols
(useful when you use ECOFF debug info in an ECOFF executable I guess),
but in this case we already have the ELF ones, so we don't want to
record the ECOFF minimal symbols.  Hence this mechanism to suppress new
minimal symbols.

The consequence of this patch, I believe, is that
minimal_symbol_reader::record_full will unnecessarily allocate minimal
symbols in this case.  These minimal symbols won't be installed, because
of the check in minimal_symbol_reader::install.  The minimal symbols
will be freed when the minimal_symbol_reader gets destroyed.  That means
a higher temporary memory usage when reading an ECOFF-in-ELF file, but
no change in behavior.  See discussion at [1].

[1] https://inbox.sourceware.org/gdb-patches/85958fad-17e1-4593-b842-d60a6610f149@polymtl.ca/T/#meaf9b53da296e7f6872b441ec97038d172ca907f

Change-Id: I7d10c6aca42cc9dcf80b483394e1e56351a9465f
Approved-By: Tom Tromey <tom@tromey.com>

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 70f5f61748a6d2bc2946a3a85e4c0932ca38fa77..1b1e43845778beb3faf4d38c7107b37a82d573ea 100644 (file)
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -143,17 +143,6 @@ msymbol_is_function (struct objfile *objfile, minimal_symbol *minsym,
     }
 }
 
-/* Accumulate the minimal symbols for each objfile in bunches of BUNCH_SIZE.
-   At the end, copy them all into one newly allocated array.  */
-
-#define BUNCH_SIZE 127
-
-struct msym_bunch
-  {
-    struct msym_bunch *next;
-    struct minimal_symbol contents[BUNCH_SIZE];
-  };
-
 /* See minsyms.h.  */
 
 unsigned int
@@ -1087,32 +1076,10 @@ get_symbol_leading_char (program_space *pspace, bfd *abfd)
 /* See minsyms.h.  */
 
 minimal_symbol_reader::minimal_symbol_reader (struct objfile *obj)
-: m_objfile (obj),
-  m_msym_bunch (NULL),
-  /* Note that presetting m_msym_bunch_index to BUNCH_SIZE causes the
-     first call to save a minimal symbol to allocate the memory for
-     the first bunch.  */
-  m_msym_bunch_index (BUNCH_SIZE),
-  m_msym_count (0)
+  : m_objfile (obj)
 {
 }
 
-/* Discard the currently collected minimal symbols, if any.  If we wish
-   to save them for later use, we must have already copied them somewhere
-   else before calling this function.  */
-
-minimal_symbol_reader::~minimal_symbol_reader ()
-{
-  struct msym_bunch *next;
-
-  while (m_msym_bunch != NULL)
-    {
-      next = m_msym_bunch->next;
-      xfree (m_msym_bunch);
-      m_msym_bunch = next;
-    }
-}
-
 /* See minsyms.h.  */
 
 void
@@ -1180,9 +1147,6 @@ minimal_symbol_reader::record_full (std::string_view name,
                                    enum minimal_symbol_type ms_type,
                                    int section)
 {
-  struct msym_bunch *newobj;
-  struct minimal_symbol *msymbol;
-
   /* Don't put gcc_compiled, __gnu_compiled_cplus, and friends into
      the minimal symbols, because if there is also another symbol
      at the same address (e.g. the first function of the file),
@@ -1207,14 +1171,7 @@ minimal_symbol_reader::record_full (std::string_view name,
                                hex_string (LONGEST (address)),
                                section, (int) name.size (), name.data ());
 
-  if (m_msym_bunch_index == BUNCH_SIZE)
-    {
-      newobj = XCNEW (struct msym_bunch);
-      m_msym_bunch_index = 0;
-      newobj->next = m_msym_bunch;
-      m_msym_bunch = newobj;
-    }
-  msymbol = &m_msym_bunch->contents[m_msym_bunch_index];
+  minimal_symbol *msymbol = &m_msyms.emplace_back ();
   msymbol->set_language (language_unknown,
                         &m_objfile->per_bfd->storage_obstack);
 
@@ -1226,17 +1183,13 @@ minimal_symbol_reader::record_full (std::string_view name,
 
   msymbol->set_unrelocated_address (address);
   msymbol->set_section_index (section);
-
   msymbol->set_type (ms_type);
 
   /* If we already read minimal symbols for this objfile, then don't
      ever allocate a new one.  */
   if (!m_objfile->per_bfd->minsyms_read)
-    {
-      m_msym_bunch_index++;
-      m_objfile->per_bfd->n_minsyms++;
-    }
-  m_msym_count++;
+    m_objfile->per_bfd->n_minsyms++;
+
   return msymbol;
 }
 
@@ -1465,59 +1418,41 @@ private:
 void
 minimal_symbol_reader::install ()
 {
-  int mcount;
-  struct msym_bunch *bunch;
-  struct minimal_symbol *msymbols;
-  int alloc_count;
-
   if (m_objfile->per_bfd->minsyms_read)
     return;
 
-  if (m_msym_count > 0)
+  if (!m_msyms.empty ())
     {
-      symtab_create_debug_printf ("installing %d minimal symbols of objfile %s",
-                                 m_msym_count, objfile_name (m_objfile));
+      symtab_create_debug_printf ("installing %zu minimal symbols of objfile %s",
+                                 m_msyms.size (), objfile_name (m_objfile));
 
       /* Allocate enough space, into which we will gather the bunches
         of new and existing minimal symbols, sort them, and then
         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;
+      int alloc_count
+       = m_msyms.size () + m_objfile->per_bfd->minimal_symbol_count;
       gdb::unique_xmalloc_ptr<minimal_symbol>
        msym_holder (XNEWVEC (minimal_symbol, alloc_count));
-      msymbols = msym_holder.get ();
+      minimal_symbol *msymbols = msym_holder.get ();
 
       /* Copy in the existing minimal symbols, if there are any.  */
-
       if (m_objfile->per_bfd->minimal_symbol_count)
        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
-        current, possibly partially filled bunch (thus we use the current
-        msym_bunch_index for the first bunch we copy over), and thereafter
-        each bunch is full.  */
-
-      mcount = m_objfile->per_bfd->minimal_symbol_count;
-
-      for (bunch = m_msym_bunch; bunch != NULL; bunch = bunch->next)
-       {
-         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;
-       }
+      /* Walk through the list of recorded minimal symbol, adding each symbol
+        to the new contiguous array of symbols.  */
+      int mcount = m_objfile->per_bfd->minimal_symbol_count;
+      std::copy (m_msyms.begin (), m_msyms.end (), &msymbols[mcount]);
+      mcount += m_msyms.size ();
 
       /* Sort the minimal symbols by address.  */
-
       std::sort (msymbols, msymbols + mcount, minimal_symbol_is_less_than);
 
       /* Compact out any duplicates, and free up whatever space we are
         no longer using.  */
-
       mcount = compact_minimal_symbols (msymbols, mcount, m_objfile);
       msym_holder.reset (XRESIZEVEC (struct minimal_symbol,
                                     msym_holder.release (),

diff --git a/gdb/minsyms.h b/gdb/minsyms.h
index ed38044a38c620b6d76c75276eb533f8039cdbd7..97be03953d57e52b7c0969e6cb595b75d15a91e3 100644 (file)
--- a/gdb/minsyms.h
+++ b/gdb/minsyms.h
@@ -20,6 +20,8 @@
 #ifndef GDB_MINSYMS_H
 #define GDB_MINSYMS_H
 
+#include <deque>
+
 struct program_space;
 struct type;
 
@@ -78,8 +80,6 @@ struct bound_minimal_symbol
    as opaque and use functions provided by minsyms.c to inspect them.
 */
 
-struct msym_bunch;
-
 /* An RAII-based object that is used to record minimal symbols while
    they are being read.  */
 class minimal_symbol_reader
@@ -92,8 +92,6 @@ class minimal_symbol_reader
 
   explicit minimal_symbol_reader (struct objfile *);
 
-  ~minimal_symbol_reader ();
-
   /* Install the minimal symbols that have been collected into the
      given objfile.  */
 
@@ -154,19 +152,10 @@ class minimal_symbol_reader
 
   struct objfile *m_objfile;
 
-  /* Bunch currently being filled up.
-     The next field points to chain of filled bunches.  */
-
-  struct msym_bunch *m_msym_bunch;
-
-  /* Number of slots filled in current bunch.  */
-
-  int m_msym_bunch_index;
-
-  /* Total number of minimal symbols recorded so far for the
-     objfile.  */
-
-  int m_msym_count;
+  /* The minimal symbols recorded so far.  This uses a deque instead of e.g. a
+     vector, because references to minimal symbols need to stay valid across
+     calls to record_full.  */
+  std::deque<minimal_symbol> m_msyms;
 };
 
 \f