[binutils-gdb] gdb: change subfile::line_vector to an std::vector

[binutils-gdb] gdb: change subfile::line_vector to an std::vector

[Simon Marchi]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=558802e4d1c5dcbd0df7d2c6ef62a6deac247a2f

commit 558802e4d1c5dcbd0df7d2c6ef62a6deac247a2f
Author: Simon Marchi <simon.marchi@polymtl.ca>
Date:   Thu Apr 7 08:55:16 2022 -0400

    gdb: change subfile::line_vector to an std::vector
    
    Change this field to an std::vector to facilitate memory management.
    Since the linetable_entry array is copied into the symtab resulting from
    the subfile, it is possible to change it without changing how symtab
    stores the linetable entries (which would be a much larger change).
    
    There is a small change in buildsym_compunit::record_line to avoid
    accessing a now invalid linetable_entry.  Before this patch, we keep a
    pointer to the last linetable entry, pop it from the vector, and then
    read last->line.  It works with the manually-maintained array, but since
    we now use std::vector::pop_back, I am afraid that it could be flagged
    as an invalid access by the various static / dynamic analysis tools to
    access the linetable_entry object after popping it from the vector.
    Instead, record just the line number in an optional and use it.
    
    There are substantial changes in xcoffread.c that simplify the code, but
    I can't test them.  I was hesitant to do this change because of that,
    but I decided to send it anyway.  I don't think that an almost dead
    platform should hold back improving the code in the common parts of GDB.
    
    The changes in xcoffread.c are:
    
     - Make arrange_linetable "arrange" the linetable passed as a parameter,
       instead of returning possibly a new one, possibly the same one.
     - In the "Process main file's line numbers.", I'm not too sure what
       happens.  We get the lintable from "main_subfile", "arrange" it, but
       then assign the result to the current subfile, obtained with
       get_current_subfile.  I assume that the current subfile is also the
       main one, so now I just call arrange_linetable on the main subfile's
       line table.
     - Remove that weird "Useless if!!!" FIXME comment.  It's been there
       forever, but the "if" is still there, so I guess the "if" can stay
       there.
    
    Change-Id: I11799006fd85189e8cf5bd3a168f8f38c2c27a80

Diff:
---
 gdb/buildsym.c  |  94 ++++++++++++++++----------------------------
 gdb/buildsym.h  |   6 +--
 gdb/xcoffread.c | 118 +++++++++++++++-----------------------------------------
 3 files changed, 67 insertions(+), 151 deletions(-)

diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index 034db598735..628903d674f 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -49,13 +49,6 @@ struct pending_block
     struct block *block;
   };
 
-/* Initial sizes of data structures.  These are realloc'd larger if
-   needed, and realloc'd down to the size actually used, when
-   completed.  */
-
-#define	INITIAL_LINE_VECTOR_LENGTH	1000
-\f
-
 buildsym_compunit::buildsym_compunit (struct objfile *objfile_,
 				      const char *name,
 				      const char *comp_dir_,
@@ -95,7 +88,6 @@ buildsym_compunit::~buildsym_compunit ()
        subfile = nextsub)
     {
       nextsub = subfile->next;
-      xfree (subfile->line_vector);
       delete subfile;
     }
 
@@ -536,9 +528,6 @@ buildsym_compunit::start_subfile (const char *name)
 
   m_current_subfile = subfile.get ();
 
