[PATCH 0/8] Remove obstack allocation of hash tables

[PATCH 0/8] Remove obstack allocation of hash tables

[Tom Tromey]

Obstack allocation of hash tables has slightly irritated me for a long
time.  The main issue with these is that if the hash table is resized,
then the old table is not freed, but instead left around as unused
garbage.

This series removes this capability entirely.  IMO there's no reason
to ever do this -- with C++, heap allocation is just as convenient,
and doesn't cause effective memory leaks.

While working on this I found a few other minor cleanups to do.  This
series includes them all.

Regression tested on x86-64 Fedora 38.

---
Tom Tromey (8):
      Don't obstack-allocate the DIE hash
      Don't obstack-allocate the CU dependency hash table
      Rename symtab::fullname
      Make symtab members private
      Add compunit_symtab::forget_cached_source_info
      Don't obstack-allocate the call site hash table
      Remove hashtab_obstack_allocate
      Prefer htab_traverse_noresize

 gdb/annotate.c             |  2 +-
 gdb/completer.c            |  2 +-
 gdb/dwarf2/cu.c            | 13 +++++-----
 gdb/dwarf2/cu.h            |  6 ++---
 gdb/dwarf2/read.c          | 63 ++++++++++++++++------------------------------
 gdb/gdb_bfd.c              |  2 +-
 gdb/objfiles.c             |  2 ++
 gdb/python/py-breakpoint.c |  4 +--
 gdb/source.c               | 21 ++++++++--------
 gdb/symfile-debug.c        | 11 +-------
 gdb/symfile.c              |  1 -
 gdb/symmisc.c              |  4 +--
 gdb/symtab.c               | 23 +++++++++++++++--
 gdb/symtab.h               | 54 +++++++++++++++++++++++++++++++--------
 gdbsupport/Makefile.am     |  1 -
 gdbsupport/Makefile.in     |  5 +---
 gdbsupport/gdb-hashtab.cc  | 43 -------------------------------
 gdbsupport/gdb-hashtab.h   |  5 ----
 18 files changed, 116 insertions(+), 146 deletions(-)
---
base-commit: ecc5fed79101ba6af6ddc6e629e2d8d93c36065b
change-id: 20240605-hash-tab-no-obstack-91d863a9add4

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH 1/8] Don't obstack-allocate the DIE hash

[Tom Tromey]

The DIE hash table is currently allocated on an obstack.  There's no
need to do this, and I think it's better to simply heap-allocate the
hash table.

This patch implements this.  I also removed store_in_ref_table as
well, inlining it into its sole caller, as I think this is clearer.
---
 gdb/dwarf2/cu.h   |  2 +-
 gdb/dwarf2/read.c | 50 ++++++++++++++++----------------------------------
 2 files changed, 17 insertions(+), 35 deletions(-)

diff --git a/gdb/dwarf2/cu.h b/gdb/dwarf2/cu.h
index 58e89960aad..94e73eb3813 100644
--- a/gdb/dwarf2/cu.h
+++ b/gdb/dwarf2/cu.h
@@ -151,7 +151,7 @@ struct dwarf2_cu
 
   /* A hash table of DIE cu_offset for following references with
      die_info->offset.sect_off as hash.  */
