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

[Simon Marchi <simark@simark.ca>]

On 1/27/23 11:26, Tom Tromey via Gdb-patches wrote:
> After the cooked index is created, the addrmaps should be const.  This
> is slightly complicated by the fact that addrmap::foreach is not const
> -- however, it should be.  The underlying splay tree might be
> rearranged during a foreach, but if so, this is a classic case where
> 'mutable' semantics make sense.

I tried making the parameter to addrmap_foreach_fn const too (which
makes sense I think, if we're going to make foreach const), and came up
with a slightly larger patch.  It also made the find method return
const, but it also makes sense given that it is marked const too.
See the patch below, I think we can rebase yours on top of that.

> diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
> index 09b3fd70b26..6f228cdc749 100644
> --- a/gdb/dwarf2/cooked-index.c
> +++ b/gdb/dwarf2/cooked-index.c
> @@ -401,10 +401,10 @@ cooked_index_vector::lookup (CORE_ADDR addr)
>  
>  /* See cooked-index.h.  */
>  
> -std::vector<addrmap *>
> +std::vector<const addrmap *>
>  cooked_index_vector::get_addrmaps ()

You can make the get_addrmaps method const itself.

Otherwise, LGTM.

Simon

----

From 5ca5ca3ebc138a3474e7708a5696f43a7abff0da 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: constify addrmap::find and addrmap::foreach

Constify the return type of addrmap::find.  Since find is a const
method, I think it makes sense for it to return a pointer to const,
indicating to the callers that they should not modify the returned
object.

Then, make the foreach method const, and change the parameter to
addrmap_foreach_fn to `const void *`.

Obviously, the const can be cast away, but if using static_cast
instead of C-style casts (like the change I did in
addrmap_index_data::operator()), then the compiler won't let you cast
the const away.

Change-Id: Ia8e69d022564f80d961413658fe6068edc71a094
---
 gdb/addrmap.c            | 16 ++++++++--------
 gdb/addrmap.h            | 18 +++++++++---------
 gdb/dwarf2/index-write.c | 10 ++++++----
 3 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/gdb/addrmap.c b/gdb/addrmap.c
index 0a6b2e5a25a9..43a521f38b23 100644
--- a/gdb/addrmap.c
+++ b/gdb/addrmap.c
@@ -39,7 +39,7 @@ addrmap_fixed::set_empty (CORE_ADDR start, CORE_ADDR end_inclusive,
 }
 
 
-void *
+const void *
 addrmap_fixed::find (CORE_ADDR addr) const
 {
   const struct addrmap_transition *bottom = &transitions[0];
@@ -82,7 +82,7 @@ addrmap_fixed::relocate (CORE_ADDR offset)
 
 
 int
-addrmap_fixed::foreach (addrmap_foreach_fn fn)
+addrmap_fixed::foreach (addrmap_foreach_fn fn) const
 {
   size_t i;
 
@@ -239,7 +239,7 @@ addrmap_mutable::set_empty (CORE_ADDR start, CORE_ADDR end_inclusive,
 }
 
 
-void *
+const void *
 addrmap_mutable::find (CORE_ADDR addr) const
 {
   splay_tree_node n = splay_tree_lookup (addr);
@@ -265,7 +265,7 @@ addrmap_fixed::addrmap_fixed (struct obstack *obstack, addrmap_mutable *mut)
   size_t transition_count = 0;
 
   /* Count the number of transitions in the tree.  */
-  mut->foreach ([&] (CORE_ADDR start, void *obj)
+  mut->foreach ([&] (CORE_ADDR start, const void *obj)
     {
       ++transition_count;
       return 0;
@@ -283,7 +283,7 @@ addrmap_fixed::addrmap_fixed (struct obstack *obstack, addrmap_mutable *mut)
 
   /* Copy all entries from the splay tree to the array, in order 
      of increasing address.  */
-  mut->foreach ([&] (CORE_ADDR start, void *obj)
+  mut->foreach ([&] (CORE_ADDR start, const void *obj)
     {
       transitions[num_transitions].addr = start;
       transitions[num_transitions].value = obj;
@@ -317,7 +317,7 @@ addrmap_mutable_foreach_worker (splay_tree_node node, void *data)
 
 
 int
-addrmap_mutable::foreach (addrmap_foreach_fn fn)
+addrmap_mutable::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;
 
@@ -449,7 +449,7 @@ test_addrmap ()
   CHECK_ADDRMAP_FIND (map2, array, 13, 19, nullptr);
 
   /* Iterate over both addrmaps.  */
-  auto callback = [&] (CORE_ADDR start_addr, void *obj)
+  auto callback = [&] (CORE_ADDR start_addr, const void *obj)
     {
       if (start_addr == core_addr (nullptr))
 	SELF_CHECK (obj == nullptr);
diff --git a/gdb/addrmap.h b/gdb/addrmap.h
index a5b784efaf25..008a2dfdd22d 100644
--- a/gdb/addrmap.h
+++ b/gdb/addrmap.h
@@ -36,8 +36,8 @@
 
 /* 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, const void *obj)>;
 
 /* The base class for addrmaps.  */
 struct addrmap
@@ -85,7 +85,7 @@ struct addrmap
 			  void *obj) = 0;
 
   /* Return the object associated with ADDR in MAP.  */
-  virtual void *find (CORE_ADDR addr) const = 0;
+  virtual const void *find (CORE_ADDR addr) const = 0;
 
   /* Relocate all the addresses in MAP by OFFSET.  (This can be applied
      to either mutable or immutable maps.)  */
@@ -95,7 +95,7 @@ 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;
+  virtual int foreach (addrmap_foreach_fn fn) const = 0;
 };
 
 struct addrmap_mutable;
@@ -111,9 +111,9 @@ 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;
+  const void *find (CORE_ADDR addr) const override;
   void relocate (CORE_ADDR offset) override;
-  int foreach (addrmap_foreach_fn fn) override;
+  int foreach (addrmap_foreach_fn fn) const override;
 
 private:
 
@@ -123,7 +123,7 @@ struct addrmap_fixed : public addrmap,
   struct addrmap_transition
   {
     CORE_ADDR addr;
-    void *value;
+    const void *value;
   };
 
   /* The number of transitions in TRANSITIONS.  */
@@ -149,9 +149,9 @@ 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;
+  const void *find (CORE_ADDR addr) const override;
   void relocate (CORE_ADDR offset) override;
-  int foreach (addrmap_foreach_fn fn) override;
+  int foreach (addrmap_foreach_fn fn) const override;
 
 private:
 
diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index ced58eab6612..648e51376eb5 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -433,7 +433,8 @@ write_hash_table (mapped_symtab *symtab, data_buf &output, data_buf &cpool)
     }
 }
 
-typedef std::unordered_map<dwarf2_per_cu_data *, unsigned int> cu_index_map;
+using cu_index_map
+  = std::unordered_map<const dwarf2_per_cu_data *, unsigned int>;
 
 /* Helper struct for building the address table.  */
 struct addrmap_index_data
@@ -446,7 +447,7 @@ struct addrmap_index_data
   data_buf &addr_vec;
   cu_index_map &cu_index_htab;
 
-  int operator() (CORE_ADDR start_addr, void *obj);
+  int operator() (CORE_ADDR start_addr, const void *obj);
 
   /* True if the previous_* fields are valid.
      We can't write an entry until we see the next entry (since it is only then
@@ -472,9 +473,10 @@ add_address_entry (data_buf &addr_vec,
 /* Worker function for traversing an addrmap to build the address table.  */
 
 int
-addrmap_index_data::operator() (CORE_ADDR start_addr, void *obj)
+addrmap_index_data::operator() (CORE_ADDR start_addr, const void *obj)
 {
-  dwarf2_per_cu_data *per_cu = (dwarf2_per_cu_data *) obj;
+  const dwarf2_per_cu_data *per_cu
+    = static_cast<const dwarf2_per_cu_data *> (obj);
 
   if (previous_valid)
     add_address_entry (addr_vec,

base-commit: 90a7c55706695c0afa8f8393c589cd90bdacfef4
-- 
2.39.0