[PATCH 0/9] Use htab_up in more places

[PATCH 0/9] Use htab_up in more places

[Tom Tromey]

This series changes various spots in gdb to use htab_up rather than
explicitly managing the hash table.  Not every spot can be changed,
but many can; and in several cases this results in a code
simplification.

Tom

[PATCH 1/9] Use htab_up in auto-load.c

[Tom Tromey]

This changes auto-load.c to use htab_up, rather than manually calling
htab_delete.

gdb/ChangeLog
2020-07-18  Tom Tromey  <tom@tromey.com>

	* auto-load.c (struct auto_load_pspace_info)
	<~auto_load_pspace_info>: Default.
	<loaded_script_files, loaded_script_texts>: Change type to
	htab_up.
	(~auto_load_pspace_info) Remove.
	(init_loaded_scripts_info, maybe_add_script_file)
	(maybe_add_script_text, auto_load_info_scripts): Update.
---
 gdb/ChangeLog   | 10 ++++++++++
 gdb/auto-load.c | 40 +++++++++++++++++-----------------------
 2 files changed, 27 insertions(+), 23 deletions(-)

diff --git a/gdb/auto-load.c b/gdb/auto-load.c
index c967261925a..e21788f481f 100644
--- a/gdb/auto-load.c
+++ b/gdb/auto-load.c
@@ -530,12 +530,12 @@ For more information about this security protection see the\n\
 struct auto_load_pspace_info
 {
   auto_load_pspace_info () = default;
-  ~auto_load_pspace_info ();
+  ~auto_load_pspace_info () = default;
 
   /* For each program space we keep track of loaded scripts, both when
      specified as file names and as scripts to be executed directly.  */
-  struct htab *loaded_script_files = nullptr;
-  struct htab *loaded_script_texts = nullptr;
+  htab_up loaded_script_files;
+  htab_up loaded_script_texts;
 
   /* Non-zero if we've issued the warning about an auto-load script not being
      supported.  We only want to issue this warning once.  */
@@ -567,14 +567,6 @@ struct loaded_script
 static const struct program_space_key<struct auto_load_pspace_info>
   auto_load_pspace_data;
 
-auto_load_pspace_info::~auto_load_pspace_info ()
-{
-  if (loaded_script_files)
-    htab_delete (loaded_script_files);
-  if (loaded_script_texts)
-    htab_delete (loaded_script_texts);
-}
-
 /* Get the current autoload data.  If none is found yet, add it now.  This
    function always returns a valid object.  */
 
@@ -621,14 +613,16 @@ init_loaded_scripts_info (struct auto_load_pspace_info *pspace_info)
      Space for each entry is obtained with one malloc so we can free them
      easily.  */
 
-  pspace_info->loaded_script_files = htab_create (31,
-						  hash_loaded_script_entry,
-						  eq_loaded_script_entry,
-						  xfree);
-  pspace_info->loaded_script_texts = htab_create (31,
-						  hash_loaded_script_entry,
-						  eq_loaded_script_entry,
-						  xfree);
+  pspace_info->loaded_script_files.reset
+    (htab_create (31,
+		  hash_loaded_script_entry,
+		  eq_loaded_script_entry,
+		  xfree));
+  pspace_info->loaded_script_texts.reset
+    (htab_create (31,
+		  hash_loaded_script_entry,
+		  eq_loaded_script_entry,
+		  xfree));
 
   pspace_info->unsupported_script_warning_printed = false;
   pspace_info->script_not_found_warning_printed = false;
@@ -660,7 +654,7 @@ maybe_add_script_file (struct auto_load_pspace_info *pspace_info, int loaded,
 		       const char *name, const char *full_path,
 		       const struct extension_language_defn *language)
 {
-  struct htab *htab = pspace_info->loaded_script_files;
+  struct htab *htab = pspace_info->loaded_script_files.get ();
   struct loaded_script **slot, entry;
   int in_hash_table;
 
@@ -708,7 +702,7 @@ maybe_add_script_text (struct auto_load_pspace_info *pspace_info,
 		       int loaded, const char *name,
 		       const struct extension_language_defn *language)
 {
-  struct htab *htab = pspace_info->loaded_script_texts;
+  struct htab *htab = pspace_info->loaded_script_texts.get ();
   struct loaded_script **slot, entry;
   int in_hash_table;
 
@@ -1299,7 +1293,7 @@ auto_load_info_scripts (const char *pattern, int from_tty,
       collect_matching_scripts_data data (&script_files, language);
 
       /* Pass a pointer to scripts as VEC_safe_push can realloc space.  */
-      htab_traverse_noresize (pspace_info->loaded_script_files,
+      htab_traverse_noresize (pspace_info->loaded_script_files.get (),
 			      collect_matching_scripts, &data);
 
       std::sort (script_files.begin (), script_files.end (),
@@ -1311,7 +1305,7 @@ auto_load_info_scripts (const char *pattern, int from_tty,
       collect_matching_scripts_data data (&script_texts, language);
 
       /* Pass a pointer to scripts as VEC_safe_push can realloc space.  */
-      htab_traverse_noresize (pspace_info->loaded_script_texts,
+      htab_traverse_noresize (pspace_info->loaded_script_texts.get (),
 			      collect_matching_scripts, &data);
 
       std::sort (script_texts.begin (), script_texts.end (),
-- 
2.17.2

Re: [PATCH 1/9] Use htab_up in auto-load.c

[Simon Marchi]

On 2020-07-18 1:29 p.m., Tom Tromey wrote:
> This changes auto-load.c to use htab_up, rather than manually calling
> htab_delete.
> 
> gdb/ChangeLog
> 2020-07-18  Tom Tromey  <tom@tromey.com>
> 
> 	* auto-load.c (struct auto_load_pspace_info)
> 	<~auto_load_pspace_info>: Default.
> 	<loaded_script_files, loaded_script_texts>: Change type to
> 	htab_up.
> 	(~auto_load_pspace_info) Remove.
> 	(init_loaded_scripts_info, maybe_add_script_file)
> 	(maybe_add_script_text, auto_load_info_scripts): Update.
> ---
>  gdb/ChangeLog   | 10 ++++++++++
>  gdb/auto-load.c | 40 +++++++++++++++++-----------------------
>  2 files changed, 27 insertions(+), 23 deletions(-)
> 
> diff --git a/gdb/auto-load.c b/gdb/auto-load.c
> index c967261925a..e21788f481f 100644
> --- a/gdb/auto-load.c
> +++ b/gdb/auto-load.c
> @@ -530,12 +530,12 @@ For more information about this security protection see the\n\
>  struct auto_load_pspace_info
>  {
>    auto_load_pspace_info () = default;
> -  ~auto_load_pspace_info ();
> +  ~auto_load_pspace_info () = default;

Can't these two just be removed?

Otherwise, LGTM.

Simon

Re: [PATCH 1/9] Use htab_up in auto-load.c

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> auto_load_pspace_info () = default;
>> -  ~auto_load_pspace_info ();
>> +  ~auto_load_pspace_info () = default;

Simon> Can't these two just be removed?

Simon> Otherwise, LGTM.

I made this change.

Tom

[PATCH 2/9] Use htab_up in breakpoint.c

[Tom Tromey]

This changes breakpoint.c to use htab_up rather than an explicit
htab_delete.  This simplifies the code somewhat.

gdb/ChangeLog
2020-07-18  Tom Tromey  <tom@tromey.com>

	* breakpoint.c (ambiguous_names_p): Use htab_up.
---
 gdb/ChangeLog    |  4 ++++
 gdb/breakpoint.c | 12 ++++--------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 6d81323dd92..0dd9aa39522 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -13257,8 +13257,8 @@ static int
 ambiguous_names_p (struct bp_location *loc)
 {
   struct bp_location *l;
-  htab_t htab = htab_create_alloc (13, htab_hash_string, streq_hash, NULL,
-				   xcalloc, xfree);
+  htab_up htab (htab_create_alloc (13, htab_hash_string, streq_hash, NULL,
+				   xcalloc, xfree));
 
   for (l = loc; l != NULL; l = l->next)
     {
@@ -13269,19 +13269,15 @@ ambiguous_names_p (struct bp_location *loc)
       if (name == NULL)
 	continue;
 
-      slot = (const char **) htab_find_slot (htab, (const void *) name,
+      slot = (const char **) htab_find_slot (htab.get (), (const void *) name,
 					     INSERT);
       /* NOTE: We can assume slot != NULL here because xcalloc never
 	 returns NULL.  */
       if (*slot != NULL)
-	{
-	  htab_delete (htab);
-	  return 1;
-	}
+	return 1;
       *slot = name;
     }
 
-  htab_delete (htab);
   return 0;
 }
 
-- 
2.17.2

[PATCH 3/9] Use htab_up in completion_tracker

[Tom Tromey]

This changes completion_tracker to use htab_up, rather than explicit
calls to htab_delete.

gdb/ChangeLog
2020-07-18  Tom Tromey  <tom@tromey.com>

	* completer.c (completion_tracker::discard_completions)
	(completion_tracker::~completion_tracker)
	(completion_tracker::maybe_add_completion)
	(completion_tracker::remove_completion)
	(completion_tracker::recompute_lowest_common_denominator)
	(completion_tracker::build_completion_result): Update.
	* completer.h (class completion_tracker) <have_completions>:
	Update.
	<m_entries_hash>: Now htab_up.
---
 gdb/ChangeLog   | 12 ++++++++++++
 gdb/completer.c | 29 +++++++++++++----------------
 gdb/completer.h |  4 ++--
 3 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 7d26774e851..e75b8b4b85d 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -1587,10 +1587,7 @@ completion_tracker::discard_completions ()
   m_lowest_common_denominator_unique = false;
   m_lowest_common_denominator_valid = false;
 
-  /* A null check here allows this function to be used from the
-     constructor.  */
-  if (m_entries_hash != NULL)
-    htab_delete (m_entries_hash);
+  m_entries_hash.reset (nullptr);
 
   /* A callback used by the hash table to compare new entries with existing
      entries.  We can't use the standard streq_hash function here as the
@@ -1618,10 +1615,10 @@ completion_tracker::discard_completions ()
 	return entry->hash_name ();
       };
 
-  m_entries_hash = htab_create_alloc (INITIAL_COMPLETION_HTAB_SIZE,
-				      entry_hash_func, entry_eq_func,
-				      completion_hash_entry::deleter,
-				      xcalloc, xfree);
+  m_entries_hash.reset (htab_create_alloc (INITIAL_COMPLETION_HTAB_SIZE,
+					   entry_hash_func, entry_eq_func,
+					   completion_hash_entry::deleter,
+					   xcalloc, xfree));
 }
 
 /* See completer.h.  */
@@ -1629,7 +1626,6 @@ completion_tracker::discard_completions ()
 completion_tracker::~completion_tracker ()
 {
   xfree (m_lowest_common_denominator);
-  htab_delete (m_entries_hash);
 }
 
 /* See completer.h.  */
@@ -1645,11 +1641,12 @@ completion_tracker::maybe_add_completion
   if (max_completions == 0)
     return false;
 
-  if (htab_elements (m_entries_hash) >= max_completions)
+  if (htab_elements (m_entries_hash.get ()) >= max_completions)
     return false;
 
   hashval_t hash = htab_hash_string (name.get ());
-  slot = htab_find_slot_with_hash (m_entries_hash, name.get (), hash, INSERT);
+  slot = htab_find_slot_with_hash (m_entries_hash.get (), name.get (),
+				   hash, INSERT);
   if (*slot == HTAB_EMPTY_ENTRY)
     {
       const char *match_for_lcd_str = NULL;
@@ -1700,10 +1697,10 @@ void
 completion_tracker::remove_completion (const char *name)
 {
   hashval_t hash = htab_hash_string (name);
-  if (htab_find_slot_with_hash (m_entries_hash, name, hash, NO_INSERT)
+  if (htab_find_slot_with_hash (m_entries_hash.get (), name, hash, NO_INSERT)
       != NULL)
     {
-      htab_remove_elt_with_hash (m_entries_hash, name, hash);
+      htab_remove_elt_with_hash (m_entries_hash.get (), name, hash);
       m_lowest_common_denominator_valid = false;
     }
 }
@@ -2144,7 +2141,7 @@ completion_tracker::recompute_lowest_common_denominator ()
 	return 1;
       };
 
-  htab_traverse (m_entries_hash, visitor_func, this);
+  htab_traverse (m_entries_hash.get (), visitor_func, this);
   m_lowest_common_denominator_valid = true;
 }
 
@@ -2227,7 +2224,7 @@ completion_result
 completion_tracker::build_completion_result (const char *text,
 					     int start, int end)
 {
-  size_t element_count = htab_elements (m_entries_hash);
+  size_t element_count = htab_elements (m_entries_hash.get ());
 
   if (element_count == 0)
     return {};
@@ -2294,7 +2291,7 @@ completion_tracker::build_completion_result (const char *text,
 	  };
 
       /* Build the completion list and add a null at the end.  */
-      htab_traverse_noresize (m_entries_hash, func, &builder);
+      htab_traverse_noresize (m_entries_hash.get (), func, &builder);
       match_list[builder.index] = NULL;
 
       return completion_result (match_list, builder.index - 1, false);
diff --git a/gdb/completer.h b/gdb/completer.h
index d3afa5fe3ec..60b3800b080 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -393,7 +393,7 @@ class completion_tracker
 
   /* True if we have any completion match recorded.  */
   bool have_completions () const
-  { return htab_elements (m_entries_hash) > 0; }
+  { return htab_elements (m_entries_hash.get ()) > 0; }
 
   /* Discard the current completion match list and the current
      LCD.  */
@@ -440,7 +440,7 @@ class completion_tracker
      will remove duplicates, and if removal of duplicates there brings
      the total under max_completions the user may think gdb quit
      searching too early.  */
-  htab_t m_entries_hash = NULL;
+  htab_up m_entries_hash;
 
   /* If non-zero, then this is the quote char that needs to be
      appended after completion (iff we have a unique completion).  We
-- 
2.17.2

[PATCH 4/9] Use htab_up in filename_seen_cache

[Tom Tromey]

This changes filename_seen_cache to use htab_up, rather than explicit
calls to htab_delete.

gdb/ChangeLog
2020-07-18  Tom Tromey  <tom@tromey.com>

	* filename-seen-cache.c (filename_seen_cache::filename_seen_cache)
	(filename_seen_cache::clear): Update.
	(~filename_seen_cache): Remove.
	(filename_seen_cache::seen): Update.
	* filename-seen-cache.h (class filename_seen_cache) <m_tab>: Now
	htab_up.
	<~filename_seen_cache>: Default.
	<traverse>: Update.
---
 gdb/ChangeLog             | 11 +++++++++++
 gdb/filename-seen-cache.c | 17 +++++------------
 gdb/filename-seen-cache.h |  6 +++---
 3 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/gdb/filename-seen-cache.c b/gdb/filename-seen-cache.c
index f3905c0fb12..b0cda087cc0 100644
--- a/gdb/filename-seen-cache.c
+++ b/gdb/filename-seen-cache.c
@@ -27,10 +27,10 @@
 /* filename_seen_cache constructor.  */
 
 filename_seen_cache::filename_seen_cache ()
+  : m_tab (htab_create_alloc (INITIAL_FILENAME_SEEN_CACHE_SIZE,
+			      filename_hash, filename_eq,
+			      NULL, xcalloc, xfree))
 {
-  m_tab = htab_create_alloc (INITIAL_FILENAME_SEEN_CACHE_SIZE,
-			     filename_hash, filename_eq,
-			     NULL, xcalloc, xfree);
 }
 
 /* See filename-seen-cache.h.  */
@@ -38,14 +38,7 @@ filename_seen_cache::filename_seen_cache ()
 void
 filename_seen_cache::clear ()
 {
-  htab_empty (m_tab);
-}
-
-/* See filename-seen-cache.h.  */
-
-filename_seen_cache::~filename_seen_cache ()
-{
-  htab_delete (m_tab);
+  htab_empty (m_tab.get ());
 }
 
 /* See filename-seen-cache.h.  */
@@ -56,7 +49,7 @@ filename_seen_cache::seen (const char *file)
   void **slot;
 
   /* Is FILE in tab?  */
-  slot = htab_find_slot (m_tab, file, INSERT);
+  slot = htab_find_slot (m_tab.get (), file, INSERT);
   if (*slot != NULL)
     return true;
 
diff --git a/gdb/filename-seen-cache.h b/gdb/filename-seen-cache.h
index ee064c32565..ccc47694fff 100644
--- a/gdb/filename-seen-cache.h
+++ b/gdb/filename-seen-cache.h
@@ -29,7 +29,7 @@ class filename_seen_cache
 {
 public:
   filename_seen_cache ();
-  ~filename_seen_cache ();
+  ~filename_seen_cache () = default;
 
   DISABLE_COPY_AND_ASSIGN (filename_seen_cache);
 
@@ -55,12 +55,12 @@ class filename_seen_cache
 	return 1;
       };
 
-    htab_traverse_noresize (m_tab, erased_cb, &callback);
+    htab_traverse_noresize (m_tab.get (), erased_cb, &callback);
   }
 
 private:
   /* Table of files seen so far.  */
-  htab_t m_tab;
+  htab_up m_tab;
 };
 
 #endif /* FILENAME_SEEN_CACHE_H */
-- 
2.17.2

Re: [PATCH 4/9] Use htab_up in filename_seen_cache

[Simon Marchi]

On 2020-07-18 1:29 p.m., Tom Tromey wrote:
> This changes filename_seen_cache to use htab_up, rather than explicit
> calls to htab_delete.
> 
> gdb/ChangeLog
> 2020-07-18  Tom Tromey  <tom@tromey.com>
> 
> 	* filename-seen-cache.c (filename_seen_cache::filename_seen_cache)
> 	(filename_seen_cache::clear): Update.
> 	(~filename_seen_cache): Remove.
> 	(filename_seen_cache::seen): Update.
> 	* filename-seen-cache.h (class filename_seen_cache) <m_tab>: Now
> 	htab_up.
> 	<~filename_seen_cache>: Default.
> 	<traverse>: Update.
> ---
>  gdb/ChangeLog             | 11 +++++++++++
>  gdb/filename-seen-cache.c | 17 +++++------------
>  gdb/filename-seen-cache.h |  6 +++---
>  3 files changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/gdb/filename-seen-cache.c b/gdb/filename-seen-cache.c
> index f3905c0fb12..b0cda087cc0 100644
> --- a/gdb/filename-seen-cache.c
> +++ b/gdb/filename-seen-cache.c
> @@ -27,10 +27,10 @@
>  /* filename_seen_cache constructor.  */
>  
>  filename_seen_cache::filename_seen_cache ()
> +  : m_tab (htab_create_alloc (INITIAL_FILENAME_SEEN_CACHE_SIZE,
> +			      filename_hash, filename_eq,
> +			      NULL, xcalloc, xfree))
>  {
> -  m_tab = htab_create_alloc (INITIAL_FILENAME_SEEN_CACHE_SIZE,
> -			     filename_hash, filename_eq,
> -			     NULL, xcalloc, xfree);
>  }
>  
>  /* See filename-seen-cache.h.  */
> @@ -38,14 +38,7 @@ filename_seen_cache::filename_seen_cache ()
>  void
>  filename_seen_cache::clear ()
>  {
> -  htab_empty (m_tab);
> -}
> -
> -/* See filename-seen-cache.h.  */
> -
> -filename_seen_cache::~filename_seen_cache ()
> -{
> -  htab_delete (m_tab);
> +  htab_empty (m_tab.get ());
>  }
>  
>  /* See filename-seen-cache.h.  */
> @@ -56,7 +49,7 @@ filename_seen_cache::seen (const char *file)
>    void **slot;
>  
>    /* Is FILE in tab?  */
> -  slot = htab_find_slot (m_tab, file, INSERT);
> +  slot = htab_find_slot (m_tab.get (), file, INSERT);
>    if (*slot != NULL)
>      return true;
>  
> diff --git a/gdb/filename-seen-cache.h b/gdb/filename-seen-cache.h
> index ee064c32565..ccc47694fff 100644
> --- a/gdb/filename-seen-cache.h
> +++ b/gdb/filename-seen-cache.h
> @@ -29,7 +29,7 @@ class filename_seen_cache
>  {
>  public:
>    filename_seen_cache ();
> -  ~filename_seen_cache ();
> +  ~filename_seen_cache () = default;

Same as in the other patch, I don't really see the point to keep the explicitly defaulted destructor.  Otherwise, LGTM.

Simon

[PATCH 5/9] Use htab_up in linespec.c

[Tom Tromey]

This changes linespec.c to use htab_up rather than explicit calls to
htab_delete.  Note that a use still exists in this file, because
linespec_state hasn't been converted to have a real destructor.

gdb/ChangeLog
2020-07-18  Tom Tromey  <tom@tromey.com>

	* linespec.c (class decode_compound_collector)
	<~decode_compound_collector>: Default.
	<m_unique_syms>: Now htab_up.
	(decode_compound_collector::operator ()): Update.
	(class symtab_collector) <~symtab_collector>: Default.
	<m_symtab_table>: Now htab_up.
	(symtab_collector::operator ()): Update.
---
 gdb/ChangeLog  | 10 ++++++++++
 gdb/linespec.c | 30 +++++++++++-------------------
 2 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 4a634e3aff9..57a6283a2d6 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -3449,17 +3449,13 @@ class decode_compound_collector
 {
 public:
   decode_compound_collector ()
+    : m_unique_syms (htab_create_alloc (1, htab_hash_pointer,
+					htab_eq_pointer, NULL,
+					xcalloc, xfree))
   {
-    m_unique_syms = htab_create_alloc (1, htab_hash_pointer,
-				       htab_eq_pointer, NULL,
-				       xcalloc, xfree);
   }
 
-  ~decode_compound_collector ()
-  {
-    if (m_unique_syms != NULL)
-      htab_delete (m_unique_syms);
-  }
+  ~decode_compound_collector () = default;
 
   /* Return all symbols collected.  */
   std::vector<block_symbol> release_symbols ()
@@ -3473,7 +3469,7 @@ class decode_compound_collector
 private:
   /* A hash table of all symbols we found.  We use this to avoid
      adding any symbol more than once.  */
-  htab_t m_unique_syms;
+  htab_up m_unique_syms;
 
   /* The result vector.  */
   std::vector<block_symbol>  m_symbols;
@@ -3496,7 +3492,7 @@ decode_compound_collector::operator () (block_symbol *bsym)
       && t->code () != TYPE_CODE_NAMESPACE)
     return true; /* Continue iterating.  */
 
-  slot = htab_find_slot (m_unique_syms, sym, INSERT);
+  slot = htab_find_slot (m_unique_syms.get (), sym, INSERT);
   if (!*slot)
     {
       *slot = sym;
@@ -3726,16 +3722,12 @@ class symtab_collector
 {
 public:
   symtab_collector ()
+    : m_symtab_table (htab_create (1, htab_hash_pointer, htab_eq_pointer,
+				   NULL))
   {
-    m_symtab_table = htab_create (1, htab_hash_pointer, htab_eq_pointer,
-				  NULL);
   }
 
-  ~symtab_collector ()
-  {
-    if (m_symtab_table != NULL)
-      htab_delete (m_symtab_table);
-  }
+  ~symtab_collector () = default;
 
   /* Callable as a symbol_found_callback_ftype callback.  */
   bool operator () (symtab *sym);
@@ -3751,7 +3743,7 @@ class symtab_collector
   std::vector<symtab *> m_symtabs;
 
   /* This is used to ensure the symtabs are unique.  */
-  htab_t m_symtab_table;
+  htab_up m_symtab_table;
 };
 
 bool
@@ -3759,7 +3751,7 @@ symtab_collector::operator () (struct symtab *symtab)
 {
   void **slot;
 
-  slot = htab_find_slot (m_symtab_table, symtab, INSERT);
+  slot = htab_find_slot (m_symtab_table.get (), symtab, INSERT);
   if (!*slot)
     {
       *slot = symtab;
-- 
2.17.2

Re: [PATCH 5/9] Use htab_up in linespec.c

[Simon Marchi]

On 2020-07-18 1:29 p.m., Tom Tromey wrote:
> This changes linespec.c to use htab_up rather than explicit calls to
> htab_delete.  Note that a use still exists in this file, because
> linespec_state hasn't been converted to have a real destructor.
> 
> gdb/ChangeLog
> 2020-07-18  Tom Tromey  <tom@tromey.com>
> 
> 	* linespec.c (class decode_compound_collector)
> 	<~decode_compound_collector>: Default.
> 	<m_unique_syms>: Now htab_up.
> 	(decode_compound_collector::operator ()): Update.
> 	(class symtab_collector) <~symtab_collector>: Default.
> 	<m_symtab_table>: Now htab_up.
> 	(symtab_collector::operator ()): Update.
> ---
>  gdb/ChangeLog  | 10 ++++++++++
>  gdb/linespec.c | 30 +++++++++++-------------------
>  2 files changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index 4a634e3aff9..57a6283a2d6 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -3449,17 +3449,13 @@ class decode_compound_collector
>  {
>  public:
>    decode_compound_collector ()
> +    : m_unique_syms (htab_create_alloc (1, htab_hash_pointer,
> +					htab_eq_pointer, NULL,
> +					xcalloc, xfree))
>    {
> -    m_unique_syms = htab_create_alloc (1, htab_hash_pointer,
> -				       htab_eq_pointer, NULL,
> -				       xcalloc, xfree);
>    }
>  
> -  ~decode_compound_collector ()
> -  {
> -    if (m_unique_syms != NULL)
> -      htab_delete (m_unique_syms);
> -  }
> +  ~decode_compound_collector () = default;
>  
>    /* Return all symbols collected.  */
>    std::vector<block_symbol> release_symbols ()
> @@ -3473,7 +3469,7 @@ class decode_compound_collector
>  private:
>    /* A hash table of all symbols we found.  We use this to avoid
>       adding any symbol more than once.  */
> -  htab_t m_unique_syms;
> +  htab_up m_unique_syms;
>  
>    /* The result vector.  */
>    std::vector<block_symbol>  m_symbols;
> @@ -3496,7 +3492,7 @@ decode_compound_collector::operator () (block_symbol *bsym)
>        && t->code () != TYPE_CODE_NAMESPACE)
>      return true; /* Continue iterating.  */
>  
> -  slot = htab_find_slot (m_unique_syms, sym, INSERT);
> +  slot = htab_find_slot (m_unique_syms.get (), sym, INSERT);
>    if (!*slot)
>      {
>        *slot = sym;
> @@ -3726,16 +3722,12 @@ class symtab_collector
>  {
>  public:
>    symtab_collector ()
> +    : m_symtab_table (htab_create (1, htab_hash_pointer, htab_eq_pointer,
> +				   NULL))
>    {
> -    m_symtab_table = htab_create (1, htab_hash_pointer, htab_eq_pointer,
> -				  NULL);
>    }
>  
> -  ~symtab_collector ()
> -  {
> -    if (m_symtab_table != NULL)
> -      htab_delete (m_symtab_table);
> -  }
> +  ~symtab_collector () = default;

Same comment as earlier, I'll stop mentioning those :).  Other than that, the rest of the series LGTM.

Simon

Re: [PATCH 5/9] Use htab_up in linespec.c

[Tom Tromey]

>> +  ~symtab_collector () = default;

Simon> Same comment as earlier, I'll stop mentioning those :).  Other
Simon> than that, the rest of the series LGTM.

Thanks.  I went through and removed these.
I'll probably hold this series until after branching.

Tom

[PATCH 6/9] Use htab_up in target-descriptions.c

[Tom Tromey]

This changes target-descriptions.c to use htab_up rather than explicit
calls to htab_delete.

gdb/ChangeLog
2020-07-18  Tom Tromey  <tom@tromey.com>

	* target-descriptions.c (tdesc_use_registers): Use htab_up.
---
 gdb/ChangeLog             |  4 ++++
 gdb/target-descriptions.c | 16 +++++++---------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index 20d624c0c65..41ecabe7809 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -1102,7 +1102,6 @@ tdesc_use_registers (struct gdbarch *gdbarch,
 {
   int num_regs = gdbarch_num_regs (gdbarch);
   struct tdesc_arch_data *data;
-  htab_t reg_hash;
 
   /* We can't use the description for registers if it doesn't describe
      any.  This function should only be called after validating
@@ -1117,11 +1116,12 @@ tdesc_use_registers (struct gdbarch *gdbarch,
   /* Build up a set of all registers, so that we can assign register
      numbers where needed.  The hash table expands as necessary, so
      the initial size is arbitrary.  */
-  reg_hash = htab_create (37, htab_hash_pointer, htab_eq_pointer, NULL);
+  htab_up reg_hash (htab_create (37, htab_hash_pointer, htab_eq_pointer,
+				 NULL));
   for (const tdesc_feature_up &feature : target_desc->features)
     for (const tdesc_reg_up &reg : feature->registers)
       {
-	void **slot = htab_find_slot (reg_hash, reg.get (), INSERT);
+	void **slot = htab_find_slot (reg_hash.get (), reg.get (), INSERT);
 
 	*slot = reg.get ();
 	/* Add reggroup if its new.  */
@@ -1136,7 +1136,7 @@ tdesc_use_registers (struct gdbarch *gdbarch,
      architecture.  */
   for (const tdesc_arch_reg &arch_reg : data->arch_regs)
     if (arch_reg.reg != NULL)
-      htab_remove_elt (reg_hash, arch_reg.reg);
+      htab_remove_elt (reg_hash.get (), arch_reg.reg);
 
   /* Assign numbers to the remaining registers and add them to the
      list of registers.  The new numbers are always above gdbarch_num_regs.
@@ -1154,7 +1154,7 @@ tdesc_use_registers (struct gdbarch *gdbarch,
     {
       for (const tdesc_feature_up &feature : target_desc->features)
 	for (const tdesc_reg_up &reg : feature->registers)
-	  if (htab_find (reg_hash, reg.get ()) != NULL)
+	  if (htab_find (reg_hash.get (), reg.get ()) != NULL)
 	    {
 	      int regno = unk_reg_cb (gdbarch, feature.get (),
 				      reg->name.c_str (), num_regs);
@@ -1165,7 +1165,7 @@ tdesc_use_registers (struct gdbarch *gdbarch,
 		    data->arch_regs.emplace_back (nullptr, nullptr);
 		  data->arch_regs[regno] = tdesc_arch_reg (reg.get (), NULL);
 		  num_regs = regno + 1;
-		  htab_remove_elt (reg_hash, reg.get ());
+		  htab_remove_elt (reg_hash.get (), reg.get ());
 		}
 	    }
     }
@@ -1177,14 +1177,12 @@ tdesc_use_registers (struct gdbarch *gdbarch,
      unnumbered registers.  */
   for (const tdesc_feature_up &feature : target_desc->features)
     for (const tdesc_reg_up &reg : feature->registers)
-      if (htab_find (reg_hash, reg.get ()) != NULL)
+      if (htab_find (reg_hash.get (), reg.get ()) != NULL)
 	{
 	  data->arch_regs.emplace_back (reg.get (), nullptr);
 	  num_regs++;
 	}
 
-  htab_delete (reg_hash);
-
   /* Update the architecture.  */
   set_gdbarch_num_regs (gdbarch, num_regs);
   set_gdbarch_register_name (gdbarch, tdesc_register_name);
-- 
2.17.2

[PATCH 7/9] Use htab_up in typedef_hash_table

[Tom Tromey]

This changes typedef_hash_table to use htab_up rather than explicit
calls to htab_delete.

gdb/ChangeLog
2020-07-18  Tom Tromey  <tom@tromey.com>

	* typeprint.h (class typedef_hash_table) <~typedef_hash_table>:
	Default.
	<m_table>: Now htab_up.
	* typeprint.c (typedef_hash_table::recursively_update)
	(typedef_hash_table::add_template_parameters)
	(typedef_hash_table::typedef_hash_table): Update.
	(typedef_hash_table::~typedef_hash_table): Remove.
	(typedef_hash_table::typedef_hash_table)
	(typedef_hash_table::find_global_typedef)
	(typedef_hash_table::find_typedef): Update.
---
 gdb/ChangeLog   | 13 +++++++++++++
 gdb/typeprint.c | 29 +++++++++++------------------
 gdb/typeprint.h |  4 ++--
 3 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/gdb/typeprint.c b/gdb/typeprint.c
index 08b9a426eac..52883a9afcf 100644
--- a/gdb/typeprint.c
+++ b/gdb/typeprint.c
@@ -209,7 +209,7 @@ typedef_hash_table::recursively_update (struct type *t)
       struct decl_field *tdef = &TYPE_TYPEDEF_FIELD (t, i);
       void **slot;
 
-      slot = htab_find_slot (m_table, tdef, INSERT);
+      slot = htab_find_slot (m_table.get (), tdef, INSERT);
       /* Only add a given typedef name once.  Really this shouldn't
 	 happen; but it is safe enough to do the updates breadth-first
 	 and thus use the most specific typedef.  */
@@ -242,7 +242,7 @@ typedef_hash_table::add_template_parameters (struct type *t)
       tf->name = TYPE_TEMPLATE_ARGUMENT (t, i)->linkage_name ();
       tf->type = SYMBOL_TYPE (TYPE_TEMPLATE_ARGUMENT (t, i));
 
-      slot = htab_find_slot (m_table, tf, INSERT);
+      slot = htab_find_slot (m_table.get (), tf, INSERT);
       if (*slot == NULL)
 	*slot = tf;
     }
@@ -251,16 +251,9 @@ typedef_hash_table::add_template_parameters (struct type *t)
 /* See typeprint.h.  */
 
 typedef_hash_table::typedef_hash_table ()
+  : m_table (htab_create_alloc (10, hash_typedef_field, eq_typedef_field,
+				NULL, xcalloc, xfree))
 {
-  m_table = htab_create_alloc (10, hash_typedef_field, eq_typedef_field,
-			       NULL, xcalloc, xfree);
-}
-
-/* Free a typedef field table.  */
-
-typedef_hash_table::~typedef_hash_table ()
-{
-  htab_delete (m_table);
 }
 
 /* Helper function for typedef_hash_table::copy.  */
@@ -282,10 +275,10 @@ copy_typedef_hash_element (void **slot, void *nt)
 
 typedef_hash_table::typedef_hash_table (const typedef_hash_table &table)
 {
-  m_table = htab_create_alloc (10, hash_typedef_field, eq_typedef_field,
-			       NULL, xcalloc, xfree);
-  htab_traverse_noresize (table.m_table, copy_typedef_hash_element,
-			  m_table);
+  m_table.reset (htab_create_alloc (10, hash_typedef_field, eq_typedef_field,
+				    NULL, xcalloc, xfree));
+  htab_traverse_noresize (table.m_table.get (), copy_typedef_hash_element,
+			  m_table.get ());
 }
 
 /* Look up the type T in the global typedef hash.  If it is found,
@@ -307,7 +300,7 @@ typedef_hash_table::find_global_typedef (const struct type_print_options *flags,
   tf.name = NULL;
   tf.type = t;
 
-  slot = htab_find_slot (flags->global_typedefs->m_table, &tf, INSERT);
+  slot = htab_find_slot (flags->global_typedefs->m_table.get (), &tf, INSERT);
   if (*slot != NULL)
     {
       new_tf = (struct decl_field *) *slot;
@@ -346,8 +339,8 @@ typedef_hash_table::find_typedef (const struct type_print_options *flags,
 
       tf.name = NULL;
       tf.type = t;
-      found = (struct decl_field *) htab_find (flags->local_typedefs->m_table,
-					       &tf);
+      htab_t table = flags->local_typedefs->m_table.get ();
+      found = (struct decl_field *) htab_find (table, &tf);
 
       if (found != NULL)
 	return found->name;
diff --git a/gdb/typeprint.h b/gdb/typeprint.h
index bd643dab894..93c4393bd7a 100644
--- a/gdb/typeprint.h
+++ b/gdb/typeprint.h
@@ -116,7 +116,7 @@ class typedef_hash_table
   /* Create a new typedef-lookup hash table.  */
   typedef_hash_table ();
 
-  ~typedef_hash_table ();
+  ~typedef_hash_table () = default;
 
   /* Copy a typedef hash.  */
   typedef_hash_table (const typedef_hash_table &);
@@ -144,7 +144,7 @@ class typedef_hash_table
 
 
   /* The actual hash table.  */
-  htab_t m_table;
+  htab_up m_table;
 
   /* Storage for typedef_field objects that must be synthesized.  */
   auto_obstack m_storage;
-- 
2.17.2

[PATCH 8/9] Use htab_up in type copying

[Tom Tromey]

This changes create_copied_types_hash to return an htab_up, then
modifies the callers to avoid explicit use of htab_delete.

gdb/ChangeLog
2020-07-18  Tom Tromey  <tom@tromey.com>

	* value.c (preserve_values): Update.
	* python/py-type.c (save_objfile_types): Update.
	* guile/scm-type.c (save_objfile_types): Update.
	* gdbtypes.h (create_copied_types_hash): Return htab_up.
	* gdbtypes.c (create_copied_types_hash): Return htab_up.
	* compile/compile-object-run.c (compile_object_run): Update.
---
 gdb/ChangeLog                    |  9 +++++++++
 gdb/compile/compile-object-run.c |  6 ++----
 gdb/gdbtypes.c                   | 10 +++++-----
 gdb/gdbtypes.h                   |  2 +-
 gdb/guile/scm-type.c             |  7 ++-----
 gdb/python/py-type.c             | 10 ++++------
 gdb/value.c                      | 11 ++++-------
 7 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/gdb/compile/compile-object-run.c b/gdb/compile/compile-object-run.c
index a2f39900053..533478a0fb4 100644
--- a/gdb/compile/compile-object-run.c
+++ b/gdb/compile/compile-object-run.c
@@ -140,14 +140,12 @@ compile_object_run (struct compile_module *module)
   try
     {
       struct type *func_type = SYMBOL_TYPE (func_sym);
-      htab_t copied_types;
       int current_arg = 0;
       struct value **vargs;
 
       /* OBJFILE may disappear while FUNC_TYPE still will be in use.  */
-      copied_types = create_copied_types_hash (objfile);
-      func_type = copy_type_recursive (objfile, func_type, copied_types);
-      htab_delete (copied_types);
+      htab_up copied_types = create_copied_types_hash (objfile);
+      func_type = copy_type_recursive (objfile, func_type, copied_types.get ());
 
       gdb_assert (func_type->code () == TYPE_CODE_FUNC);
       func_val = value_from_pointer (lookup_pointer_type (func_type),
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index e87648813ec..9c395846895 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -5201,13 +5201,13 @@ type_pair_eq (const void *item_lhs, const void *item_rhs)
    types without duplicates.  We use OBJFILE's obstack, because
    OBJFILE is about to be deleted.  */
 
-htab_t
+htab_up
 create_copied_types_hash (struct objfile *objfile)
 {
-  return htab_create_alloc_ex (1, type_pair_hash, type_pair_eq,
-			       NULL, &objfile->objfile_obstack,
-			       hashtab_obstack_allocate,
-			       dummy_obstack_deallocate);
+  return htab_up (htab_create_alloc_ex (1, type_pair_hash, type_pair_eq,
+					NULL, &objfile->objfile_obstack,
+					hashtab_obstack_allocate,
+					dummy_obstack_deallocate));
 }
 
 /* Recursively copy (deep copy) a dynamic attribute list of a type.  */
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index eaa4cff608d..3a8b5e7ad80 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -2379,7 +2379,7 @@ extern int class_or_union_p (const struct type *);
 
 extern void maintenance_print_type (const char *, int);
 
-extern htab_t create_copied_types_hash (struct objfile *objfile);
+extern htab_up create_copied_types_hash (struct objfile *objfile);
 
 extern struct type *copy_type_recursive (struct objfile *objfile,
 					 struct type *type,
diff --git a/gdb/guile/scm-type.c b/gdb/guile/scm-type.c
index 19b7996c946..8fc9629eb0d 100644
--- a/gdb/guile/scm-type.c
+++ b/gdb/guile/scm-type.c
@@ -387,20 +387,17 @@ static void
 save_objfile_types (struct objfile *objfile, void *datum)
 {
   htab_t htab = (htab_t) datum;
-  htab_t copied_types;
 
   if (!gdb_scheme_initialized)
     return;
 
-  copied_types = create_copied_types_hash (objfile);
+  htab_up copied_types = create_copied_types_hash (objfile);
 
   if (htab != NULL)
     {
-      htab_traverse_noresize (htab, tyscm_copy_type_recursive, copied_types);
+      htab_traverse_noresize (htab, tyscm_copy_type_recursive, copied_types.get ());
       htab_delete (htab);
     }
-
-  htab_delete (copied_types);
 }
 \f
 /* Administrivia for field smobs.  */
diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
index d0dfb52811b..f37a7652f6d 100644
--- a/gdb/python/py-type.c
+++ b/gdb/python/py-type.c
@@ -1068,7 +1068,6 @@ static void
 save_objfile_types (struct objfile *objfile, void *datum)
 {
   type_object *obj = (type_object *) datum;
-  htab_t copied_types;
 
   if (!gdb_python_initialized)
     return;
@@ -1077,23 +1076,22 @@ save_objfile_types (struct objfile *objfile, void *datum)
      operating on.  */
   gdbpy_enter enter_py (objfile->arch (), current_language);
 
-  copied_types = create_copied_types_hash (objfile);
+  htab_up copied_types = create_copied_types_hash (objfile);
 
   while (obj)
     {
       type_object *next = obj->next;
 
-      htab_empty (copied_types);
+      htab_empty (copied_types.get ());
 
-      obj->type = copy_type_recursive (objfile, obj->type, copied_types);
+      obj->type = copy_type_recursive (objfile, obj->type,
+				       copied_types.get ());
 
       obj->next = NULL;
       obj->prev = NULL;
 
       obj = next;
     }
-
-  htab_delete (copied_types);
 }
 
 static void
diff --git a/gdb/value.c b/gdb/value.c
index 3a5b02bcb46..a018443a2c5 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -2516,22 +2516,19 @@ preserve_one_internalvar (struct internalvar *var, struct objfile *objfile,
 void
 preserve_values (struct objfile *objfile)
 {
-  htab_t copied_types;
   struct internalvar *var;
 
   /* Create the hash table.  We allocate on the objfile's obstack, since
      it is soon to be deleted.  */
-  copied_types = create_copied_types_hash (objfile);
+  htab_up copied_types = create_copied_types_hash (objfile);
 
   for (const value_ref_ptr &item : value_history)
-    preserve_one_value (item.get (), objfile, copied_types);
+    preserve_one_value (item.get (), objfile, copied_types.get ());
 
   for (var = internalvars; var; var = var->next)
-    preserve_one_internalvar (var, objfile, copied_types);
+    preserve_one_internalvar (var, objfile, copied_types.get ());
 
-  preserve_ext_lang_values (objfile, copied_types);
-
-  htab_delete (copied_types);
+  preserve_ext_lang_values (objfile, copied_types.get ());
 }
 
 static void
-- 
2.17.2

Re: [PATCH 8/9] Use htab_up in type copying

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2020-07-18 11:29:14 -0600]:

> This changes create_copied_types_hash to return an htab_up, then
> modifies the callers to avoid explicit use of htab_delete.
> 
> gdb/ChangeLog
> 2020-07-18  Tom Tromey  <tom@tromey.com>
> 
> 	* value.c (preserve_values): Update.
> 	* python/py-type.c (save_objfile_types): Update.
> 	* guile/scm-type.c (save_objfile_types): Update.
> 	* gdbtypes.h (create_copied_types_hash): Return htab_up.
> 	* gdbtypes.c (create_copied_types_hash): Return htab_up.
> 	* compile/compile-object-run.c (compile_object_run): Update.

This introduced a use after free error, which is addressed by the
patch below.

Thanks,
Andrew

---


From 1c878706d8752b1f1ff61a8a5270675f2b4d828a Mon Sep 17 00:00:00 2001
From: Andrew Burgess <andrew.burgess@embecosm.com>
Date: Fri, 18 Sep 2020 18:23:06 +0100
Subject: [PATCH] gdb: Fix use after free bug in compile_object_run

In this commit:

  commit 6108fd1823f9cf036bbbe528ffcdf2fee489b40a
  Date:   Thu Sep 17 11:47:50 2020 -0600

      Use htab_up in type copying

A use after free bug was introduced.  In compile-object-run.c, in the
function compile_object_run, the code used to look like this:

    htab_t copied_types;

    /* .... snip .... */

    /* OBJFILE may disappear while FUNC_TYPE still will be in use.  */
    copied_types = create_copied_types_hash (objfile);
    func_type = copy_type_recursive (objfile, func_type, copied_types);
    htab_delete (copied_types);

    /* .... snip .... */

    call_function_by_hand_dummy (func_val, NULL, args,
                                 do_module_cleanup, data);

The copied_types table exists on the obstack of objfile, but is
deleted once the call to copy_type_recursive has been completed.

After the change the code now looks like this:

    /* OBJFILE may disappear while FUNC_TYPE still will be in use.  */
    htab_up copied_types = create_copied_types_hash (objfile);
    func_type = copy_type_recursive (objfile, func_type, copied_types.get ());

    /* .... snip .... */

    call_function_by_hand_dummy (func_val, NULL, args,
                                 do_module_cleanup, data);

The copied_types is now a unique_ptr and deleted automatically when it
goes out of scope.

The problem however is that objfile, and its included obstack, may be
deleted by the call to do_module_cleanup, which is called by
call_function_by_hand_dummy.

This means that in the new code the objfile, and its obstack, are
deleted before copied_types is deleted, and as copied_types is on the
objfiles obstack, we are now reading undefined memory.

The solution in this commit is to wrap the call to
create_copied_types_hash and copy_type_recursive into a new static
helper function.  The htab_up will then be deleted within the new
function's scope, before objfile is deleted.

gdb/ChangeLog:

	* compile/compile-object-run.c (create_copied_type_recursive): New
	function.
	(compile_object_run): Use new function.
---
 gdb/ChangeLog                    |  6 ++++++
 gdb/compile/compile-object-run.c | 17 ++++++++++++++---
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/gdb/compile/compile-object-run.c b/gdb/compile/compile-object-run.c
index 533478a0fb4..985c6f363a3 100644
--- a/gdb/compile/compile-object-run.c
+++ b/gdb/compile/compile-object-run.c
@@ -105,6 +105,16 @@ do_module_cleanup (void *arg, int registers_valid)
   xfree (data);
 }
 
+/* Create a copy of FUNC_TYPE that is independent of OBJFILE.  */
+
+static type *
+create_copied_type_recursive (objfile *objfile, type *func_type)
+{
+  htab_up copied_types = create_copied_types_hash (objfile);
+  func_type = copy_type_recursive (objfile, func_type, copied_types.get ());
+  return func_type;
+}
+
 /* Perform inferior call of MODULE.  This function may throw an error.
    This function may leave files referenced by MODULE on disk until
    the inferior call dummy frame is discarded.  This function may throw errors.
@@ -143,9 +153,10 @@ compile_object_run (struct compile_module *module)
       int current_arg = 0;
       struct value **vargs;
 
-      /* OBJFILE may disappear while FUNC_TYPE still will be in use.  */
-      htab_up copied_types = create_copied_types_hash (objfile);
-      func_type = copy_type_recursive (objfile, func_type, copied_types.get ());
+      /* OBJFILE may disappear while FUNC_TYPE is still in use as a
+	 result of the call to DO_MODULE_CLEANUP below, so we need a copy
+	 that does not depend on the objfile in anyway.  */
+      func_type = create_copied_type_recursive (objfile, func_type);
 
       gdb_assert (func_type->code () == TYPE_CODE_FUNC);
       func_val = value_from_pointer (lookup_pointer_type (func_type),
-- 
2.25.4

Re: [PATCH 8/9] Use htab_up in type copying

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> A use after free bug was introduced.  In compile-object-run.c, in the
Andrew> function compile_object_run, the code used to look like this:

Oops, sorry.

Andrew> 	* compile/compile-object-run.c (create_copied_type_recursive): New
Andrew> 	function.
Andrew> 	(compile_object_run): Use new function.

Looks good.

Tom

Re: [PATCH 8/9] Use htab_up in type copying

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2020-09-18 12:03:05 -0600]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew> A use after free bug was introduced.  In compile-object-run.c, in the
> Andrew> function compile_object_run, the code used to look like this:
> 
> Oops, sorry.
> 
> Andrew> 	* compile/compile-object-run.c (create_copied_type_recursive): New
> Andrew> 	function.
> Andrew> 	(compile_object_run): Use new function.
> 
> Looks good.

Thanks,

Pushed.

Andrew

[PATCH 9/9] Use htab_up in dwarf2/read.c

[Tom Tromey]

This changes dwarf2/read.c to use htab_up rather than explicit calls
to htab_delete.

gdb/ChangeLog
2020-07-18  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (compute_compunit_symtab_includes): Use htab_up.
---
 gdb/ChangeLog     |  4 ++++
 gdb/dwarf2/read.c | 20 +++++++++-----------
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 39ed455def5..4a065974ec2 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -9818,23 +9818,24 @@ compute_compunit_symtab_includes (dwarf2_per_cu_data *per_cu,
     {
       int len;
       std::vector<compunit_symtab *> result_symtabs;
-      htab_t all_children, all_type_symtabs;
       compunit_symtab *cust = per_objfile->get_symtab (per_cu);
 
       /* If we don't have a symtab, we can just skip this case.  */
       if (cust == NULL)
 	return;
 
-      all_children = htab_create_alloc (1, htab_hash_pointer, htab_eq_pointer,
-					NULL, xcalloc, xfree);
-      all_type_symtabs = htab_create_alloc (1, htab_hash_pointer, htab_eq_pointer,
-					    NULL, xcalloc, xfree);
+      htab_up all_children (htab_create_alloc (1, htab_hash_pointer,
+					       htab_eq_pointer,
+					       NULL, xcalloc, xfree));
+      htab_up all_type_symtabs (htab_create_alloc (1, htab_hash_pointer,
+						   htab_eq_pointer,
+						   NULL, xcalloc, xfree));
 
       for (dwarf2_per_cu_data *ptr : *per_cu->imported_symtabs)
 	{
-	  recursively_compute_inclusions (&result_symtabs, all_children,
-					  all_type_symtabs, ptr, per_objfile,
-					  cust);
+	  recursively_compute_inclusions (&result_symtabs, all_children.get (),
+					  all_type_symtabs.get (), ptr,
+					  per_objfile, cust);
 	}
 
       /* Now we have a transitive closure of all the included symtabs.  */
@@ -9845,9 +9846,6 @@ compute_compunit_symtab_includes (dwarf2_per_cu_data *per_cu,
       memcpy (cust->includes, result_symtabs.data (),
 	      len * sizeof (compunit_symtab *));
       cust->includes[len] = NULL;
-
-      htab_delete (all_children);
-      htab_delete (all_type_symtabs);
     }
 }
 
-- 
2.17.2

Re: [PATCH 0/9] Use htab_up in more places

[Tom Tromey]

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

Tom> This series changes various spots in gdb to use htab_up rather than
Tom> explicitly managing the hash table.  Not every spot can be changed,
Tom> but many can; and in several cases this results in a code
Tom> simplification.

I'm checking this in now.

Tom