-  htab_t die_hash = nullptr;
+  htab_up die_hash;
 
   /* Full DIEs if read in.  */
   struct die_info *dies = nullptr;
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 6a19064409c..5367e68326f 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -995,9 +995,6 @@ static const char *dwarf2_physname (const char *name, struct die_info *die,
 static struct die_info *dwarf2_extension (struct die_info *die,
 					  struct dwarf2_cu **);
 
-static void store_in_ref_table (struct die_info *,
-				struct dwarf2_cu *);
-
 static struct die_info *follow_die_ref_or_sig (struct die_info *,
 					       const struct attribute *,
 					       struct dwarf2_cu **);
@@ -5492,14 +5489,10 @@ load_full_comp_unit (dwarf2_per_cu_data *this_cu,
   const gdb_byte *info_ptr = reader.info_ptr;
 
   gdb_assert (cu->die_hash == NULL);
-  cu->die_hash =
-    htab_create_alloc_ex (cu->header.get_length_without_initial () / 12,
-			  die_info::hash,
-			  die_info::eq,
-			  NULL,
-			  &cu->comp_unit_obstack,
-			  hashtab_obstack_allocate,
-			  dummy_obstack_deallocate);
+  cu->die_hash.reset (htab_create_alloc
+		      (cu->header.get_length_without_initial () / 12,
+		       die_info::hash, die_info::eq,
+		       nullptr, xcalloc, xfree));
 
   if (reader.comp_unit_die->has_children)
     reader.comp_unit_die->child
@@ -15765,7 +15758,11 @@ read_die_and_children (const struct die_reader_specs *reader,
       *new_info_ptr = cur_ptr;
       return NULL;
     }
-  store_in_ref_table (die, reader->cu);
+
+  void **slot = htab_find_slot_with_hash (reader->cu->die_hash.get (), die,
+					  to_underlying (die->sect_off),
+					  INSERT);
+  *slot = die;
 
   if (die->has_children)
     die->child = read_die_and_siblings_1 (reader, cur_ptr, new_info_ptr, die);
@@ -20242,18 +20239,6 @@ dwarf2_extension (struct die_info *die, struct dwarf2_cu **ext_cu)
   return follow_die_ref (die, attr, ext_cu);
 }
 
-static void
-store_in_ref_table (struct die_info *die, struct dwarf2_cu *cu)
-{
-  void **slot;
-
-  slot = htab_find_slot_with_hash (cu->die_hash, die,
-				   to_underlying (die->sect_off),
-				   INSERT);
-
-  *slot = die;
-}
-
 /* Follow reference or signature attribute ATTR of SRC_DIE.
    On entry *REF_CU is the CU of SRC_DIE.
    On exit *REF_CU is the CU of the result.  */
@@ -20345,7 +20330,7 @@ follow_die_offset (sect_offset sect_off, int offset_in_dwz,
   *ref_cu = target_cu;
   temp_die.sect_off = sect_off;
 
-  return (struct die_info *) htab_find_with_hash (target_cu->die_hash,
+  return (struct die_info *) htab_find_with_hash (target_cu->die_hash.get (),
 						  &temp_die,
 						  to_underlying (sect_off));
 }
@@ -20725,7 +20710,8 @@ follow_die_sig_1 (struct die_info *src_die, struct signatured_type *sig_type,
   gdb_assert (sig_cu != NULL);
   gdb_assert (to_underlying (sig_type->type_offset_in_section) != 0);
   temp_die.sect_off = sig_type->type_offset_in_section;
-  die = (struct die_info *) htab_find_with_hash (sig_cu->die_hash, &temp_die,
+  die = (struct die_info *) htab_find_with_hash (sig_cu->die_hash.get (),
+						 &temp_die,
 						 to_underlying (temp_die.sect_off));
   if (die)
     {
@@ -20913,14 +20899,10 @@ read_signatured_type (signatured_type *sig_type,
       const gdb_byte *info_ptr = reader.info_ptr;
 
       gdb_assert (cu->die_hash == NULL);
-      cu->die_hash =
-	htab_create_alloc_ex (cu->header.get_length_without_initial () / 12,
-			      die_info::hash,
-			      die_info::eq,
-			      NULL,
-			      &cu->comp_unit_obstack,
-			      hashtab_obstack_allocate,
-			      dummy_obstack_deallocate);
+      cu->die_hash.reset (htab_create_alloc
+			  (cu->header.get_length_without_initial () / 12,
+			   die_info::hash, die_info::eq,
+			   nullptr, xcalloc, xfree));
 
       if (reader.comp_unit_die->has_children)
 	reader.comp_unit_die->child

-- 
2.44.0

[PATCH 2/8] Don't obstack-allocate the CU dependency hash table

[Tom Tromey]

The CU dependency hash table is obstack-allocated, but there's no need
to do this.
---
 gdb/dwarf2/cu.c | 12 +++++-------
 gdb/dwarf2/cu.h |  2 +-
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/gdb/dwarf2/cu.c b/gdb/dwarf2/cu.c
index a64cb1d8380..aae61cccd21 100644
--- a/gdb/dwarf2/cu.c
+++ b/gdb/dwarf2/cu.c
@@ -146,7 +146,7 @@ dwarf2_cu::mark ()
     {
       m_mark = true;
       if (m_dependencies != nullptr)
-	htab_traverse (m_dependencies, dwarf2_mark_helper, per_objfile);
+	htab_traverse (m_dependencies.get (), dwarf2_mark_helper, per_objfile);
     }
 }
 
@@ -158,13 +158,11 @@ dwarf2_cu::add_dependence (struct dwarf2_per_cu_data *ref_per_cu)
   void **slot;
 
   if (m_dependencies == nullptr)
-    m_dependencies
-      = htab_create_alloc_ex (5, htab_hash_pointer, htab_eq_pointer,
-			      NULL, &comp_unit_obstack,
-			      hashtab_obstack_allocate,
-			      dummy_obstack_deallocate);
+    m_dependencies.reset (htab_create_alloc
+			  (5, htab_hash_pointer, htab_eq_pointer,
+			   nullptr, xcalloc, xfree));
 
-  slot = htab_find_slot (m_dependencies, ref_per_cu, INSERT);
+  slot = htab_find_slot (m_dependencies.get (), ref_per_cu, INSERT);
   if (*slot == nullptr)
     *slot = ref_per_cu;
 }
diff --git a/gdb/dwarf2/cu.h b/gdb/dwarf2/cu.h
index 94e73eb3813..d23cc9b70e5 100644
--- a/gdb/dwarf2/cu.h
+++ b/gdb/dwarf2/cu.h
@@ -122,7 +122,7 @@ struct dwarf2_cu
   /* A set of pointers to dwarf2_per_cu_data objects for compilation
      units referenced by this one.  Only set during full symbol processing;
      partial symbol tables do not have dependencies.  */
-  htab_t m_dependencies = nullptr;
+  htab_up m_dependencies;
 
 public:
   /* The generic symbol table building routines have separate lists for

-- 
2.44.0

[PATCH 3/8] Rename symtab::fullname

[Tom Tromey]

This renames symtab::fullname to m_fullname and adds new accessor
methods.
---
 gdb/annotate.c             |  2 +-
 gdb/python/py-breakpoint.c |  4 ++--
 gdb/source.c               | 21 ++++++++++-----------
 gdb/symfile-debug.c        |  8 +-------
 gdb/symfile.c              |  1 -
 gdb/symmisc.c              |  4 ++--
 gdb/symtab.h               | 23 ++++++++++++++++++++++-
 7 files changed, 38 insertions(+), 25 deletions(-)

diff --git a/gdb/annotate.c b/gdb/annotate.c
index 4ff3eb89807..88180295d1d 100644
--- a/gdb/annotate.c
+++ b/gdb/annotate.c
@@ -451,7 +451,7 @@ annotate_source_line (struct symtab *s, int line, int mid_statement,
       if (line > offsets->size ())
 	return false;
 
-      annotate_source (s->fullname, line, (int) (*offsets)[line - 1],
+      annotate_source (s->fullname (), line, (int) (*offsets)[line - 1],
 		       mid_statement, s->compunit ()->objfile ()->arch (),
 		       pc);
 
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index da74d69d357..c178199d6d0 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -1716,10 +1716,10 @@ bplocpy_get_fullname (PyObject *py_self, void *closure)
   BPPY_REQUIRE_VALID (self->owner);
   BPLOCPY_REQUIRE_VALID (self->owner, self);
   const auto symtab = self->bp_loc->symtab;
-  if (symtab != nullptr && symtab->fullname != nullptr)
+  if (symtab != nullptr && symtab->fullname () != nullptr)
     {
       gdbpy_ref<> fullname
-	= host_string_to_python_string (symtab->fullname);
+	= host_string_to_python_string (symtab->fullname ());
       return fullname.release ();
     }
   Py_RETURN_NONE;
diff --git a/gdb/source.c b/gdb/source.c
index 24a8769da91..9e528d3f705 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -685,8 +685,8 @@ info_source_command (const char *ignore, int from_tty)
   gdb_printf (_("Current source file is %s\n"), s->filename);
   if (s->compunit ()->dirname () != NULL)
     gdb_printf (_("Compilation directory is %s\n"), s->compunit ()->dirname ());
-  if (s->fullname)
-    gdb_printf (_("Located in %s\n"), s->fullname);
+  if (s->fullname () != nullptr)
+    gdb_printf (_("Located in %s\n"), s->fullname ());
   const std::vector<off_t> *offsets;
   if (g_source_cache.get_line_charpos (s, &offsets))
     gdb_printf (_("Contains %d line%s.\n"), (int) offsets->size (),
@@ -1145,8 +1145,7 @@ open_source_file (struct symtab *s)
   if (!s)
     return scoped_fd (-EINVAL);
 
-  gdb::unique_xmalloc_ptr<char> fullname (s->fullname);
-  s->fullname = NULL;
+  gdb::unique_xmalloc_ptr<char> fullname = s->release_fullname ();
   scoped_fd fd = find_and_open_source (s->filename, s->compunit ()->dirname (),
 				       &fullname);
 
@@ -1182,14 +1181,14 @@ open_source_file (struct symtab *s)
 		 It handles the reporting of its own errors.  */
 	      if (query_fd.get () >= 0)
 		{
-		  s->fullname = fullname.release ();
+		  s->set_fullname (std::move (fullname));
 		  return query_fd;
 		}
 	    }
 	}
     }
 
-  s->fullname = fullname.release ();
+  s->set_fullname (std::move (fullname));
   return fd;
 }
 
