[PATCH] Use gdb::function_view in addrmap_foreach

[PATCH] Use gdb::function_view in addrmap_foreach

[Tom Tromey]

While working on the DWARF psymtab replacement, I needed
addrmap_foreach to accept a gdb::function_view.  This seemed like a
worthwhile change on its own, so I've written it separately for
submission.

Regression tested on x86-64 Fedora 32.

gdb/ChangeLog
2021-06-23  Tom Tromey  <tom@tromey.com>

	* dwarf2/index-write.c (struct addrmap_index_data): Add
	initializers.
	<operator()>: Declare.
	(addrmap_index_data::operator()): Rename from
	add_address_entry_worker.  Remove 'datap' parameter.
	(write_address_map): Update.
	* psymtab.c (struct dump_psymtab_addrmap_data): Remove
	(dump_psymtab_addrmap_1): Remove 'data' parameter, add other
	parameters.
	(dump_psymtab_addrmap): Update.
	* addrmap.c (struct addrmap_funcs) <foreach>: Remove 'data'
	parameter.
	(addrmap_foreach, addrmap_fixed_foreach): Likewise.
	(struct mutable_foreach_data): Remove.
	(addrmap_mutable_foreach_worker): Update.
	(addrmap_mutable_foreach): Remove 'data' parameter.
	* addrmap.h (addrmap_foreach_fn): Use gdb::function_view.
	(addrmap_foreach): Remove 'data' parameter.
---
 gdb/ChangeLog            | 21 +++++++++++++
 gdb/addrmap.c            | 34 ++++++---------------
 gdb/addrmap.h            | 18 ++++++-----
 gdb/dwarf2/index-write.c | 41 +++++++++++--------------
 gdb/psymtab.c            | 65 +++++++++++++++++-----------------------
 5 files changed, 86 insertions(+), 93 deletions(-)

diff --git a/gdb/addrmap.c b/gdb/addrmap.c
index f735123c408..9bd924e8382 100644
--- a/gdb/addrmap.c
+++ b/gdb/addrmap.c
@@ -41,7 +41,7 @@ struct addrmap_funcs
   struct addrmap *(*create_fixed) (struct addrmap *self,
 				   struct obstack *obstack);
   void (*relocate) (struct addrmap *self, CORE_ADDR offset);
-  int (*foreach) (struct addrmap *self, addrmap_foreach_fn fn, void *data);
+  int (*foreach) (struct addrmap *self, addrmap_foreach_fn fn);
 };
 
 
@@ -84,9 +84,9 @@ addrmap_relocate (struct addrmap *map, CORE_ADDR offset)
 
 
 int
-addrmap_foreach (struct addrmap *map, addrmap_foreach_fn fn, void *data)
+addrmap_foreach (struct addrmap *map, addrmap_foreach_fn fn)
 {
-  return map->funcs->foreach (map, fn, data);
+  return map->funcs->foreach (map, fn);
 }
 \f
 /* Fixed address maps.  */
@@ -182,15 +182,14 @@ addrmap_fixed_relocate (struct addrmap *self, CORE_ADDR offset)
 
 
 static int
-addrmap_fixed_foreach (struct addrmap *self, addrmap_foreach_fn fn,
-		       void *data)
+addrmap_fixed_foreach (struct addrmap *self, addrmap_foreach_fn fn)
 {
   struct addrmap_fixed *map = (struct addrmap_fixed *) self;
   size_t i;
 
   for (i = 0; i < map->num_transitions; i++)
     {
-      int res = fn (data, map->transitions[i].addr, map->transitions[i].value);
+      int res = fn (map->transitions[i].addr, map->transitions[i].value);
 
       if (res != 0)
 	return res;
@@ -471,39 +470,24 @@ addrmap_mutable_relocate (struct addrmap *self, CORE_ADDR offset)
 }
 
 
-/* Struct to map addrmap's foreach function to splay_tree's version.  */
-struct mutable_foreach_data
-{
-  addrmap_foreach_fn fn;
-  void *data;
-};
-
-
 /* This is a splay_tree_foreach_fn.  */
 
 static int
 addrmap_mutable_foreach_worker (splay_tree_node node, void *data)
 {
-  struct mutable_foreach_data *foreach_data
-    = (struct mutable_foreach_data *) data;
+  addrmap_foreach_fn *fn = (addrmap_foreach_fn *) data;
 
-  return foreach_data->fn (foreach_data->data,
-			   addrmap_node_key (node),
-			   addrmap_node_value (node));
+  return (*fn) (addrmap_node_key (node), addrmap_node_value (node));
 }
 
 
 static int
-addrmap_mutable_foreach (struct addrmap *self, addrmap_foreach_fn fn,
-			 void *data)
+addrmap_mutable_foreach (struct addrmap *self, addrmap_foreach_fn fn)
 {
   struct addrmap_mutable *mutable_obj = (struct addrmap_mutable *) self;
-  struct mutable_foreach_data foreach_data;
 
-  foreach_data.fn = fn;
-  foreach_data.data = data;
   return splay_tree_foreach (mutable_obj->tree, addrmap_mutable_foreach_worker,
-			     &foreach_data);
+			     &fn);
 }
 
 
