diff mbox series

[v4,21/22] Convert dwarf2_cu::call_site_htab to new hash table

Message ID 20240823184910.883268-22-simon.marchi@efficios.com
State New
Headers
Series Add a C++ hash table to gdbsupport |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Simon Marchi Aug. 23, 2024, 6:48 p.m. UTC 
  Convert one use of htab_t, mapping (unrelocated) pc to call_site
objects, to `gdb::unordered_map<unrelocated_addr, call_site *>`.

Change-Id: I40a0903253a8589dbdcb75d52ad4d233931f6641
---
 gdb/dwarf2/call-site.h |  3 +++
 gdb/dwarf2/cu.h        |  2 +-
 gdb/dwarf2/read.c      | 27 ++++++++++-----------------
 gdb/symtab.c           | 30 ++++++++++--------------------
 gdb/symtab.h           |  5 +++--
 5 files changed, 27 insertions(+), 40 deletions(-)

Comments

Tom Tromey Oct. 4, 2024, 8:53 p.m. UTC | #1 
>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> Convert one use of htab_t, mapping (unrelocated) pc to call_site
Simon> objects, to `gdb::unordered_map<unrelocated_addr, call_site *>`.

Simon> -    cu->call_site_htab.reset (htab_create_alloc (16, call_site::hash,
Simon> -						 call_site::eq, nullptr,
Simon> -						 xcalloc, xfree));

Are there other users of call_site::eq and call_site::hash?
I suspect these can be removed now.

Simon> +using call_site_htab_t = gdb::unordered_map<unrelocated_addr, call_site *>;

This is another case of replacing a set with a map.
I wonder what the expense is.  You'd have to examine a large program
built with optimization to really know.

Tom
Simon Marchi Oct. 7, 2024, 6:19 p.m. UTC | #2 
On 2024-10-04 16:53, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> Convert one use of htab_t, mapping (unrelocated) pc to call_site
> Simon> objects, to `gdb::unordered_map<unrelocated_addr, call_site *>`.
> 
> Simon> -    cu->call_site_htab.reset (htab_create_alloc (16, call_site::hash,
> Simon> -						 call_site::eq, nullptr,
> Simon> -						 xcalloc, xfree));
> 
> Are there other users of call_site::eq and call_site::hash?
> I suspect these can be removed now.
> 
> Simon> +using call_site_htab_t = gdb::unordered_map<unrelocated_addr, call_site *>;
> 
> This is another case of replacing a set with a map.
> I wonder what the expense is.  You'd have to examine a large program
> built with optimization to really know.
> 
> Tom

I checked an optimized build of GDB, and it contained 861737
instances of DW_TAG_call_site (although not all in the same compunit,
of course).  So, possibly quite a lot.

I prototyped it, and it's possible to use a set instead, it looks
similar to the patch I sent for die_info.  I will include it in the next
version of the series.

Simon
diff mbox series

Patch

diff --git a/gdb/dwarf2/call-site.h b/gdb/dwarf2/call-site.h
index 0a0c7e83b813..bbf459770a37 100644
--- a/gdb/dwarf2/call-site.h
+++ b/gdb/dwarf2/call-site.h
@@ -25,6 +25,7 @@ 
 #include "dwarf2/types.h"
 #include "../frame.h"
 #include "gdbsupport/function-view.h"
+#include "gdbsupport/unordered_map.h"
 
 struct dwarf2_locexpr_baton;
 struct dwarf2_per_cu_data;
@@ -241,4 +242,6 @@  struct call_site
   struct call_site_parameter parameter[];
 };
 
+using call_site_htab_t = gdb::unordered_map<unrelocated_addr, call_site *>;
+
 #endif /* CALL_SITE_H */
diff --git a/gdb/dwarf2/cu.h b/gdb/dwarf2/cu.h
index dc11be0e7d79..e917ae1ff6d3 100644
--- a/gdb/dwarf2/cu.h
+++ b/gdb/dwarf2/cu.h
@@ -172,7 +172,7 @@  struct dwarf2_cu
   std::vector<delayed_method_info> method_list;
 
   /* To be copied to symtab->call_site_htab.  */
-  htab_up call_site_htab;
+  call_site_htab_t 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 4813dc23bb91..227ad19aa832 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -10183,7 +10183,6 @@  read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
   struct objfile *objfile = per_objfile->objfile;
   struct gdbarch *gdbarch = objfile->arch ();
   struct attribute *attr;
-  void **slot;
   int nparams;
   struct die_info *child_die;
 
@@ -10203,21 +10202,6 @@  read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
     }
   unrelocated_addr pc = attr->as_address ();
 