@@ -1236,7 +1235,7 @@ symtab_to_fullname (struct symtab *s)
   /* Use cached copy if we have it.
      We rely on forget_cached_source_info being called appropriately
      to handle cases like the file being moved.  */
-  if (s->fullname == NULL)
+  if (s->fullname () == nullptr)
     {
       scoped_fd fd = open_source_file (s);
 
@@ -1254,13 +1253,13 @@ symtab_to_fullname (struct symtab *s)
 	    fullname.reset (concat (s->compunit ()->dirname (), SLASH_STRING,
 				    s->filename, (char *) NULL));
 
-	  s->fullname = rewrite_source_path (fullname.get ()).release ();
-	  if (s->fullname == NULL)
-	    s->fullname = fullname.release ();
+	  s->set_fullname (rewrite_source_path (fullname.get ()));
+	  if (s->fullname () == nullptr)
+	    s->set_fullname (std::move (fullname));
 	}
     } 
 
-  return s->fullname;
+  return s->fullname ();
 }
 
 /* See commentary in source.h.  */
diff --git a/gdb/symfile-debug.c b/gdb/symfile-debug.c
index 86c7010b2d2..39128894a5c 100644
--- a/gdb/symfile-debug.c
+++ b/gdb/symfile-debug.c
@@ -156,13 +156,7 @@ objfile::forget_cached_source_info ()
   for (compunit_symtab *cu : compunits ())
     {
       for (symtab *s : cu->filetabs ())
-	{
-	  if (s->fullname != NULL)
-	    {
-	      xfree (s->fullname);
-	      s->fullname = NULL;
-	    }
-	}
+	s->release_fullname ();
     }
 
   for (const auto &iter : qf)