diff --git a/gdb/addrmap.h b/gdb/addrmap.h
index bcefc004a03..4b1a59684b1 100644
--- a/gdb/addrmap.h
+++ b/gdb/addrmap.h
@@ -20,6 +20,8 @@
 #ifndef ADDRMAP_H
 #define ADDRMAP_H
 
+#include "gdbsupport/function-view.h"
+
 /* An address map is essentially a table mapping CORE_ADDRs onto GDB
    data structures, like blocks, symtabs, partial symtabs, and so on.
    An address map uses memory proportional to the number of
@@ -93,13 +95,13 @@ void addrmap_relocate (struct addrmap *map, CORE_ADDR offset);
 
 /* The type of a function used to iterate over the map.
    OBJ is NULL for unmapped regions.  */
-typedef int (*addrmap_foreach_fn) (void *data, CORE_ADDR start_addr,
-				   void *obj);
-
-/* Call FN, passing it DATA, for every address in MAP, following an
-   in-order traversal.  If FN ever returns a non-zero value, the
-   iteration ceases immediately, and the value is returned.
-   Otherwise, this function returns 0.  */
-int addrmap_foreach (struct addrmap *map, addrmap_foreach_fn fn, void *data);
+typedef gdb::function_view<int (CORE_ADDR start_addr, void *obj)>
+     addrmap_foreach_fn;
+
+/* Call FN for every address in MAP, following an in-order traversal.
+   If FN ever returns a non-zero value, the iteration ceases
+   immediately, and the value is returned.  Otherwise, this function
+   returns 0.  */
+int addrmap_foreach (struct addrmap *map, addrmap_foreach_fn fn);
 
 #endif /* ADDRMAP_H */
diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index 4b78add0a5a..2da222a6b5d 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -419,14 +419,16 @@ struct addrmap_index_data
   data_buf &addr_vec;
   psym_index_map &cu_index_htab;
 
+  int operator() (CORE_ADDR start_addr, void *obj);
+
   /* Non-zero if the previous_* fields are valid.
      We can't write an entry until we see the next entry (since it is only then
      that we know the end of the entry).  */
-  int previous_valid;
+  int previous_valid = 0;
   /* Index of the CU in the table of all CUs in the index file.  */
-  unsigned int previous_cu_index;
+  unsigned int previous_cu_index = 0;
   /* Start address of the CU.  */
-  CORE_ADDR previous_cu_start;
+  CORE_ADDR previous_cu_start = 0;
 };
 
 /* Write an address entry to ADDR_VEC.  */
