[PATCH 1/4] gdb: allocate subfile with new

[PATCH 1/4] gdb: allocate subfile with new

[Simon Marchi]

Allocate struct subfile with new, initialize its fields instead of
memset-ing it to 0.  Use a unique_ptr for the window after a subfile has
been allocated but before it is linked in the buildsym_compunit's list
of subfile (and therefore owned by the buildsym_compunit.

I can't test the change in xcoffread.c, it's best-effort.  I couldn't
find where subfiles are freed in that file, I assume they were
intentionally (or not) leaked.

Change-Id: Ib3b6877de31b7e65bc466682f08dbf5840225f24
---
 gdb/buildsym.c  | 37 ++++++++++++++++---------------------
 gdb/buildsym.h  | 22 ++++++++++++++++------
 gdb/xcoffread.c | 10 ++--------
 3 files changed, 34 insertions(+), 35 deletions(-)

diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index 0df355151f45..aedc3c67b9ac 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -97,7 +97,7 @@ buildsym_compunit::~buildsym_compunit ()
       nextsub = subfile->next;
       xfree (subfile->name);
       xfree (subfile->line_vector);
-      xfree (subfile);
+      delete subfile;
     }
 
   struct pending *next, *next1;
@@ -504,13 +504,12 @@ void
 buildsym_compunit::start_subfile (const char *name)
 {
   const char *subfile_dirname;
-  struct subfile *subfile;
 
   subfile_dirname = m_comp_dir.get ();
 
   /* See if this subfile is already registered.  */
 
-  for (subfile = m_subfiles; subfile; subfile = subfile->next)
+  for (subfile *subfile = m_subfiles; subfile; subfile = subfile->next)
     {
       char *subfile_name;
 
@@ -537,13 +536,9 @@ buildsym_compunit::start_subfile (const char *name)
 
   /* This subfile is not known.  Add an entry for it.  */
 
-  subfile = XNEW (struct subfile);
-  memset (subfile, 0, sizeof (struct subfile));
-
-  subfile->next = m_subfiles;
-  m_subfiles = subfile;
+  subfile_up subfile (new struct subfile);
 
-  m_current_subfile = subfile;
+  m_current_subfile = subfile.get ();
 
   subfile->name = xstrdup (name);
 
@@ -562,11 +557,8 @@ buildsym_compunit::start_subfile (const char *name)
      source file.  */
 
   subfile->language = deduce_language_from_filename (subfile->name);
-  if (subfile->language == language_unknown
-      && subfile->next != NULL)
-    {
-      subfile->language = subfile->next->language;
-    }
+  if (subfile->language == language_unknown && m_subfiles != nullptr)
+    subfile->language = m_subfiles->language;
 
   /* If the filename of this subfile ends in .C, then change the
      language of any pending subfiles from C to C++.  We also accept
@@ -586,12 +578,14 @@ buildsym_compunit::start_subfile (const char *name)
 
   /* And patch up this file if necessary.  */
   if (subfile->language == language_c
-      && subfile->next != NULL
-      && (subfile->next->language == language_cplus
-	  || subfile->next->language == language_fortran))
-    {
-      subfile->language = subfile->next->language;
-    }
+      && m_subfiles != nullptr
+      && (m_subfiles->language == language_cplus
+	  || m_subfiles->language == language_fortran))
+    subfile->language = m_subfiles->language;
+
+  /* Link this subfile at the front of the subfile list.  */
+  subfile->next = m_subfiles;
+  m_subfiles = subfile.release ();
 }
 
 /* For stabs readers, the first N_SO symbol is assumed to be the
@@ -791,7 +785,8 @@ buildsym_compunit::watch_main_source_file_lossage ()
 	  else
 	    prev_mainsub_alias->next = mainsub_alias->next;
 	  xfree (mainsub_alias->name);
-	  xfree (mainsub_alias);
+
+	  delete mainsub_alias;
 	}
     }
 }
diff --git a/gdb/buildsym.h b/gdb/buildsym.h
index 5f0e0230fd9f..50ecd33d5441 100644
--- a/gdb/buildsym.h
+++ b/gdb/buildsym.h
@@ -45,16 +45,26 @@ struct dynamic_prop;
 
 struct subfile
 {
-  struct subfile *next;
+  subfile () = default;
+
+  /* There's nothing wrong with copying a subfile, but we don't need to, so use
+     this to avoid copying one by mistake.  */
+  DISABLE_COPY_AND_ASSIGN (subfile);
+
+  struct subfile *next = nullptr;
+
   /* Space for this is malloc'd.  */