diff --git a/gdb/symfile.c b/gdb/symfile.c
index f7f5be5a39a..e4563f09c23 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2814,7 +2814,6 @@ allocate_symtab (struct compunit_symtab *cust, const char *filename,
 
   symtab->filename = objfile->intern (filename);
   symtab->filename_for_id = objfile->intern (filename_for_id);
-  symtab->fullname = NULL;
   symtab->set_language (deduce_language_from_filename (filename));
 
   /* This can be very verbose with lots of headers.
diff --git a/gdb/symmisc.c b/gdb/symmisc.c
index 2956ad92fce..b4e0360041e 100644
--- a/gdb/symmisc.c
+++ b/gdb/symmisc.c
@@ -811,8 +811,8 @@ maintenance_info_symtabs (const char *regexp, int from_tty)
 		    gdb_printf ("((struct symtab *) %s)\n",
 				host_address_to_string (symtab));
 		    gdb_printf ("\t  fullname %s\n",
-				symtab->fullname != NULL
-				? symtab->fullname
+				symtab->fullname () != nullptr
+				? symtab->fullname ()
 				: "(null)");
 		    gdb_printf ("\t  "
 				"linetable ((struct linetable *) %s)\n",
diff --git a/gdb/symtab.h b/gdb/symtab.h
index bf9a3cfb79f..49c1eb4889d 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1735,6 +1735,27 @@ struct symtab
     m_language = language;
   }
 
+  /* Return the current full name of this symtab.  */
+  const char *fullname () const
+  { return m_fullname; }
+
+  /* Transfer ownership of the current full name to the caller.  The
+     full name is reset to nullptr.  */
+  gdb::unique_xmalloc_ptr<char> release_fullname ()
+  {
+    gdb::unique_xmalloc_ptr<char> result (m_fullname);
+    m_fullname = nullptr;
+    return result;
+  }
+
+  /* Set the current full name to NAME, transferring ownership to this
+     symtab.  */
+  void set_fullname (gdb::unique_xmalloc_ptr<char> name)
+  {
+    gdb_assert (m_fullname == nullptr);
+    m_fullname = name.release ();
+  }
+
   /* Unordered chain of all filetabs in the compunit,  with the exception
      that the "main" source file is the first entry in the list.  */
 
@@ -1772,7 +1793,7 @@ struct symtab
   /* Full name of file as found by searching the source path.
      NULL if not yet known.  */
 
-  char *fullname;
+  char *m_fullname;
 };
 
 /* A range adapter to allowing iterating over all the file tables in a list.  */