@@ -442,27 +444,26 @@ add_address_entry (data_buf &addr_vec,
 
 /* Worker function for traversing an addrmap to build the address table.  */
 
-static int
-add_address_entry_worker (void *datap, CORE_ADDR start_addr, void *obj)
+int
+addrmap_index_data::operator() (CORE_ADDR start_addr, void *obj)
 {
-  struct addrmap_index_data *data = (struct addrmap_index_data *) datap;
   partial_symtab *pst = (partial_symtab *) obj;
 
-  if (data->previous_valid)
-    add_address_entry (data->addr_vec,
-		       data->previous_cu_start, start_addr,
-		       data->previous_cu_index);
+  if (previous_valid)
+    add_address_entry (addr_vec,
+		       previous_cu_start, start_addr,
+		       previous_cu_index);
 
-  data->previous_cu_start = start_addr;
+  previous_cu_start = start_addr;
   if (pst != NULL)
     {
-      const auto it = data->cu_index_htab.find (pst);
-      gdb_assert (it != data->cu_index_htab.cend ());
-      data->previous_cu_index = it->second;
-      data->previous_valid = 1;
+      const auto it = cu_index_htab.find (pst);
+      gdb_assert (it != cu_index_htab.cend ());
+      previous_cu_index = it->second;
+      previous_valid = 1;
     }
   else
-    data->previous_valid = 0;
+    previous_valid = 0;
 
   return 0;
 }
@@ -477,14 +478,8 @@ write_address_map (dwarf2_per_bfd *per_bfd, data_buf &addr_vec,
 {
   struct addrmap_index_data addrmap_index_data (addr_vec, cu_index_htab);
 
-  /* When writing the address table, we have to cope with the fact that
-     the addrmap iterator only provides the start of a region; we have to
-     wait until the next invocation to get the start of the next region.  */
-
-  addrmap_index_data.previous_valid = 0;
-
   addrmap_foreach (per_bfd->partial_symtabs->psymtabs_addrmap,
-		   add_address_entry_worker, &addrmap_index_data);
+		   addrmap_index_data);
 
   /* It's highly unlikely the last entry (end address = 0xff...ff)
      is valid, but we should still handle it.
diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index fe32486e2fc..cbd21b3209f 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -1465,52 +1465,39 @@ psymtab_storage::discard_psymtab (struct partial_symtab *pst)
 
 \f
 
-/* We need to pass a couple of items to the addrmap_foreach function,
-   so use a struct.  */
-
-struct dump_psymtab_addrmap_data
-{
-  struct objfile *objfile;
-  struct partial_symtab *psymtab;
-  struct ui_file *outfile;
-
-  /* Non-zero if the previously printed addrmap entry was for PSYMTAB.
-     If so, we want to print the next one as well (since the next addrmap
-     entry defines the end of the range).  */
-  int previous_matched;
-};
-
 /* Helper function for dump_psymtab_addrmap to print an addrmap entry.  */
 
 static int
-dump_psymtab_addrmap_1 (void *datap, CORE_ADDR start_addr, void *obj)
+dump_psymtab_addrmap_1 (struct objfile *objfile,
+			struct partial_symtab *psymtab,
+			struct ui_file *outfile,
+			int *previous_matched,
+			CORE_ADDR start_addr,
+			void *obj)
 {
-  struct dump_psymtab_addrmap_data *data
-    = (struct dump_psymtab_addrmap_data *) datap;
-  struct gdbarch *gdbarch = data->objfile->arch ();
+  struct gdbarch *gdbarch = objfile->arch ();
   struct partial_symtab *addrmap_psymtab = (struct partial_symtab *) obj;
   const char *psymtab_address_or_end = NULL;
 
   QUIT;
 
-  if (data->psymtab == NULL
-      || data->psymtab == addrmap_psymtab)
+  if (psymtab == NULL
+      || psymtab == addrmap_psymtab)
     psymtab_address_or_end = host_address_to_string (addrmap_psymtab);
-  else if (data->previous_matched)
+  else if (*previous_matched)
     psymtab_address_or_end = "<ends here>";
 
-  if (data->psymtab == NULL
-      || data->psymtab == addrmap_psymtab
-      || data->previous_matched)
+  if (psymtab == NULL
+      || psymtab == addrmap_psymtab
+      || *previous_matched)
     {
-      fprintf_filtered (data->outfile, "  %s%s %s\n",
-			data->psymtab != NULL ? "  " : "",
+      fprintf_filtered (outfile, "  %s%s %s\n",
+			psymtab != NULL ? "  " : "",
 			paddress (gdbarch, start_addr),
 			psymtab_address_or_end);
     }
 
-  data->previous_matched = (data->psymtab == NULL
-			    || data->psymtab == addrmap_psymtab);
+  *previous_matched = psymtab == NULL || psymtab == addrmap_psymtab;
 
   return 0;
 }
@@ -1524,20 +1511,24 @@ dump_psymtab_addrmap (struct objfile *objfile,
 		      struct partial_symtab *psymtab,
 		      struct ui_file *outfile)
 {
-  struct dump_psymtab_addrmap_data addrmap_dump_data;
-
   if ((psymtab == NULL
        || psymtab->psymtabs_addrmap_supported)
       && partial_symtabs->psymtabs_addrmap != NULL)
     {
-      addrmap_dump_data.objfile = objfile;
-      addrmap_dump_data.psymtab = psymtab;
-      addrmap_dump_data.outfile = outfile;
-      addrmap_dump_data.previous_matched = 0;
+      /* Non-zero if the previously printed addrmap entry was for
+	 PSYMTAB.  If so, we want to print the next one as well (since
+	 the next addrmap entry defines the end of the range).  */
+      int previous_matched = 0;
+
+      auto callback = [&] (CORE_ADDR start_addr, void *obj)
+      {
+	return dump_psymtab_addrmap_1 (objfile, psymtab, outfile,
+				       &previous_matched, start_addr, obj);
+      };
+
       fprintf_filtered (outfile, "%sddress map:\n",
 			psymtab == NULL ? "Entire a" : "  A");
-      addrmap_foreach (partial_symtabs->psymtabs_addrmap,
-		       dump_psymtab_addrmap_1, &addrmap_dump_data);
+      addrmap_foreach (partial_symtabs->psymtabs_addrmap, callback);
     }
 }
 
-- 
2.26.3

Re: [PATCH] Use gdb::function_view in addrmap_foreach

[Simon Marchi]

On 2021-06-23 4:44 p.m., Tom Tromey wrote:
> While working on the DWARF psymtab replacement, I needed
> addrmap_foreach to accept a gdb::function_view.  This seemed like a
> worthwhile change on its own, so I've written it separately for
> submission.

LGTM.  I guess that we could make ranged for loops work with addrmap,
but this is already an improvement.

Simon