-  /* Initialize line-number recording for this subfile.  */
-  subfile->line_vector = NULL;
-
   /* Default the source language to whatever can be deduced from the
      filename.  If nothing can be deduced (such as for a C/C++ include
      file with a ".h" extension), then inherit whatever language the
@@ -657,28 +646,7 @@ void
 buildsym_compunit::record_line (struct subfile *subfile, int line,
 				CORE_ADDR pc, linetable_entry_flags flags)
 {
-  struct linetable_entry *e;
-
-  /* Make sure line vector exists and is big enough.  */
-  if (!subfile->line_vector)
-    {
-      subfile->line_vector_length = INITIAL_LINE_VECTOR_LENGTH;
-      subfile->line_vector = (struct linetable *)
-	xmalloc (sizeof (struct linetable)
-	   + subfile->line_vector_length * sizeof (struct linetable_entry));
-      subfile->line_vector->nitems = 0;
-      m_have_line_numbers = true;
-    }
-
-  if (subfile->line_vector->nitems >= subfile->line_vector_length)
-    {
-      subfile->line_vector_length *= 2;
-      subfile->line_vector = (struct linetable *)
-	xrealloc ((char *) subfile->line_vector,
-		  (sizeof (struct linetable)
-		   + (subfile->line_vector_length
-		      * sizeof (struct linetable_entry))));
-    }
+  m_have_line_numbers = true;
 
   /* Normally, we treat lines as unsorted.  But the end of sequence
      marker is special.  We sort line markers at the same PC by line
@@ -695,25 +663,30 @@ buildsym_compunit::record_line (struct subfile *subfile, int line,
      anyway.  */
   if (line == 0)
     {
-      struct linetable_entry *last = nullptr;
-      while (subfile->line_vector->nitems > 0)
+      gdb::optional<int> last_line;
+
+      while (!subfile->line_vector_entries.empty ())
 	{
-	  last = subfile->line_vector->item + subfile->line_vector->nitems - 1;
+	  linetable_entry *last = &subfile->line_vector_entries.back ();
+	  last_line = last->line;
+
 	  if (last->pc != pc)
 	    break;
-	  subfile->line_vector->nitems--;
+
+	  subfile->line_vector_entries.pop_back ();
 	}
 
       /* Ignore an end-of-sequence marker marking an empty sequence.  */
-      if (last == nullptr || last->line == 0)
+      if (!last_line.has_value () || *last_line == 0)
 	return;
     }
 
-  e = subfile->line_vector->item + subfile->line_vector->nitems++;
-  e->line = line;
-  e->is_stmt = (flags & LEF_IS_STMT) != 0;
-  e->pc = pc;
-  e->prologue_end = (flags & LEF_PROLOGUE_END) != 0;
+  subfile->line_vector_entries.emplace_back ();
+  linetable_entry &e = subfile->line_vector_entries.back ();
+  e.line = line;
+  e.is_stmt = (flags & LEF_IS_STMT) != 0;
+  e.pc = pc;
+  e.prologue_end = (flags & LEF_PROLOGUE_END) != 0;
 }
 
 \f
@@ -738,7 +711,7 @@ buildsym_compunit::watch_main_source_file_lossage ()
   /* If the main source file doesn't have any line number or symbol
      info, look for an alias in another subfile.  */
 