-- 
2.44.0

[PATCH 4/8] Make symtab members private

[Tom Tromey]

This rearranges symtab so that the private members appear at the end,
and then adds the "private" keyword.
---
 gdb/symtab.h | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/gdb/symtab.h b/gdb/symtab.h
index 49c1eb4889d..59c0b7d5735 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1761,15 +1761,6 @@ struct symtab
 
   struct symtab *next;
 
-  /* Backlink to containing compunit symtab.  */
-
-  struct compunit_symtab *m_compunit;
-
-  /* Table mapping core addresses to line numbers for this file.
-     Can be NULL if none.  Never shared between different symtabs.  */
-
-  const struct linetable *m_linetable;
-
   /* Name of this source file, in a form appropriate to print to the user.
 
      This pointer is never nullptr.  */
@@ -1786,6 +1777,17 @@ struct symtab
      This pointer is never nullptr.*/
   const char *filename_for_id;
 
+private:
+
+  /* Backlink to containing compunit symtab.  */
+
+  struct compunit_symtab *m_compunit;
+
+  /* Table mapping core addresses to line numbers for this file.
+     Can be NULL if none.  Never shared between different symtabs.  */
+
+  const struct linetable *m_linetable;
+
   /* Language of this source file.  */
 
   enum language m_language;

-- 
2.44.0

[PATCH 5/8] Add compunit_symtab::forget_cached_source_info

[Tom Tromey]

It seemed cleaner to me for compunit_symtab to have a
forget_cached_source_info method, then for the objfile to know how to
do this.
---
 gdb/symfile-debug.c | 5 +----
 gdb/symtab.c        | 9 +++++++++
 gdb/symtab.h        | 3 +++
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/gdb/symfile-debug.c b/gdb/symfile-debug.c
index 39128894a5c..3a223d0789e 100644
--- a/gdb/symfile-debug.c
+++ b/gdb/symfile-debug.c
@@ -154,10 +154,7 @@ objfile::forget_cached_source_info ()
 		objfile_debug_name (this));
 
   for (compunit_symtab *cu : compunits ())
-    {
-      for (symtab *s : cu->filetabs ())
-	s->release_fullname ();
-    }
+    cu->forget_cached_source_info ();
 
   for (const auto &iter : qf)
     iter->forget_cached_source_info (this);
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 0464b6d1479..c202ec9bcff 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -472,6 +472,15 @@ compunit_symtab::language () const
   return symtab->language ();
 }
 
+/* See symtab.h.  */
+
+void
+compunit_symtab::forget_cached_source_info ()
+{
+  for (symtab *s : filetabs ())
+    s->release_fullname ();
+}
+
 /* The relocated address of the minimal symbol, using the section
    offsets from OBJFILE.  */
 
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 59c0b7d5735..fc99c88a6ea 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1961,6 +1961,9 @@ struct compunit_symtab
   /* Return the language of this compunit_symtab.  */
   enum language language () const;
 
+  /* Clear any cached source file names.  */
+  void forget_cached_source_info ();
+
   /* Unordered chain of all compunit symtabs of this objfile.  */
   struct compunit_symtab *next;
 

-- 
2.44.0

[PATCH 6/8] Don't obstack-allocate the call site hash table

[Tom Tromey]

The call site hash table is the last hash table using obstack
allocation.  In one large (non-public) test case, these hash tables
take a substiantial amount of memory.  Some of this memory is wasted
-- whenever the hash table is resized, the old table is not freed.

This patch fixes the problem by changing this hash table to be
heap-allocated.  This means that resizing will no longer "leak"
memory.
---
 gdb/dwarf2/cu.h   |  2 +-
 gdb/dwarf2/read.c | 13 ++++++-------
 gdb/objfiles.c    |  2 ++
 gdb/symtab.c      | 14 ++++++++++++--
 gdb/symtab.h      |  8 +++++++-
 5 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/gdb/dwarf2/cu.h b/gdb/dwarf2/cu.h