-  char *name;
+  char *name = nullptr;
+
   /* Space for this is malloc'd.  */
-  struct linetable *line_vector;
-  int line_vector_length;
-  enum language language;
-  struct symtab *symtab;
+  struct linetable *line_vector = nullptr;
+  int line_vector_length = 0;
+  enum language language = language_unknown;
+  struct symtab *symtab = nullptr;
 };
 
+using subfile_up = std::unique_ptr<subfile>;
+
 /* Record the symbols defined for each context in a list.  We don't
    create a struct block for the context until we know how long to
    make it.  */
diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
index c5d2d0a492f8..296e71356dde 100644
--- a/gdb/xcoffread.c
+++ b/gdb/xcoffread.c
@@ -632,8 +632,6 @@ process_linenos (CORE_ADDR start, CORE_ADDR end)
   if (offset == 0)
     goto return_after_cleanup;
 
-  memset (&main_subfile, '\0', sizeof (main_subfile));
-
   if (inclIndx == 0)
     /* All source lines were in the main source file.  None in include
        files.  */
@@ -651,8 +649,6 @@ process_linenos (CORE_ADDR start, CORE_ADDR end)
 
       for (ii = 0; ii < inclIndx; ++ii)
 	{
-	  struct subfile *tmpSubfile;
-
 	  /* If there is main file source before include file, enter it.  */
 	  if (offset < inclTable[ii].begin)
 	    {
@@ -675,14 +671,12 @@ process_linenos (CORE_ADDR start, CORE_ADDR end)
 	  else
 	    {
 	      /* Have a new subfile for the include file.  */
+	      inclTable[ii].subfile = new subfile;
 
-	      tmpSubfile = inclTable[ii].subfile = XNEW (struct subfile);
-
-	      memset (tmpSubfile, '\0', sizeof (struct subfile));
 	      firstLine = &(inclTable[ii].funStartLine);
 
 	      /* Enter include file's lines now.  */
-	      enter_line_range (tmpSubfile, inclTable[ii].begin,
+	      enter_line_range (inclTable[ii].subfile, inclTable[ii].begin,
 				inclTable[ii].end, start, 0, firstLine);
 	    }
 

base-commit: 50192212a72b48de7ae4d87c79d394f4e3461a5b
-- 
2.35.1

[PATCH 2/4] gdb: change subfile::name and buildsym_compunit::m_comp_dir to strings

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

Change subfile::name to be a string, for easier memory management.
Change buildsym_compunit::m_comp_dir as well, since we move one in to
the other at some point in patch_subfile_names, so it's easier to do
both at the same time.  There are various NULL checks for both fields
currently, replace them with empty checks, I think it ends up
equivalent.

I can't test the change in xcoffread.c, it's best-effort.

Change-Id: I62b5fb08b2089e096768a090627ac7617e90a016
---
 gdb/buildsym.c    | 64 +++++++++++++++++++++--------------------------
 gdb/buildsym.h    |  8 +++---
 gdb/dwarf2/read.c | 13 +++++-----
 gdb/xcoffread.c   |  3 +--
 4 files changed, 40 insertions(+), 48 deletions(-)

diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index aedc3c67b9ac..034db5987356 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -63,7 +63,7 @@ buildsym_compunit::buildsym_compunit (struct objfile *objfile_,
 				      CORE_ADDR last_addr)
   : m_objfile (objfile_),
     m_last_source_file (name == nullptr ? nullptr : xstrdup (name)),
-    m_comp_dir (comp_dir_ == nullptr ? nullptr : xstrdup (comp_dir_)),
+    m_comp_dir (comp_dir_ == nullptr ? "" : comp_dir_),
     m_language (language_),
     m_last_source_start_addr (last_addr)
 {
@@ -95,7 +95,6 @@ buildsym_compunit::~buildsym_compunit ()
        subfile = nextsub)
     {
       nextsub = subfile->next;
-      xfree (subfile->name);
       xfree (subfile->line_vector);
       delete subfile;
     }
@@ -503,45 +502,40 @@ buildsym_compunit::make_blockvector ()
 void
 buildsym_compunit::start_subfile (const char *name)
 {
-  const char *subfile_dirname;
-
-  subfile_dirname = m_comp_dir.get ();
-
   /* See if this subfile is already registered.  */
 
   for (subfile *subfile = m_subfiles; subfile; subfile = subfile->next)
     {
-      char *subfile_name;
+      std::string subfile_name_holder;
+      const char *subfile_name;
 
       /* If NAME is an absolute path, and this subfile is not, then
 	 attempt to create an absolute path to compare.  */
       if (IS_ABSOLUTE_PATH (name)
 	  && !IS_ABSOLUTE_PATH (subfile->name)
-	  && subfile_dirname != NULL)
-	subfile_name = concat (subfile_dirname, SLASH_STRING,
-			       subfile->name, (char *) NULL);
+	  && !m_comp_dir.empty ())
+	{
+	  subfile_name_holder = string_printf ("%s/%s", m_comp_dir.c_str (),
+					       subfile->name.c_str ());
+	  subfile_name = subfile_name_holder.c_str ();
+	}
       else
-	subfile_name = subfile->name;
+	subfile_name = subfile->name.c_str ();
 
       if (FILENAME_CMP (subfile_name, name) == 0)
 	{
 	  m_current_subfile = subfile;
-	  if (subfile_name != subfile->name)
-	    xfree (subfile_name);
 	  return;
 	}
-      if (subfile_name != subfile->name)
-	xfree (subfile_name);
     }
 
   /* This subfile is not known.  Add an entry for it.  */
 
   subfile_up subfile (new struct subfile);
+  subfile->name = name;
 
   m_current_subfile = subfile.get ();
 
-  subfile->name = xstrdup (name);
-
   /* Initialize line-number recording for this subfile.  */
   subfile->line_vector = NULL;
 
@@ -556,7 +550,7 @@ buildsym_compunit::start_subfile (const char *name)
      until after all the symbols have been processed for a given
      source file.  */
 
-  subfile->language = deduce_language_from_filename (subfile->name);
+  subfile->language = deduce_language_from_filename (subfile->name.c_str ());
   if (subfile->language == language_unknown && m_subfiles != nullptr)
     subfile->language = m_subfiles->language;
 
@@ -565,10 +559,10 @@ buildsym_compunit::start_subfile (const char *name)
      any other C++ suffixes accepted by deduce_language_from_filename.  */
   /* Likewise for f2c.  */
 
-  if (subfile->name)
+  if (!subfile->name.empty ())
     {
       struct subfile *s;
-      enum language sublang = deduce_language_from_filename (subfile->name);
+      language sublang = deduce_language_from_filename (subfile->name.c_str ());
 
       if (sublang == language_cplus || sublang == language_fortran)
 	for (s = m_subfiles; s != NULL; s = s->next)
@@ -605,12 +599,12 @@ buildsym_compunit::patch_subfile_names (struct subfile *subfile,
 					const char *name)
 {
   if (subfile != NULL
-      && m_comp_dir == NULL
-      && subfile->name != NULL
-      && IS_DIR_SEPARATOR (subfile->name[strlen (subfile->name) - 1]))
+      && m_comp_dir.empty ()
+      && !subfile->name.empty ()
+      && IS_DIR_SEPARATOR (subfile->name.back ()))
     {
-      m_comp_dir.reset (subfile->name);
-      subfile->name = xstrdup (name);
+      m_comp_dir = std::move (subfile->name);
+      subfile->name = name;
       set_last_source_file (name);
 
       /* Default the source language to whatever can be deduced from
@@ -624,7 +618,8 @@ buildsym_compunit::patch_subfile_names (struct subfile *subfile,
 	 symbols, since symtabs aren't allocated until after all the
 	 symbols have been processed for a given source file.  */
 
-      subfile->language = deduce_language_from_filename (subfile->name);
+      subfile->language
+	= deduce_language_from_filename (subfile->name.c_str ());
       if (subfile->language == language_unknown
 	  && subfile->next != NULL)
 	{
@@ -642,8 +637,8 @@ void
 buildsym_compunit::push_subfile ()
 {
   gdb_assert (m_current_subfile != NULL);
-  gdb_assert (m_current_subfile->name != NULL);
-  m_subfile_stack.push_back (m_current_subfile->name);
+  gdb_assert (!m_current_subfile->name.empty ());
+  m_subfile_stack.push_back (m_current_subfile->name.c_str ());
 }
 
 const char *
@@ -746,7 +741,7 @@ buildsym_compunit::watch_main_source_file_lossage ()
   if (mainsub->line_vector == NULL
       && mainsub->symtab == NULL)
     {
-      const char *mainbase = lbasename (mainsub->name);
+      const char *mainbase = lbasename (mainsub->name.c_str ());
       int nr_matches = 0;
       struct subfile *prevsub;
       struct subfile *mainsub_alias = NULL;
@@ -759,7 +754,7 @@ buildsym_compunit::watch_main_source_file_lossage ()
 	{
 	  if (subfile == mainsub)
 	    continue;
-	  if (filename_cmp (lbasename (subfile->name), mainbase) == 0)
+	  if (filename_cmp (lbasename (subfile->name.c_str ()), mainbase) == 0)
 	    {
 	      ++nr_matches;
 	      mainsub_alias = subfile;
@@ -784,7 +779,6 @@ buildsym_compunit::watch_main_source_file_lossage ()
 	    m_subfiles = mainsub_alias->next;
 	  else
 	    prev_mainsub_alias->next = mainsub_alias->next;
-	  xfree (mainsub_alias->name);
 
 	  delete mainsub_alias;
 	}
@@ -967,7 +961,8 @@ buildsym_compunit::end_compunit_symtab_with_blockvector
 
       /* Allocate a symbol table if necessary.  */
       if (subfile->symtab == NULL)
-	subfile->symtab = allocate_symtab (cu, subfile->name);
+	subfile->symtab = allocate_symtab (cu, subfile->name.c_str ());
+
       struct symtab *symtab = subfile->symtab;
 
       /* Fill in its components.  */
@@ -997,12 +992,11 @@ buildsym_compunit::end_compunit_symtab_with_blockvector
 
   /* Fill out the compunit symtab.  */
 
-  if (m_comp_dir != NULL)
+  if (!m_comp_dir.empty ())
     {
       /* Reallocate the dirname on the symbol obstack.  */
-      const char *comp_dir = m_comp_dir.get ();
       cu->set_dirname (obstack_strdup (&m_objfile->objfile_obstack,
-				       comp_dir));
+				       m_comp_dir.c_str ()));
     }
 
   /* Save the debug format string (if any) in the symtab.  */
diff --git a/gdb/buildsym.h b/gdb/buildsym.h
index 50ecd33d5441..6284aafc878f 100644
--- a/gdb/buildsym.h
+++ b/gdb/buildsym.h
@@ -52,9 +52,7 @@ struct subfile
   DISABLE_COPY_AND_ASSIGN (subfile);
 
   struct subfile *next = nullptr;
-
-  /* Space for this is malloc'd.  */
-  char *name = nullptr;
+  std::string name;
 
   /* Space for this is malloc'd.  */
   struct linetable *line_vector = nullptr;
@@ -154,7 +152,7 @@ struct buildsym_compunit
 		     CORE_ADDR last_addr, struct compunit_symtab *cust)
     : m_objfile (objfile_),
       m_last_source_file (name == nullptr ? nullptr : xstrdup (name)),
-      m_comp_dir (comp_dir_ == nullptr ? nullptr : xstrdup (comp_dir_)),
+      m_comp_dir (comp_dir_ == nullptr ? "" : comp_dir_),
       m_compunit_symtab (cust),
       m_language (language_),
       m_last_source_start_addr (last_addr)
@@ -342,7 +340,7 @@ struct buildsym_compunit
   gdb::unique_xmalloc_ptr<char> m_last_source_file;
 
   /* E.g., DW_AT_comp_dir if DWARF.  Space for this is malloc'd.  */
-  gdb::unique_xmalloc_ptr<char> m_comp_dir;
+  std::string m_comp_dir;
 
   /* Space for this is not malloc'd, and is assumed to have at least
      the same lifetime as objfile.  */
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index ac5408f25734..992ffaad6bb4 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -10766,8 +10766,8 @@ dwarf2_cu::setup_type_unit_groups (struct die_info *die)
 		 assume there's a simple mapping from
 		 cu->line_header->file_names to subfiles, plus
 		 cu->line_header->file_names may contain dups.  */
-	      b->get_current_subfile ()->symtab
-		= allocate_symtab (cust, b->get_current_subfile ()->name);
+	      const char *name = b->get_current_subfile ()->name.c_str ();
+	      b->get_current_subfile ()->symtab = allocate_symtab (cust, name);
 	    }
 
 	  fe.symtab = b->get_current_subfile ()->symtab;
@@ -21149,7 +21149,7 @@ dwarf_record_line_1 (struct gdbarch *gdbarch, struct subfile *subfile,
     {
       gdb_printf (gdb_stdlog,
 		  "Recording line %u, file %s, address %s\n",
-		  line, lbasename (subfile->name),
+		  line, lbasename (subfile->name.c_str ()),
 		  paddress (gdbarch, address));
     }
 
@@ -21173,7 +21173,7 @@ dwarf_finish_line (struct gdbarch *gdbarch, struct subfile *subfile,
     {
       gdb_printf (gdb_stdlog,
 		  "Finishing current line, file %s, address %s\n",
-		  lbasename (subfile->name),
+		  lbasename (subfile->name.c_str ()),
 		  paddress (gdbarch, address));
     }
 
@@ -21608,9 +21608,10 @@ dwarf_decode_lines (struct line_header *lh, const file_and_directory &fnd,
 	  dwarf2_start_subfile (cu, fe.name, fe.include_dir (lh));
 	  if (builder->get_current_subfile ()->symtab == NULL)
 	    {
+	      const char *name
+		= builder->get_current_subfile ()->name .c_str ();
 	      builder->get_current_subfile ()->symtab
-		= allocate_symtab (cust,
-				   builder->get_current_subfile ()->name);
+		= allocate_symtab (cust, name);
 	    }
 	  fe.symtab = builder->get_current_subfile ()->symtab;
 	}
diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
index 296e71356dde..894a12ea865c 100644
--- a/gdb/xcoffread.c
+++ b/gdb/xcoffread.c
@@ -766,10 +766,9 @@ process_linenos (CORE_ADDR start, CORE_ADDR end)
 	    if (fakename == NULL)
 	      fakename = " ?";
 	    start_subfile (fakename);
-	    xfree (get_current_subfile ()->name);
 	  }
 	  struct subfile *current_subfile = get_current_subfile ();
-	  current_subfile->name = xstrdup (inclTable[ii].name);
+	  current_subfile->name = inclTable[ii].name;
 #endif
 
 	  if (lv == lineTb)
-- 
2.35.1

[PATCH 3/4] gdb: use std::vector for temporary linetable_entry array in arrange_linetable

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

Reduce manual memory management and make the code a bit easier to read.
This helps me a bit in the following patch.

I don't have a way to test this, it's best-effort.

Change-Id: I64af9cd756311deabc6cd95e701dfb21234a40a5
---
 gdb/xcoffread.c | 78 ++++++++++++++++++++-----------------------------
 1 file changed, 31 insertions(+), 47 deletions(-)

diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
index 894a12ea865c..b89e98ae05f4 100644
--- a/gdb/xcoffread.c
+++ b/gdb/xcoffread.c
@@ -420,37 +420,23 @@ add_stab_to_list (char *stabname, struct pending_stabs **stabvector)
 static struct linetable *
 arrange_linetable (struct linetable *oldLineTb)
 {
-  int ii, jj, newline,		/* new line count */
-    function_count;		/* # of functions */
-
-  struct linetable_entry *fentry;	/* function entry vector */
-  int fentry_size;		/* # of function entries */
-  struct linetable *newLineTb;	/* new line table */
   int extra_lines = 0;
 
-#define NUM_OF_FUNCTIONS 20
-
-  fentry_size = NUM_OF_FUNCTIONS;
-  fentry = XNEWVEC (struct linetable_entry, fentry_size);
+  std::vector<linetable_entry> fentries;
 
-  for (function_count = 0, ii = 0; ii < oldLineTb->nitems; ++ii)
+  for (int ii = 0; ii < oldLineTb->nitems; ++ii)
     {
       if (oldLineTb->item[ii].is_stmt == 0)
 	continue;
 
       if (oldLineTb->item[ii].line == 0)
-	{			/* Function entry found.  */
-	  if (function_count >= fentry_size)
-	    {			/* Make sure you have room.  */
-	      fentry_size *= 2;
-	      fentry = (struct linetable_entry *)
-		xrealloc (fentry,
-			  fentry_size * sizeof (struct linetable_entry));
-	    }
-	  fentry[function_count].line = ii;
-	  fentry[function_count].is_stmt = 1;
-	  fentry[function_count].pc = oldLineTb->item[ii].pc;
-	  ++function_count;
+	{
+	  /* Function entry found.  */
+	  fentries.emplace_back ();
+	  linetable_entry &e = fentries.back ();
+	  e.line = ii;
+	  e.is_stmt = 1;
+	  e.pc = oldLineTb->item[ii].pc;
 
 	  /* If the function was compiled with XLC, we may have to add an
 	     extra line entry later.  Reserve space for that.  */
@@ -460,57 +446,55 @@ arrange_linetable (struct linetable *oldLineTb)
 	}
     }
 
-  if (function_count == 0)
-    {
-      xfree (fentry);
-      return oldLineTb;
-    }
-  else if (function_count > 1)
-    std::sort (fentry, fentry + function_count,
-	       [] (const linetable_entry &lte1, const linetable_entry& lte2)
-		{ return lte1.pc < lte2.pc; });
+  if (fentries.empty ())
+    return oldLineTb;
+
+  std::sort (fentries.begin (), fentries.end (),
+	     [] (const linetable_entry &lte1, const linetable_entry& lte2)
+	     { return lte1.pc < lte2.pc; });
 
   /* Allocate a new line table.  */
-  newLineTb = (struct linetable *)
-    xmalloc
-    (sizeof (struct linetable) +
-    (oldLineTb->nitems - function_count + extra_lines) * sizeof (struct linetable_entry));
+  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))));
 
   /* If line table does not start with a function beginning, copy up until
      a function begin.  */
 
-  newline = 0;
+  int newline = 0;
   if (oldLineTb->item[0].line != 0)
     for (newline = 0;
 	 newline < oldLineTb->nitems && oldLineTb->item[newline].line;
 	 ++newline)
-      newLineTb->item[newline] = oldLineTb->item[newline];
+      new_linetable->item[newline] = oldLineTb->item[newline];
 
   /* Now copy function lines one by one.  */
 
-  for (ii = 0; ii < function_count; ++ii)
+  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.  */
-      jj = fentry[ii].line;
+      int jj = entry.line;
       if (jj + 1 < oldLineTb->nitems
 	  && oldLineTb->item[jj].pc != oldLineTb->item[jj + 1].pc)
 	{
-	  newLineTb->item[newline] = oldLineTb->item[jj];
-	  newLineTb->item[newline].line = oldLineTb->item[jj + 1].line;
+	  new_linetable->item[newline] = oldLineTb->item[jj];
+	  new_linetable->item[newline].line = oldLineTb->item[jj + 1].line;
 	  newline++;
 	}
 
-      for (jj = fentry[ii].line + 1;
+      for (jj = entry.line + 1;
 	   jj < oldLineTb->nitems && oldLineTb->item[jj].line != 0;
 	   ++jj, ++newline)
-	newLineTb->item[newline] = oldLineTb->item[jj];
+	new_linetable->item[newline] = oldLineTb->item[jj];
     }
-  xfree (fentry);
+
   /* The number of items in the line table must include these
      extra lines which were added in case of XLC compiled functions.  */
-  newLineTb->nitems = oldLineTb->nitems - function_count + extra_lines;
-  return newLineTb;
+  new_linetable->nitems = new_linetable_nitems;
+  return new_linetable;
 }
 
 /* include file support: C_BINCL/C_EINCL pairs will be kept in the 
-- 
2.35.1

[PATCH 4/4] gdb: change subfile::line_vector to an std::vector

[Simon Marchi]

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
---
 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 034db5987356..4ec600a794a6 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 6284aafc878f..ee75e6fd95d3 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 b89e98ae05f4..566c0824cd5a 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 ());
 	}
     }
-- 
2.35.1

Re: [PATCH 1/4] gdb: allocate subfile with new

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Allocate struct subfile with new, initialize its fields instead of
Simon> memset-ing it to 0.  Use a unique_ptr for the window after a subfile has
Simon> been allocated but before it is linked in the buildsym_compunit's list
Simon> of subfile (and therefore owned by the buildsym_compunit.

Looks good to me.

Simon> I can't test the change in xcoffread.c, it's best-effort.  I couldn't
Simon> find where subfiles are freed in that file, I assume they were
Simon> intentionally (or not) leaked.

xcoffread uses buildsym-legacy and creates a scoped_free_pendings, which
I believe does the freeing.

Tom

Re: [PATCH 2/4] gdb: change subfile::name and buildsym_compunit::m_comp_dir to strings

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Change subfile::name to be a string, for easier memory management.
Simon> Change buildsym_compunit::m_comp_dir as well, since we move one in to
Simon> the other at some point in patch_subfile_names, so it's easier to do
Simon> both at the same time.  There are various NULL checks for both fields
Simon> currently, replace them with empty checks, I think it ends up
Simon> equivalent.

I think the basic change looks good, but I found one oddity:

Simon> -	subfile_name = concat (subfile_dirname, SLASH_STRING,
Simon> -			       subfile->name, (char *) NULL);
Simon> +	  && !m_comp_dir.empty ())
Simon> +	{
Simon> +	  subfile_name_holder = string_printf ("%s/%s", m_comp_dir.c_str (),
Simon> +					       subfile->name.c_str ());

This replaces a use of SLASH_STRING with "/".

Tom

Re: [PATCH 3/4] gdb: use std::vector for temporary linetable_entry array in arrange_linetable

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> From: Simon Marchi <simon.marchi@efficios.com>
Simon> Reduce manual memory management and make the code a bit easier to read.
Simon> This helps me a bit in the following patch.

Looks good.  Thank you.

Tom

Re: [PATCH 1/4] gdb: allocate subfile with new

[Simon Marchi]

On 2022-04-12 12:51, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> Allocate struct subfile with new, initialize its fields instead of
> Simon> memset-ing it to 0.  Use a unique_ptr for the window after a subfile has
> Simon> been allocated but before it is linked in the buildsym_compunit's list
> Simon> of subfile (and therefore owned by the buildsym_compunit.
> 
> Looks good to me.
> 
> Simon> I can't test the change in xcoffread.c, it's best-effort.  I couldn't
> Simon> find where subfiles are freed in that file, I assume they were
> Simon> intentionally (or not) leaked.
> 
> xcoffread uses buildsym-legacy and creates a scoped_free_pendings, which
> I believe does the freeing.
> 
> Tom

I don't know, it creates some subfiles and puts them in "struct _inclTable" objects,
but I don't see where / if those objects are ever linked in a buildsym_compunit
object.

Simon

Re: [PATCH 2/4] gdb: change subfile::name and buildsym_compunit::m_comp_dir to strings

[Simon Marchi]

On 2022-04-12 12:54, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> Change subfile::name to be a string, for easier memory management.
> Simon> Change buildsym_compunit::m_comp_dir as well, since we move one in to
> Simon> the other at some point in patch_subfile_names, so it's easier to do
> Simon> both at the same time.  There are various NULL checks for both fields
> Simon> currently, replace them with empty checks, I think it ends up
> Simon> equivalent.
> 
> I think the basic change looks good, but I found one oddity:
> 
> Simon> -	subfile_name = concat (subfile_dirname, SLASH_STRING,
> Simon> -			       subfile->name, (char *) NULL);
> Simon> +	  && !m_comp_dir.empty ())
> Simon> +	{
> Simon> +	  subfile_name_holder = string_printf ("%s/%s", m_comp_dir.c_str (),
> Simon> +					       subfile->name.c_str ());
> 
> This replaces a use of SLASH_STRING with "/".
> 
> Tom

I think that's ok, I don't really see the point of hiding the slash behind a
macro.  It's not like it changes on a per-host basis.

Simo

Re: [PATCH 4/4] gdb: change subfile::line_vector to an std::vector

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Change this field to an std::vector to facilitate memory management.
Simon> Since the linetable_entry array is copied into the symtab resulting from
Simon> the subfile, it is possible to change it without changing how symtab
Simon> stores the linetable entries (which would be a much larger change).

Thanks for doing this.

Simon> +	  size_t n_entries = subfile->line_vector_entries.size ();
Simon> +	  size_t entry_array_size = n_entries * sizeof (struct linetable_entry);
Simon> +	  int linetablesize = sizeof (struct linetable) + entry_array_size;

I wonder if this computation has been wrong for years... it seems to me
that entry_array_size should use (n_entries - 1), because struct
linetable uses an array of 1 item in its struct hack.  So, I think, this
is allocating one too many entries?

Other than that, this looks good to me.

Tom

Re: [PATCH 2/4] gdb: change subfile::name and buildsym_compunit::m_comp_dir to strings

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> I think that's ok, I don't really see the point of hiding the slash behind a
Simon> macro.  It's not like it changes on a per-host basis.

Oh, I was under the impression that it did, but I see that it does not.
Yeah, this is fine as-is.

Tom

Re: [PATCH 4/4] gdb: change subfile::line_vector to an std::vector

[Simon Marchi]

On 2022-04-12 13:34, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Simon> Change this field to an std::vector to facilitate memory management.
> Simon> Since the linetable_entry array is copied into the symtab resulting from
> Simon> the subfile, it is possible to change it without changing how symtab
> Simon> stores the linetable entries (which would be a much larger change).
>
> Thanks for doing this.
>
> Simon> +	  size_t n_entries = subfile->line_vector_entries.size ();
> Simon> +	  size_t entry_array_size = n_entries * sizeof (struct linetable_entry);
> Simon> +	  int linetablesize = sizeof (struct linetable) + entry_array_size;
>
> I wonder if this computation has been wrong for years... it seems to me
> that entry_array_size should use (n_entries - 1), because struct
> linetable uses an array of 1 item in its struct hack.  So, I think, this
> is allocating one too many entries?

Yeah, I noticed that too, but I didn't point it out because it doesn't
really matter.  If we want to fix it, I'd change the struct to use:

  struct linetable_entry item[];

>
> Other than that, this looks good to me.

Thanks, I will push the series.

Simon