-  if (mainsub->line_vector == NULL
+  if (mainsub->line_vector_entries.empty ()
       && mainsub->symtab == NULL)
     {
       const char *mainbase = lbasename (mainsub->name.c_str ());
@@ -771,8 +744,8 @@ buildsym_compunit::watch_main_source_file_lossage ()
 	     Copy its line_vector and symtab to the main subfile
 	     and then discard it.  */
 
-	  mainsub->line_vector = mainsub_alias->line_vector;
-	  mainsub->line_vector_length = mainsub_alias->line_vector_length;
+	  mainsub->line_vector_entries
+	    = std::move (mainsub_alias->line_vector_entries);
 	  mainsub->symtab = mainsub_alias->symtab;
 
 	  if (prev_mainsub_alias == NULL)
@@ -929,13 +902,8 @@ buildsym_compunit::end_compunit_symtab_with_blockvector
        subfile != NULL;
        subfile = subfile->next)
     {
-      int linetablesize = 0;
-
-      if (subfile->line_vector)
+      if (!subfile->line_vector_entries.empty ())
 	{
-	  linetablesize = sizeof (struct linetable) +
-	    subfile->line_vector->nitems * sizeof (struct linetable_entry);
-
 	  const auto lte_is_less_than
 	    = [] (const linetable_entry &ln1,
 		  const linetable_entry &ln2) -> bool
@@ -953,9 +921,8 @@ buildsym_compunit::end_compunit_symtab_with_blockvector
 	     address, as this maintains the inline function caller/callee
 	     relationships, this is why std::stable_sort is used.  */
 	  if (m_objfile->flags & OBJF_REORDERED)
-	    std::stable_sort (subfile->line_vector->item,
-			      subfile->line_vector->item
-			      + subfile->line_vector->nitems,
+	    std::stable_sort (subfile->line_vector_entries.begin (),
+			      subfile->line_vector_entries.end (),
 			      lte_is_less_than);
 	}
 
@@ -967,13 +934,20 @@ buildsym_compunit::end_compunit_symtab_with_blockvector
 
       /* Fill in its components.  */
 
-      if (subfile->line_vector)
+      if (!subfile->line_vector_entries.empty ())
 	{
-	  /* Reallocate the line table on the symbol obstack.  */
+	  /* Reallocate the line table on the objfile obstack.  */
+	  size_t n_entries = subfile->line_vector_entries.size ();
+	  size_t entry_array_size = n_entries * sizeof (struct linetable_entry);
+	  int linetablesize = sizeof (struct linetable) + entry_array_size;
+
 	  symtab->set_linetable
-	    ((struct linetable *)
-	     obstack_alloc (&m_objfile->objfile_obstack, linetablesize));
-	  memcpy (symtab->linetable (), subfile->line_vector, linetablesize);
+	    (XOBNEWVAR (&m_objfile->objfile_obstack, struct linetable,
+			linetablesize));
+
+	  symtab->linetable ()->nitems = n_entries;
+	  memcpy (symtab->linetable ()->item,
+		  subfile->line_vector_entries.data (), entry_array_size);
 	}
       else
 	symtab->set_linetable (nullptr);
diff --git a/gdb/buildsym.h b/gdb/buildsym.h
index 6284aafc878..ee75e6fd95d 100644
--- a/gdb/buildsym.h
+++ b/gdb/buildsym.h
@@ -20,6 +20,7 @@
 #define BUILDSYM_H 1
 
 #include "gdbsupport/gdb_obstack.h"
+#include "symtab.h"
 
 struct objfile;
 struct symbol;
@@ -53,10 +54,7 @@ struct subfile
 
   struct subfile *next = nullptr;
   std::string name;
-
-  /* Space for this is malloc'd.  */
-  struct linetable *line_vector = nullptr;
-  int line_vector_length = 0;
+  std::vector<linetable_entry> line_vector_entries;
   enum language language = language_unknown;
   struct symtab *symtab = nullptr;
 };
diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
index b89e98ae05f..566c0824cd5 100644
--- a/gdb/xcoffread.c
+++ b/gdb/xcoffread.c
@@ -239,8 +239,6 @@ static void read_xcoff_symtab (struct objfile *, legacy_psymtab *);
 static void add_stab_to_list (char *, struct pending_stabs **);
 #endif
 
-static struct linetable *arrange_linetable (struct linetable *);
-
 static void record_include_end (struct coff_symbol *);
 
 static void process_linenos (CORE_ADDR, CORE_ADDR);
@@ -412,89 +410,77 @@ add_stab_to_list (char *stabname, struct pending_stabs **stabvector)
 
 /* Given a line table with function entries are marked, arrange its
    functions in ascending order and strip off function entry markers
-   and return it in a newly created table.  If the old one is good
-   enough, return the old one.  */
+   and return it in a newly created table.  */
+
 /* FIXME: I think all this stuff can be replaced by just passing
    sort_linevec = 1 to end_compunit_symtab.  */
 
-static struct linetable *
-arrange_linetable (struct linetable *oldLineTb)
+static void
+arrange_linetable (std::vector<linetable_entry> &old_linetable)
 {
   int extra_lines = 0;
 
   std::vector<linetable_entry> fentries;
 
-  for (int ii = 0; ii < oldLineTb->nitems; ++ii)
+  for (int ii = 0; ii < old_linetable.size (); ++ii)
     {
-      if (oldLineTb->item[ii].is_stmt == 0)
+      if (old_linetable[ii].is_stmt == 0)
 	continue;
 
-      if (oldLineTb->item[ii].line == 0)
+      if (old_linetable[ii].line == 0)
 	{
 	  /* Function entry found.  */
 	  fentries.emplace_back ();
 	  linetable_entry &e = fentries.back ();
 	  e.line = ii;
 	  e.is_stmt = 1;
-	  e.pc = oldLineTb->item[ii].pc;
+	  e.pc = old_linetable[ii].pc;
 
 	  /* If the function was compiled with XLC, we may have to add an
 	     extra line entry later.  Reserve space for that.  */
-	  if (ii + 1 < oldLineTb->nitems
-	      && oldLineTb->item[ii].pc != oldLineTb->item[ii + 1].pc)
+	  if (ii + 1 < old_linetable.size ()
+	      && old_linetable[ii].pc != old_linetable[ii + 1].pc)
 	    extra_lines++;
 	}
     }
 
   if (fentries.empty ())
-    return oldLineTb;
+    return;
 
   std::sort (fentries.begin (), fentries.end (),
 	     [] (const linetable_entry &lte1, const linetable_entry& lte2)
 	     { return lte1.pc < lte2.pc; });
 
   /* Allocate a new line table.  */
-  int new_linetable_nitems = oldLineTb->nitems - fentries.size () + extra_lines;
-  linetable *new_linetable
-    = XNEWVAR (linetable,
-	       (sizeof (struct linetable)
-		+ (new_linetable_nitems * sizeof (struct linetable_entry))));
+  std::vector<linetable_entry> new_linetable;
+  new_linetable.reserve (old_linetable.size ());
 
   /* If line table does not start with a function beginning, copy up until
      a function begin.  */
-
-  int newline = 0;
-  if (oldLineTb->item[0].line != 0)
-    for (newline = 0;
-	 newline < oldLineTb->nitems && oldLineTb->item[newline].line;
-	 ++newline)
-      new_linetable->item[newline] = oldLineTb->item[newline];
+  for (int i = 0; i < old_linetable.size () && old_linetable[i].line != 0; ++i)
+    new_linetable.push_back (old_linetable[i]);
 
   /* Now copy function lines one by one.  */
-
   for (const linetable_entry &entry : fentries)
     {
       /* If the function was compiled with XLC, we may have to add an
 	 extra line to cover the function prologue.  */
       int jj = entry.line;
-      if (jj + 1 < oldLineTb->nitems
-	  && oldLineTb->item[jj].pc != oldLineTb->item[jj + 1].pc)
+      if (jj + 1 < old_linetable.size ()
+	  && old_linetable[jj].pc != old_linetable[jj + 1].pc)
 	{
-	  new_linetable->item[newline] = oldLineTb->item[jj];
-	  new_linetable->item[newline].line = oldLineTb->item[jj + 1].line;
-	  newline++;
+	  new_linetable.push_back (old_linetable[jj]);
+	  new_linetable.back ().line = old_linetable[jj + 1].line;
 	}
 
       for (jj = entry.line + 1;
-	   jj < oldLineTb->nitems && oldLineTb->item[jj].line != 0;
-	   ++jj, ++newline)
-	new_linetable->item[newline] = oldLineTb->item[jj];
+	   jj < old_linetable.size () && old_linetable[jj].line != 0;
+	   ++jj)
+	new_linetable.push_back (old_linetable[jj]);
     }
 
-  /* The number of items in the line table must include these
-     extra lines which were added in case of XLC compiled functions.  */
-  new_linetable->nitems = new_linetable_nitems;
-  return new_linetable;
+  new_linetable.shrink_to_fit ();
+  old_linetable = std::move (new_linetable);
 }
 
 /* include file support: C_BINCL/C_EINCL pairs will be kept in the 
@@ -595,7 +581,7 @@ static struct objfile *this_symtab_objfile;
 static void
 process_linenos (CORE_ADDR start, CORE_ADDR end)
 {
-  int offset, ii;
+  int offset;
   file_ptr max_offset
     = XCOFF_DATA (this_symtab_objfile)->max_lineno_offset;
 
@@ -631,7 +617,7 @@ process_linenos (CORE_ADDR start, CORE_ADDR end)
 	coff_data (this_symtab_objfile->obfd)->local_linesz;
       main_source_baseline = 0;
 
-      for (ii = 0; ii < inclIndx; ++ii)
+      for (int ii = 0; ii < inclIndx; ++ii)
 	{
 	  /* If there is main file source before include file, enter it.  */
 	  if (offset < inclTable[ii].begin)
@@ -678,49 +664,23 @@ process_linenos (CORE_ADDR start, CORE_ADDR end)
     }
 
   /* Process main file's line numbers.  */