index d23cc9b70e5..b0ec2d6fabc 100644
--- a/gdb/dwarf2/cu.h
+++ b/gdb/dwarf2/cu.h
@@ -170,7 +170,7 @@ struct dwarf2_cu
   std::vector<delayed_method_info> method_list;
 
   /* To be copied to symtab->call_site_htab.  */
-  htab_t call_site_htab = nullptr;
+  htab_up call_site_htab;
 
   /* Non-NULL if this CU came from a DWO file.
      There is an invariant here that is important to remember:
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 5367e68326f..37045b87138 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -6233,7 +6233,7 @@ process_full_comp_unit (dwarf2_cu *cu, enum language pretend_language)
       else
 	cust->set_epilogue_unwind_valid (true);
 
-      cust->set_call_site_htab (cu->call_site_htab);
+      cust->set_call_site_htab (std::move (cu->call_site_htab));
     }
 
   per_objfile->set_symtab (cu->per_cu, cust);
@@ -10208,13 +10208,12 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
     }
   unrelocated_addr pc = attr->as_address ();
 
-  if (cu->call_site_htab == NULL)
-    cu->call_site_htab = htab_create_alloc_ex (16, call_site::hash,
-					       call_site::eq, NULL,
-					       &objfile->objfile_obstack,
-					       hashtab_obstack_allocate, NULL);
+  if (cu->call_site_htab == nullptr)
+    cu->call_site_htab.reset (htab_create_alloc (16, call_site::hash,
+						 call_site::eq, nullptr,
+						 xcalloc, xfree));
   struct call_site call_site_local (pc, nullptr, nullptr);
-  slot = htab_find_slot (cu->call_site_htab, &call_site_local, INSERT);
+  slot = htab_find_slot (cu->call_site_htab.get (), &call_site_local, INSERT);
   if (*slot != NULL)
     {
       complaint (_("Duplicate PC %s for DW_TAG_call_site "
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index e6b0ca7f29b..368e129b49d 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -530,6 +530,8 @@ objfile::~objfile ()
   /* It still may reference data modules have associated with the objfile and
      the symbol file data.  */
   forget_cached_source_info ();
+  for (compunit_symtab *cu : compunits ())
+    cu->finalize ();
 
   breakpoint_free_objfile (this);
   btrace_free_objfile (this);
diff --git a/gdb/symtab.c b/gdb/symtab.c
index c202ec9bcff..3088e4b5d10 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -415,10 +415,10 @@ compunit_symtab::find_call_site (CORE_ADDR pc) const
 /* See symtab.h.  */
 
 void
-compunit_symtab::set_call_site_htab (htab_t call_site_htab)
+compunit_symtab::set_call_site_htab (htab_up call_site_htab)
 {
   gdb_assert (m_call_site_htab == nullptr);
-  m_call_site_htab = call_site_htab;
+  m_call_site_htab = call_site_htab.release ();
 }
 
 /* See symtab.h.  */
@@ -481,6 +481,16 @@ compunit_symtab::forget_cached_source_info ()
     s->release_fullname ();
 }
 
+/* See symtab.h.  */
+
+void
+compunit_symtab::finalize ()
+{
+  this->forget_cached_source_info ();
+  if (m_call_site_htab != nullptr)
+    htab_delete (m_call_site_htab);
+}
+
 /* The relocated address of the minimal symbol, using the section
    offsets from OBJFILE.  */
 
diff --git a/gdb/symtab.h b/gdb/symtab.h
index fc99c88a6ea..4477bad5e81 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1953,7 +1953,7 @@ struct compunit_symtab
   symtab *primary_filetab () const;
 
   /* Set m_call_site_htab.  */
-  void set_call_site_htab (htab_t call_site_htab);
+  void set_call_site_htab (htab_up call_site_htab);
 
   /* Find call_site info for PC.  */
   call_site *find_call_site (CORE_ADDR pc) const;
@@ -1964,6 +1964,12 @@ struct compunit_symtab
   /* Clear any cached source file names.  */
   void forget_cached_source_info ();
 
+  /* This is called when an objfile is being destroyed and will free
+     any resources used by this compunit_symtab.  Normally a
+     destructor would be used instead, but at the moment
+     compunit_symtab objects are allocated on an obstack.  */
+  void finalize ();
+
   /* Unordered chain of all compunit symtabs of this objfile.  */
   struct compunit_symtab *next;
 

