Re: [PATCH] Make addrmap const-correct in cooked index

[Simon Marchi <simark@simark.ca>]

On 1/27/23 16:18, Tom Tromey via Gdb-patches wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> -void *
> Simon> +const void *
> Simon>  addrmap_fixed::find (CORE_ADDR addr) const
> Simon>  {
> Simon>    const struct addrmap_transition *bottom = &transitions[0];
> 
> Actually, this is going to rely on casting away const in some spots.
> 
> For example cooked_index::lookup:
> 
>   dwarf2_per_cu_data *lookup (CORE_ADDR addr) const
>   {
>     return (dwarf2_per_cu_data *) m_addrmap->find (addr);
>   }
> 
> Probably we can't constify dwarf2_per_cu_data all over.

I tried constifying dwarf2_per_cu_data to see where it would break.  My
thinking is that after the quick symbol implementation has produced all
those dwarf2_per_cu_data objects, they are not supposed to change.  But
there is one ligitimate-ish case where we change one after the fact,
when we call set_length in cutu_reader::cutu_reader.

So, I redesigned my patch to instead provide both const and non-const
versions of find and foreach.  I think it's typical for containers in
C++ anyway.  See v2 below:


From 030f7e72af736f2f46c2c6c033ed1b74bce6bde8 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Fri, 27 Jan 2023 14:46:50 -0500
Subject: [PATCH] gdb: provide const-correct versions of addrmap::find and
 addrmap::foreach

Users of addrmap::find and addrmap::foreach that have a const addrmap
should ideally receive const pointers to objects, to indicate they
should not be modified.  However, users that have a non-const addrmap
should still receive a non-const pointer.

To achieve this, without adding more virtual methods, make the existing
find and foreach virtual methods private and prefix them with "do_".
Add small non-const and const wrappers for find and foreach.

Obviously, the const can be cast away, but if using static_cast
instead of C-style casts, then the compiler won't let you cast
the const away.  I changed all the callers of addrmap::find and
addrmap::foreach I could find to make them use static_cast.

Change-Id: Ia8e69d022564f80d961413658fe6068edc71a094
---
 gdb/addrmap.c             | 10 +++++-----
 gdb/addrmap.h             | 34 ++++++++++++++++++++++++++--------
 gdb/dwarf2/cooked-index.h |  2 +-
 gdb/dwarf2/index-write.c  |  2 +-
 gdb/dwarf2/read.c         | 13 +++++++------
 5 files changed, 40 insertions(+), 21 deletions(-)

diff --git a/gdb/addrmap.c b/gdb/addrmap.c
index 0a6b2e5a25a9..33dc7762d91c 100644
--- a/gdb/addrmap.c
+++ b/gdb/addrmap.c
@@ -40,7 +40,7 @@ addrmap_fixed::set_empty (CORE_ADDR start, CORE_ADDR end_inclusive,
 
 
 void *
-addrmap_fixed::find (CORE_ADDR addr) const
+addrmap_fixed::do_find (CORE_ADDR addr) const
 {
   const struct addrmap_transition *bottom = &transitions[0];
   const struct addrmap_transition *top = &transitions[num_transitions - 1];
@@ -82,7 +82,7 @@ addrmap_fixed::relocate (CORE_ADDR offset)
 
 
 int
-addrmap_fixed::foreach (addrmap_foreach_fn fn)
+addrmap_fixed::do_foreach (addrmap_foreach_fn fn) const
 {
   size_t i;
 
@@ -240,7 +240,7 @@ addrmap_mutable::set_empty (CORE_ADDR start, CORE_ADDR end_inclusive,
 
 
 void *
-addrmap_mutable::find (CORE_ADDR addr) const
+addrmap_mutable::do_find (CORE_ADDR addr) const
 {
   splay_tree_node n = splay_tree_lookup (addr);
   if (n != nullptr)
@@ -317,7 +317,7 @@ addrmap_mutable_foreach_worker (splay_tree_node node, void *data)
 
 
 int
-addrmap_mutable::foreach (addrmap_foreach_fn fn)
+addrmap_mutable::do_foreach (addrmap_foreach_fn fn) const
 {
   return splay_tree_foreach (tree, addrmap_mutable_foreach_worker, &fn);
 }
@@ -368,7 +368,7 @@ addrmap_dump (struct addrmap *map, struct ui_file *outfile, void *payload)
      addrmap entry defines the end of the range).  */
   bool previous_matched = false;
 
-  auto callback = [&] (CORE_ADDR start_addr, void *obj)
+  auto callback = [&] (CORE_ADDR start_addr, const void *obj)
   {
     QUIT;
 
diff --git a/gdb/addrmap.h b/gdb/addrmap.h
index a5b784efaf25..e00dda6e7115 100644
--- a/gdb/addrmap.h
+++ b/gdb/addrmap.h
@@ -36,8 +36,10 @@
 
 /* The type of a function used to iterate over the map.
    OBJ is NULL for unmapped regions.  */
-typedef gdb::function_view<int (CORE_ADDR start_addr, void *obj)>
-     addrmap_foreach_fn;
+using addrmap_foreach_fn
+  = gdb::function_view<int (CORE_ADDR start_addr, void *obj)>;
+using addrmap_foreach_const_fn
+  = gdb::function_view<int (CORE_ADDR start_addr, const void *obj)>;
 
 /* The base class for addrmaps.  */
 struct addrmap
@@ -85,7 +87,11 @@ struct addrmap
 			  void *obj) = 0;
 
   /* Return the object associated with ADDR in MAP.  */
-  virtual void *find (CORE_ADDR addr) const = 0;
+  const void *find (CORE_ADDR addr) const
+  { return this->do_find (addr); }
+
+  void *find (CORE_ADDR addr)
+  { return this->do_find (addr); }
 
   /* Relocate all the addresses in MAP by OFFSET.  (This can be applied
      to either mutable or immutable maps.)  */
@@ -95,7 +101,19 @@ struct addrmap
      If FN ever returns a non-zero value, the iteration ceases
      immediately, and the value is returned.  Otherwise, this function
      returns 0.  */
-  virtual int foreach (addrmap_foreach_fn fn) = 0;
+  int foreach (addrmap_foreach_const_fn fn) const
+  { return this->do_foreach (fn); }
+
+  int foreach (addrmap_foreach_fn fn)
+  { return this->do_foreach (fn); }
+
+
+private:
+  /* Worker for find, implemented by sub-classes.  */
+  virtual void *do_find (CORE_ADDR addr) const = 0;
+
+  /* Worker for foreach, implemented by sub-classes.  */
+  virtual int do_foreach (addrmap_foreach_fn fn) const = 0;
 };
 
 struct addrmap_mutable;
@@ -111,11 +129,11 @@ struct addrmap_fixed : public addrmap,
 
   void set_empty (CORE_ADDR start, CORE_ADDR end_inclusive,
 		  void *obj) override;
-  void *find (CORE_ADDR addr) const override;
   void relocate (CORE_ADDR offset) override;
-  int foreach (addrmap_foreach_fn fn) override;
 
 private:
+  void *do_find (CORE_ADDR addr) const override;
+  int do_foreach (addrmap_foreach_fn fn) const override;
 
   /* A transition: a point in an address map where the value changes.
      The map maps ADDR to VALUE, but if ADDR > 0, it maps ADDR-1 to
@@ -149,11 +167,11 @@ struct addrmap_mutable : public addrmap
 
   void set_empty (CORE_ADDR start, CORE_ADDR end_inclusive,
 		  void *obj) override;
-  void *find (CORE_ADDR addr) const override;
   void relocate (CORE_ADDR offset) override;
-  int foreach (addrmap_foreach_fn fn) override;
 
 private:
+  void *do_find (CORE_ADDR addr) const override;
+  int do_foreach (addrmap_foreach_fn fn) const override;
 
   /* A splay tree, with a node for each transition; there is a
      transition at address T if T-1 and T map to different objects.
diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index e12376cdf982..7a8c18975e5a 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -254,7 +254,7 @@ class cooked_index
      found.  */
   dwarf2_per_cu_data *lookup (CORE_ADDR addr)
   {
-    return (dwarf2_per_cu_data *) m_addrmap->find (addr);
+    return static_cast<dwarf2_per_cu_data *> (m_addrmap->find (addr));
   }
 
   /* Create a new cooked_index_entry and register it with this object.
diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index ced58eab6612..7b1b2d6520cb 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -474,7 +474,7 @@ add_address_entry (data_buf &addr_vec,
 int
 addrmap_index_data::operator() (CORE_ADDR start_addr, void *obj)
 {
-  dwarf2_per_cu_data *per_cu = (dwarf2_per_cu_data *) obj;
+  dwarf2_per_cu_data *per_cu = static_cast<dwarf2_per_cu_data *> (obj);
 
   if (previous_valid)
     add_address_entry (addr_vec,
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index cd937f24ee74..9d8952a4eb8d 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -4276,8 +4276,9 @@ dwarf2_base_index_functions::find_per_cu (dwarf2_per_bfd *per_bfd,
 {
   if (per_bfd->index_addrmap == nullptr)
     return nullptr;
-  return ((struct dwarf2_per_cu_data *)
-	  per_bfd->index_addrmap->find (adjusted_pc));
+
+  void *obj = per_bfd->index_addrmap->find (adjusted_pc);
+  return static_cast<dwarf2_per_cu_data *> (obj);
 }
 
 struct compunit_symtab *
@@ -18276,8 +18277,8 @@ cooked_indexer::scan_attributes (dwarf2_per_cu_data *scanning_per_cu,
 	  else if (*parent_entry == nullptr)
 	    {
 	      CORE_ADDR lookup = form_addr (origin_offset, origin_is_dwz);
-	      *parent_entry
-		= (cooked_index_entry *) m_die_range_map.find (lookup);
+	      void *obj = m_die_range_map.find (lookup);
+	      *parent_entry = static_cast <cooked_index_entry *> (obj);
 	    }
 
 	  unsigned int bytes_read;
@@ -18568,8 +18569,8 @@ cooked_indexer::make_index (cutu_reader *reader)
   for (const auto &entry : m_deferred_entries)
     {
       CORE_ADDR key = form_addr (entry.die_offset, m_per_cu->is_dwz);
-      cooked_index_entry *parent
-	= (cooked_index_entry *) m_die_range_map.find (key);
+      void *obj = m_die_range_map.find (key);
+      cooked_index_entry *parent = static_cast <cooked_index_entry *> (obj);
       m_index_storage->add (entry.die_offset, entry.tag, entry.flags,
 			    entry.name, parent, m_per_cu);
     }

base-commit: 35e1763185224b2bf634e19cdfd06dab0ba914e3
-- 
2.39.0