-  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.get (), &call_site_local, INSERT);
-  if (*slot != NULL)
-    {
-      complaint (_("Duplicate PC %s for DW_TAG_call_site "
-		   "DIE %s [in module %s]"),
-		 paddress (gdbarch, (CORE_ADDR) pc), sect_offset_str (die->sect_off),
-		 objfile_name (objfile));
-      return;
-    }
-
   /* Count parameters at the caller.  */
 
   nparams = 0;
@@ -10242,7 +10226,16 @@  read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
 		      struct call_site,
 		      sizeof (*call_site) + sizeof (call_site->parameter[0]) * nparams))
     struct call_site (pc, cu->per_cu, per_objfile);
-  *slot = call_site;
+  
+  bool inserted = cu->call_site_htab.emplace (pc, call_site).second;
+  if (!inserted)
+    {
+      complaint (_("Duplicate PC %s for DW_TAG_call_site "
+		   "DIE %s [in module %s]"),
+		 paddress (gdbarch, (CORE_ADDR) pc), sect_offset_str (die->sect_off),
+		 objfile_name (objfile));
+      return;
+    }
 
   /* We never call the destructor of call_site, so we must ensure it is
      trivially destructible.  */
diff --git a/gdb/symtab.c b/gdb/symtab.c
index ce5e2520bd17..bde537114203 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -396,17 +396,10 @@  linetable_entry::pc (const struct objfile *objfile) const
 call_site *
 compunit_symtab::find_call_site (CORE_ADDR pc) const
 {
-  if (m_call_site_htab == nullptr)
-    return nullptr;
-
   CORE_ADDR delta = this->objfile ()->text_section_offset ();
-  unrelocated_addr unrelocated_pc = (unrelocated_addr) (pc - delta);
-
-  struct call_site call_site_local (unrelocated_pc, nullptr, nullptr);
-  void **slot
-    = htab_find_slot (m_call_site_htab, &call_site_local, NO_INSERT);
-  if (slot != nullptr)
-    return (call_site *) *slot;
+  auto it = m_call_site_htab->find (static_cast<unrelocated_addr> (pc - delta));
+  if (it != m_call_site_htab->end ())
+    return it->second;
 
   /* See if the arch knows another PC we should try.  On some
      platforms, GCC emits a DWARF call site that is offset from the
@@ -416,22 +409,20 @@  compunit_symtab::find_call_site (CORE_ADDR pc) const
   if (pc == new_pc)
     return nullptr;
 
-  unrelocated_pc = (unrelocated_addr) (new_pc - delta);
-  call_site new_call_site_local (unrelocated_pc, nullptr, nullptr);
-  slot = htab_find_slot (m_call_site_htab, &new_call_site_local, NO_INSERT);
-  if (slot == nullptr)
-    return nullptr;
+  it = m_call_site_htab->find (static_cast<unrelocated_addr> (new_pc - delta));
+  if (it != m_call_site_htab->end ())
+    return it->second;
 
-  return (call_site *) *slot;
+  return nullptr;
 }
 
 /* See symtab.h.  */
 
 void
-compunit_symtab::set_call_site_htab (htab_up call_site_htab)
+compunit_symtab::set_call_site_htab (call_site_htab_t &&call_site_htab)
 {
   gdb_assert (m_call_site_htab == nullptr);
-  m_call_site_htab = call_site_htab.release ();
+  m_call_site_htab = new call_site_htab_t (std::move (call_site_htab));
 }
 
 /* See symtab.h.  */
@@ -500,8 +491,7 @@  void
 compunit_symtab::finalize ()
 {
   this->forget_cached_source_info ();
-  if (m_call_site_htab != nullptr)
-    htab_delete (m_call_site_htab);
+  delete m_call_site_htab;
 }
 
 /* The relocated address of the minimal symbol, using the section
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 4197a3a164c5..68df6abb27d4 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -24,6 +24,7 @@ 
 #include <vector>
 #include <string>
 #include <set>
+#include "dwarf2/call-site.h"
 #include "gdbsupport/gdb_vecs.h"
 #include "gdbtypes.h"
 #include "gdbsupport/gdb_obstack.h"
@@ -1957,7 +1958,7 @@  struct compunit_symtab
   symtab *primary_filetab () const;
 
   /* Set m_call_site_htab.  */
-  void set_call_site_htab (htab_up call_site_htab);
+  void set_call_site_htab (call_site_htab_t &&call_site_htab);
 
   /* Find call_site info for PC.  */
   call_site *find_call_site (CORE_ADDR pc) const;
@@ -2024,7 +2025,7 @@  struct compunit_symtab
   unsigned int m_epilogue_unwind_valid : 1;
 
   /* struct call_site entries for this compilation unit or NULL.  */
-  htab_t m_call_site_htab;
+  call_site_htab_t *m_call_site_htab;
 
   /* The macro table for this symtab.  Like the blockvector, this
      is shared between different symtabs in a given compilation unit.