-- 
2.44.0

[PATCH 7/8] Remove hashtab_obstack_allocate

[Tom Tromey]

I think that hashtabs should never be obstack-allocated.  In the past
this was convenient sometimes, because any new data structure needed a
corresponding cleanup.  However, with the switch to C++, resource
management has become much simpler; for example, a local variable can
simply be of type htab_up rather than hashtab_t, and the problem is
solved.

This patch removes hashtab_obstack_allocate to try to prevent this
anti-pattern from being used again.
---
 gdbsupport/Makefile.am    |  1 -
 gdbsupport/Makefile.in    |  5 +----
 gdbsupport/gdb-hashtab.cc | 43 -------------------------------------------
 gdbsupport/gdb-hashtab.h  |  5 -----
 4 files changed, 1 insertion(+), 53 deletions(-)

diff --git a/gdbsupport/Makefile.am b/gdbsupport/Makefile.am
index 2b0f987125c..36e561e015d 100644
--- a/gdbsupport/Makefile.am
+++ b/gdbsupport/Makefile.am
@@ -68,7 +68,6 @@ libgdbsupport_a_SOURCES = \
     filestuff.cc \
     format.cc \
     gdb-dlfcn.cc \
-    gdb-hashtab.cc \
     gdb_obstack.cc \
     gdb_regex.cc \
     gdb_tilde_expand.cc \
diff --git a/gdbsupport/Makefile.in b/gdbsupport/Makefile.in
index ee709112aae..9cff86bd987 100644
--- a/gdbsupport/Makefile.in
+++ b/gdbsupport/Makefile.in
@@ -159,8 +159,7 @@ am_libgdbsupport_a_OBJECTS = agent.$(OBJEXT) btrace-common.$(OBJEXT) \
 	common-regcache.$(OBJEXT) common-utils.$(OBJEXT) \
 	environ.$(OBJEXT) errors.$(OBJEXT) event-loop.$(OBJEXT) \
 	fileio.$(OBJEXT) filestuff.$(OBJEXT) format.$(OBJEXT) \
-	gdb-dlfcn.$(OBJEXT) gdb-hashtab.$(OBJEXT) \
-	gdb_obstack.$(OBJEXT) gdb_regex.$(OBJEXT) \
+	gdb-dlfcn.$(OBJEXT) gdb_obstack.$(OBJEXT) gdb_regex.$(OBJEXT) \
 	gdb_tilde_expand.$(OBJEXT) gdb_wait.$(OBJEXT) \
 	gdb_vecs.$(OBJEXT) job-control.$(OBJEXT) netstuff.$(OBJEXT) \
 	new-op.$(OBJEXT) pathstuff.$(OBJEXT) print-utils.$(OBJEXT) \
@@ -426,7 +425,6 @@ libgdbsupport_a_SOURCES = \
     filestuff.cc \
     format.cc \
     gdb-dlfcn.cc \
-    gdb-hashtab.cc \
     gdb_obstack.cc \
     gdb_regex.cc \
     gdb_tilde_expand.cc \
@@ -536,7 +534,6 @@ distclean-compile:
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/filestuff.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/format.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/gdb-dlfcn.Po@am__quote@
-@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/gdb-hashtab.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/gdb_obstack.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/gdb_regex.Po@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/gdb_tilde_expand.Po@am__quote@
diff --git a/gdbsupport/gdb-hashtab.cc b/gdbsupport/gdb-hashtab.cc
deleted file mode 100644
index 42f80faa403..00000000000
--- a/gdbsupport/gdb-hashtab.cc
+++ /dev/null
@@ -1,43 +0,0 @@
-/* Hash table wrappers for gdb.
-   Copyright (C) 2021-2024 Free Software Foundation, Inc.
-
-   This file is part of GDB.
-
-   This program is free software; you can redistribute it and/or modify
-   it under the terms of the GNU General Public License as published by
-   the Free Software Foundation; either version 3 of the License, or
-   (at your option) any later version.
-
-   This program is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-   GNU General Public License for more details.
-
-   You should have received a copy of the GNU General Public License
-   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
-
-#include "gdb-hashtab.h"
-
-/* Allocation function for the libiberty hash table which uses an
-   obstack.  The obstack is passed as DATA.  */
-
-void *
-hashtab_obstack_allocate (void *data, size_t size, size_t count)
-{
-  size_t total = size * count;
-  void *ptr = obstack_alloc ((struct obstack *) data, total);
-
-  memset (ptr, 0, total);
-  return ptr;
-}
-
-/* Trivial deallocation function for the libiberty splay tree and hash
-   table - don't deallocate anything.  Rely on later deletion of the
-   obstack.  DATA will be the obstack, although it is not needed
-   here.  */
-
-void
-dummy_obstack_deallocate (void *object, void *data)
-{
-  return;
-}
diff --git a/gdbsupport/gdb-hashtab.h b/gdbsupport/gdb-hashtab.h
index 0cdcc840425..05465f9a81f 100644
--- a/gdbsupport/gdb-hashtab.h
+++ b/gdbsupport/gdb-hashtab.h
@@ -42,9 +42,4 @@ htab_delete_entry (void *ptr)
   delete (T *) ptr;
 }
 