-  if (main_subfile.line_vector)
+  if (!main_subfile.line_vector_entries.empty ())
     {
-      struct linetable *lineTb, *lv;
-
-      lv = main_subfile.line_vector;
-
       /* Line numbers are not necessarily ordered.  xlc compilation will
 	 put static function to the end.  */
-
-      struct subfile *current_subfile = get_current_subfile ();
-      lineTb = arrange_linetable (lv);
-      if (lv == lineTb)
-	{
-	  current_subfile->line_vector = (struct linetable *)
-	    xrealloc (lv, (sizeof (struct linetable)
-			   + lv->nitems * sizeof (struct linetable_entry)));
-	}
-      else
-	{
-	  xfree (lv);
-	  current_subfile->line_vector = lineTb;
-	}
-
-      current_subfile->line_vector_length =
-	current_subfile->line_vector->nitems;
+      arrange_linetable (main_subfile.line_vector_entries);
     }
 
   /* Now, process included files' line numbers.  */
 
-  for (ii = 0; ii < inclIndx; ++ii)
+  for (int ii = 0; ii < inclIndx; ++ii)
     {
       if (inclTable[ii].subfile != ((struct subfile *) &main_subfile)
-	  && (inclTable[ii].subfile)->line_vector)	/* Useless if!!!
-							   FIXMEmgo */
+	  && !inclTable[ii].subfile->line_vector_entries.empty ())
 	{
-	  struct linetable *lineTb, *lv;
-
-	  lv = (inclTable[ii].subfile)->line_vector;
-
 	  /* Line numbers are not necessarily ordered.  xlc compilation will
 	     put static function to the end.  */
-
-	  lineTb = arrange_linetable (lv);
+	  arrange_linetable (inclTable[ii].subfile->line_vector_entries);
 
 	  push_subfile ();
 
@@ -755,22 +715,6 @@ process_linenos (CORE_ADDR start, CORE_ADDR end)
 	  current_subfile->name = inclTable[ii].name;
 #endif
 
-	  if (lv == lineTb)
-	    {
-	      current_subfile->line_vector =
-		(struct linetable *) xrealloc
-		(lv, (sizeof (struct linetable)
-		      + lv->nitems * sizeof (struct linetable_entry)));
-
-	    }
-	  else
-	    {
-	      xfree (lv);
-	      current_subfile->line_vector = lineTb;
-	    }
-
-	  current_subfile->line_vector_length =
-	    current_subfile->line_vector->nitems;
 	  start_subfile (pop_subfile ());
 	}
     }