-/* Allocation and deallocation functions for the libiberty hash table
-   which use obstacks.  */
-void *hashtab_obstack_allocate (void *data, size_t size, size_t count);
-void dummy_obstack_deallocate (void *object, void *data);
-
 #endif /* GDBSUPPORT_GDB_HASHTAB_H */

-- 
2.44.0

[PATCH 8/8] Prefer htab_traverse_noresize

[Tom Tromey]

A few spots in gdb were using htab_traverse.  IMO this is almost never
useful and htab_traverse_noresize should be preferred.
---
 gdb/completer.c | 2 +-
 gdb/dwarf2/cu.c | 3 ++-
 gdb/gdb_bfd.c   | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index f1f44109bdc..bd5118f53c5 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -2058,7 +2058,7 @@ completion_tracker::recompute_lowest_common_denominator ()
 	return 1;
       };
 
-  htab_traverse (m_entries_hash.get (), visitor_func, this);
+  htab_traverse_noresize (m_entries_hash.get (), visitor_func, this);
   m_lowest_common_denominator_valid = true;
 }
 
diff --git a/gdb/dwarf2/cu.c b/gdb/dwarf2/cu.c
index aae61cccd21..5cb22919c32 100644
--- a/gdb/dwarf2/cu.c
+++ b/gdb/dwarf2/cu.c
@@ -146,7 +146,8 @@ dwarf2_cu::mark ()
     {
       m_mark = true;
       if (m_dependencies != nullptr)
-	htab_traverse (m_dependencies.get (), dwarf2_mark_helper, per_objfile);
+	htab_traverse_noresize (m_dependencies.get (), dwarf2_mark_helper,
+				per_objfile);
     }
 }
 
diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index cb9a91d0923..7e272c719c3 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -1200,7 +1200,7 @@ maintenance_info_bfds (const char *arg, int from_tty)
   uiout->table_header (40, ui_left, "filename", "Filename");
 
   uiout->table_body ();
-  htab_traverse (all_bfds, print_one_bfd, uiout);
+  htab_traverse_noresize (all_bfds, print_one_bfd, uiout);
 }
 
 /* BFD related per-inferior data.  */

-- 
2.44.0

Re: [PATCH 0/8] Remove obstack allocation of hash tables

[Keith Seitz]

On 6/5/24 10:19 AM, Tom Tromey wrote:
> Obstack allocation of hash tables has slightly irritated me for a long
> time.  The main issue with these is that if the hash table is resized,
> then the old table is not freed, but instead left around as unused
> garbage.
> 
> This series removes this capability entirely.  IMO there's no reason
> to ever do this -- with C++, heap allocation is just as convenient,
> and doesn't cause effective memory leaks.

I am biased -- I've always had a fair amount of disdain for obstacks --
so I really am pleased to see this series.

Having looked through it, and having <cough>refreshed my memory on a
few things</cough>, this series LGTM. I encourage you to commit it.

I've also regression tested it through Fedora 40 (GCC 14.1.1), just
make sure that the newer compiler didn't find something to complain
about (which IME it often does). It didn't in this case. :-)

Keith

Re: [PATCH 0/8] Remove obstack allocation of hash tables

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> I am biased -- I've always had a fair amount of disdain for obstacks --
Keith> so I really am pleased to see this series.

Yeah.  I have mixed feelings about them, for PODs they seem ok-ish; but
for hash tables I never liked them.

Keith> Having looked through it, and having <cough>refreshed my memory on a
Keith> few things</cough>, this series LGTM. I encourage you to commit it.

Thanks, I'm checking it in.

Tom