[PATCH 1/3] gdb/block: make find_iterator_compunit_symtab a method of block_iterator

[PATCH 1/3] gdb/block: make find_iterator_compunit_symtab a method of block_iterator

[simon.marchi]

From: Simon Marchi <simon.marchi@efficios.com>

I noticed some code in bkscm_print_block_syms_progress_smob peeking a
bit in the implementation details of block_iterator, and doing
essentially the same thing as the existing static function
find_iterator_compunit_symtab.

Change find_iterator_compunit_symtab to become
block_iterator::compunit_symtab, and use it in
bkscm_print_block_syms_progress_smob.

Change-Id: I344a2155a2edb5d278e98684c71f1ee161e8d1d4
---
 gdb/block.c           | 19 ++++++++-----------
 gdb/block.h           |  4 ++++
 gdb/guile/scm-block.c |  7 ++-----
 3 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/gdb/block.c b/gdb/block.c
index 0ee29d9e3f26..fa2246383d1e 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -440,15 +440,14 @@ initialize_block_iterator (const struct block *block,
     }
 }
 
-/* A helper function that finds the current compunit over whose static
-   or global block we should iterate.  */
+/* See block.h.  */
 
-static struct compunit_symtab *
-find_iterator_compunit_symtab (struct block_iterator *iterator)
+compunit_symtab *
+block_iterator::compunit_symtab () const
 {
-  if (iterator->idx == -1)
-    return iterator->d.compunit_symtab;
-  return iterator->d.compunit_symtab->includes[iterator->idx];
+  if (this->idx == -1)
+    return this->d.compunit_symtab;
+  return this->d.compunit_symtab->includes[this->idx];
 }
 
 /* Perform a single step for a plain block iterator, iterating across
@@ -466,8 +465,7 @@ block_iterator_step (struct block_iterator *iterator, int first)
     {
       if (first)
 	{
-	  struct compunit_symtab *cust
-	    = find_iterator_compunit_symtab (iterator);
+	  compunit_symtab *cust = iterator->compunit_symtab ();
 	  const struct block *block;
 
 	  /* Iteration is complete.  */
@@ -508,8 +506,7 @@ block_iter_match_step (struct block_iterator *iterator,
     {
       if (first)
 	{
-	  struct compunit_symtab *cust
-	    = find_iterator_compunit_symtab (iterator);
+	  compunit_symtab *cust = iterator->compunit_symtab ();
 	  const struct block *block;
 
 	  /* Iteration is complete.  */
diff --git a/gdb/block.h b/gdb/block.h
index c3d060af92fb..535db5ced51d 100644
--- a/gdb/block.h
+++ b/gdb/block.h
@@ -553,6 +553,10 @@ extern const struct block *block_for_pc_sect (CORE_ADDR, struct obj_section *);
 
 struct block_iterator
 {
+/* Return the compunit over whose static or global block the iterator currently
+   iterates.  Return nullptr is the iteration is finished.  */
+  struct compunit_symtab *compunit_symtab () const;
+
   /* If we're iterating over a single block, this holds the block.
      Otherwise, it holds the canonical compunit.  */
 
diff --git a/gdb/guile/scm-block.c b/gdb/guile/scm-block.c
index c1edd2b5782a..5db73ec917e1 100644
--- a/gdb/guile/scm-block.c
+++ b/gdb/guile/scm-block.c
@@ -535,16 +535,13 @@ bkscm_print_block_syms_progress_smob (SCM self, SCM port,
 	case GLOBAL_BLOCK:
 	case STATIC_BLOCK:
 	  {
-	    struct compunit_symtab *cust;
-
 	    gdbscm_printf (port, " %s",
 			   i_smob->iter.which == GLOBAL_BLOCK
 			   ? "global" : "static");
 	    if (i_smob->iter.idx != -1)
 	      gdbscm_printf (port, " @%d", i_smob->iter.idx);
-	    cust = (i_smob->iter.idx == -1
-		    ? i_smob->iter.d.compunit_symtab
-		    : i_smob->iter.d.compunit_symtab->includes[i_smob->iter.idx]);
+
+	    compunit_symtab *cust = i_smob->iter.compunit_symtab ();
 	    gdbscm_printf (port, " %s",
 			   symtab_to_filename_for_display
 			     (cust->primary_filetab ()));

base-commit: be3c73b4610a7ccf915daca693565bc895f3584e
-- 
2.52.0

[PATCH 2/3] gdb/symtab: make compunit_symtab::includes a std::vector

[simon.marchi]

From: Simon Marchi <simon.marchi@efficios.com>

Since compunit_symtab is now a properly constructed and destructed
object, we can use fancy C++ things in it.  Change the includes field to
be an std::vector.

Previously, the includes list was NULL-terminated.  I went through all
users and I'm pretty sure that none of them rely on it being
NULL-terminated anymore.

Change-Id: Ie68a8dc0f227fd49c291d85c3e8e020463e9d0d4
---
 gdb/block.c       | 11 +++++++++--
 gdb/dwarf2/read.c | 37 +++++++++----------------------------
 gdb/symmisc.c     | 42 ++++++++++++++----------------------------
 gdb/symtab.h      |  5 ++---
 4 files changed, 34 insertions(+), 61 deletions(-)

diff --git a/gdb/block.c b/gdb/block.c
index fa2246383d1e..e61122410650 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -426,7 +426,7 @@ initialize_block_iterator (const struct block *block,
      functions.  If there are no included symtabs, we only need to
      search a single block, so we might as well just do that
      directly.  */
-  if (cu->includes == NULL)
+  if (cu->includes.empty ())
     {
       iter->d.block = block;
       /* A signal value meaning that we're iterating over a single
@@ -447,7 +447,14 @@ block_iterator::compunit_symtab () const
 {
   if (this->idx == -1)
     return this->d.compunit_symtab;
-  return this->d.compunit_symtab->includes[this->idx];
+
+  auto &includes = this->d.compunit_symtab->includes;
+
+  if (this->idx < includes.size ())
+    return includes[this->idx];
+
+  /* Iteration is complete.  */
+  return nullptr;
 }
 
 /* Perform a single step for a plain block iterator, iterating across
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 036ec7e6337f..94fa3919b885 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -2117,24 +2117,16 @@ static struct compunit_symtab *
 recursively_find_pc_sect_compunit_symtab (struct compunit_symtab *cust,
 					  CORE_ADDR pc)
 {
-  int i;
-
   if (cust->blockvector () != nullptr && cust->blockvector ()->contains (pc))
     return cust;
 
-  if (cust->includes == NULL)
-    return NULL;
-
-  for (i = 0; cust->includes[i]; ++i)
-    {
-      struct compunit_symtab *s = cust->includes[i];
-
-      s = recursively_find_pc_sect_compunit_symtab (s, pc);
-      if (s != NULL)
-	return s;
-    }
+  for (compunit_symtab *include : cust->includes)
+    if (compunit_symtab *found
+	  = recursively_find_pc_sect_compunit_symtab (include, pc);
+	found != nullptr)
+      return found;
 
-  return NULL;
+  return nullptr;
 }
 
 struct compunit_symtab *
@@ -4744,8 +4736,8 @@ recursively_compute_inclusions
 				    cust);
 }
 
-/* Compute the compunit_symtab 'includes' fields for the compunit_symtab of
-   PER_CU.  */
+/* Compute compunit_symtab::includes for the compunit_symtab of PER_CU.  This
+   is the transitive closure of all the included compunit_symtabs.  */
 
 static void
 compute_compunit_symtab_includes (dwarf2_per_cu *per_cu,
@@ -4755,8 +4747,6 @@ compute_compunit_symtab_includes (dwarf2_per_cu *per_cu,
 
   if (!per_cu->imported_symtabs.empty ())
     {
-      int len;
-      std::vector<compunit_symtab *> result_symtabs;
       compunit_symtab *cust = per_objfile->get_symtab (per_cu);
 
       /* If we don't have a symtab, we can just skip this case.  */
@@ -4767,18 +4757,9 @@ compute_compunit_symtab_includes (dwarf2_per_cu *per_cu,
       gdb::unordered_set<compunit_symtab *> all_type_symtabs;
 
       for (dwarf2_per_cu *ptr : per_cu->imported_symtabs)
-	recursively_compute_inclusions (&result_symtabs, all_children,
+	recursively_compute_inclusions (&cust->includes, all_children,
 					all_type_symtabs, ptr,
 					per_objfile, cust);
-
-      /* Now we have a transitive closure of all the included symtabs.  */
-      len = result_symtabs.size ();
-      cust->includes
-	= XOBNEWVEC (&per_objfile->objfile->objfile_obstack,
-		     struct compunit_symtab *, len + 1);
-      memcpy (cust->includes, result_symtabs.data (),
-	      len * sizeof (compunit_symtab *));
-      cust->includes[len] = NULL;
     }
 }
 
diff --git a/gdb/symmisc.c b/gdb/symmisc.c
index 838c2a002e15..258489fe8f07 100644
--- a/gdb/symmisc.c
+++ b/gdb/symmisc.c
@@ -362,21 +362,13 @@ dump_symtab_1 (struct symtab *symtab, struct ui_file *outfile)
       struct compunit_symtab *cust = symtab->compunit ();
 
       if (cust->user != nullptr)
-	{
-	  const char *addr
-	    = host_address_to_string (cust->user->primary_filetab ());
-	  gdb_printf (outfile, _("Compunit user: %s\n"), addr);
-	}
-      if (cust->includes != nullptr)
-	for (int i = 0; ; ++i)
-	  {
-	    struct compunit_symtab *include = cust->includes[i];
-	    if (include == nullptr)
-	      break;
-	    const char *addr
-	      = host_address_to_string (include->primary_filetab ());
-	    gdb_printf (outfile, _("Compunit include: %s\n"), addr);
-	  }
+	gdb_printf (outfile, _("Compunit user: %s\n"),
+		    host_address_to_string (cust->user->primary_filetab ()));
+
+
+      for (compunit_symtab *include : cust->includes)
+	gdb_printf (outfile, _("Compunit include: %s\n"),
+		    host_address_to_string (include->primary_filetab ()));
     }
 }
 
@@ -816,21 +808,15 @@ maintenance_info_symtabs (const char *regexp, int from_tty)
 				    cust.user != nullptr
 				    ? host_address_to_string (cust.user)
 				    : "(null)");
-			if (cust.includes != nullptr)
+			if (!cust.includes.empty ())
 			  {
 			    gdb_printf (_("    ( includes\n"));
-			    for (int i = 0; ; ++i)
-			      {
-				struct compunit_symtab *include
-				  = cust.includes[i];
-				if (include == nullptr)
-				  break;
-				const char *addr
-				  = host_address_to_string (include);
-				gdb_printf ("      (%s %s)\n",
-					    "(struct compunit_symtab *)",
-					    addr);
-			      }
+
+			    for (compunit_symtab *include : cust.includes)
+			      gdb_printf ("      (%s %s)\n",
+					  "(struct compunit_symtab *)",
+					  host_address_to_string (include));
+
 			    gdb_printf ("    )\n");
 			  }
 			printed_compunit_symtab_start = 1;
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 6a1320aabad8..8f0cc728410d 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2008,14 +2008,13 @@ struct compunit_symtab : intrusive_list_node<compunit_symtab>
      the given compilation unit, but it currently is.  */
   struct macro_table *m_macro_table = nullptr;
 
-  /* If non-NULL, then this points to a NULL-terminated vector of
-     included compunits.  When searching the static or global
+  /* Vector of included compunit symtabs.  When searching the static or global
      block of this compunit, the corresponding block of all
      included compunits will also be searched.  Note that this
      list must be flattened -- the symbol reader is responsible for
      ensuring that this vector contains the transitive closure of all
      included compunits.  */
-  struct compunit_symtab **includes = nullptr;
+  std::vector<compunit_symtab *> includes;
 
   /* If this is an included compunit, this points to one includer
      of the table.  This user is considered the canonical compunit
-- 
2.52.0

Re: [PATCH 2/3] gdb/symtab: make compunit_symtab::includes a std::vector

[Tom Tromey]

>>>>> "Simon" == simon marchi <simon.marchi@polymtl.ca> writes:

Simon> From: Simon Marchi <simon.marchi@efficios.com>
Simon> Since compunit_symtab is now a properly constructed and destructed
Simon> object, we can use fancy C++ things in it.  Change the includes field to
Simon> be an std::vector.

Simon> Previously, the includes list was NULL-terminated.  I went through all
Simon> users and I'm pretty sure that none of them rely on it being
Simon> NULL-terminated anymore.

Looks good.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

[PATCH 3/3] gdb/block: bool-ify some parameters

[simon.marchi]

From: Simon Marchi <simon.marchi@efficios.com>

Change-Id: Ia89872ea4388c909bc2bf1eaf3bf900a68e1e53b
---
 gdb/block.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/gdb/block.c b/gdb/block.c
index e61122410650..64ca59d44a1d 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -462,7 +462,7 @@ block_iterator::compunit_symtab () const
    iteration is complete.  */
 
 static struct symbol *
-block_iterator_step (struct block_iterator *iterator, int first)
+block_iterator_step (struct block_iterator *iterator, bool first)
 {
   struct symbol *sym;
 
@@ -503,7 +503,7 @@ block_iterator_step (struct block_iterator *iterator, int first)
 
 static struct symbol *
 block_iter_match_step (struct block_iterator *iterator,
-		       int first)
+		       bool first)
 {
   struct symbol *sym;
 
@@ -553,14 +553,14 @@ block_iterator_first (const struct block *block,
 	return mdict_iterator_first (block->multidict (),
 				     &iterator->mdict_iter);
 
-      return block_iterator_step (iterator, 1);
+      return block_iterator_step (iterator, true);
     }
 
   if (iterator->which == FIRST_LOCAL_BLOCK)
     return mdict_iter_match_first (block->multidict (), *name,
 				   &iterator->mdict_iter);
 
-  return block_iter_match_step (iterator, 1);
+  return block_iter_match_step (iterator, true);
 }
 
 /* See block.h.  */
@@ -573,13 +573,13 @@ block_iterator_next (struct block_iterator *iterator)
       if (iterator->which == FIRST_LOCAL_BLOCK)
 	return mdict_iterator_next (&iterator->mdict_iter);
 
-      return block_iterator_step (iterator, 0);
+      return block_iterator_step (iterator, false);
     }
 
   if (iterator->which == FIRST_LOCAL_BLOCK)
     return mdict_iter_match_next (*iterator->name, &iterator->mdict_iter);
 
-  return block_iter_match_step (iterator, 0);
+  return block_iter_match_step (iterator, false);
 }
 
 /* See block.h.  */
-- 
2.52.0

[PATCH] gdb/dwarf2: don't search included symtabs recursively

[Simon Marchi]

This patch depends on this mini-series that converts
compunit_symtab::includes to a vector:

  https://inbox.sourceware.org/gdb-patches/20260130025546.322629-1-simon.marchi@polymtl.ca/

The compunit_symtab::includes field says that it contains the flattened
list of all recursively included symtabs:

  /* Vector of included compunit symtabs.  When searching the static or global
     block of this compunit, the corresponding block of all
     included compunits will also be searched.  Note that this
     list must be flattened -- the symbol reader is responsible for
     ensuring that this vector contains the transitive closure of all
     included compunits.  */
  std::vector<compunit_symtab *> includes;

The DWARF reader appears to do exactly that, see
recursively_compute_inclusions.

It therefore seems unnecessary to do a recursive search, in
recursively_find_pc_sect_compunit_symtab, it will search some symtabs
multiple times.  I confirmed this by hacking the function to print the
searched CUs and to never find anything:

    diff --git i/gdb/dwarf2/read.c w/gdb/dwarf2/read.c
    index aeed0fa13a46..098fecb06cd9 100644
    --- i/gdb/dwarf2/read.c
    +++ w/gdb/dwarf2/read.c
    @@ -2114,8 +2114,7 @@ static struct compunit_symtab *
     recursively_find_pc_sect_compunit_symtab (struct compunit_symtab *cust,
     					  CORE_ADDR pc)
     {
    -  if (cust->blockvector () != nullptr && cust->blockvector ()->contains (pc))
    -    return cust;
    +  printf ("\nSearch CU %p\n", cust);

       for (compunit_symtab *include : cust->includes)
         if (compunit_symtab *found

And then:

    $ ./gdb -nx  --data-directory=data-directory ~/build/babeltrace/src/lib/.libs/libbabeltrace2.so.0.0.0 -ex "p bt_common_assert_failed" -batch | grep 'Search CU' | sort | uniq -c
          1 Search CU 0x7c83249e44b0
          1 Search CU 0x7c83249e4690
          1 Search CU 0x7c83249e4870
          1 Search CU 0x7c83249e4960
          1 Search CU 0x7c83249e4b40
          1 Search CU 0x7c83249e4d20
          1 Search CU 0x7c83249e4f00
          2 Search CU 0x7c83249e4ff0
          2 Search CU 0x7c83249e50e0
          4 Search CU 0x7c83249e51d0
          2 Search CU 0x7c83249e52c0
          2 Search CU 0x7c83249e53b0
          4 Search CU 0x7c83249e54a0

Change recursively_find_pc_sect_compunit_symtab to only search the
flattened list, and rename it accordingly.

With the patch, the same hack (putting the print in the "is_the_one"
lambda) and command as above shows that all CUs are searched exactly
once:

      1 Search CU 0x7cc78ffe44b0
      1 Search CU 0x7cc78ffe4690
      1 Search CU 0x7cc78ffe4870
      1 Search CU 0x7cc78ffe4960
      1 Search CU 0x7cc78ffe4b40
      1 Search CU 0x7cc78ffe4d20
      1 Search CU 0x7cc78ffe4f00
      1 Search CU 0x7cc78ffe4ff0
      1 Search CU 0x7cc78ffe50e0
      1 Search CU 0x7cc78ffe51d0
      1 Search CU 0x7cc78ffe52c0
      1 Search CU 0x7cc78ffe53b0
      1 Search CU 0x7cc78ffe54a0

I am not sure how blockvectors work exactly, whether the blockvector of
the includer CU is a superset of the blockvectors of the includees.  I
ask that because it seems like in practice, the requested PC always
seems to be found in the first searched CU.  I haven't investigated this
point more than that though.

Change-Id: I4064f9ddf6131226593ac864f0f804460201502e
---
 gdb/dwarf2/read.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index aeed0fa13a46..75fd737f6a66 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -2110,18 +2110,21 @@ dw_search_file_matcher
 /* A helper for dw2_find_pc_sect_compunit_symtab which finds the most specific
    symtab.  */
 
-static struct compunit_symtab *
-recursively_find_pc_sect_compunit_symtab (struct compunit_symtab *cust,
-					  CORE_ADDR pc)
+static compunit_symtab *
+find_pc_sect_compunit_symtab_includes (compunit_symtab *cust, CORE_ADDR pc)
 {
-  if (cust->blockvector () != nullptr && cust->blockvector ()->contains (pc))
+  auto is_the_one = [pc] (compunit_symtab *one_cust)
+    {
+      return (one_cust->blockvector () != nullptr
+	      && one_cust->blockvector ()->contains (pc));
+    };
+
+  if (is_the_one (cust))
     return cust;
 
   for (compunit_symtab *include : cust->includes)
-    if (compunit_symtab *found
-	  = recursively_find_pc_sect_compunit_symtab (include, pc);
-	found != nullptr)
-      return found;
+    if (is_the_one (include))
+      return include;
 
   return nullptr;
 }
@@ -2134,8 +2137,6 @@ dwarf2_base_index_functions::find_pc_sect_compunit_symtab
       struct obj_section *section,
       int warn_if_readin)
 {
-  struct compunit_symtab *result;
-
   dwarf2_per_objfile *per_objfile = get_dwarf2_per_objfile (objfile);
   dwarf2_per_bfd *per_bfd = per_objfile->per_bfd;
 
@@ -2152,7 +2153,7 @@ dwarf2_base_index_functions::find_pc_sect_compunit_symtab
     warning (_("(Internal error: pc %s in read in CU, but not in symtab.)"),
 	     paddress (objfile->arch (), pc));
 
-  result = recursively_find_pc_sect_compunit_symtab
+  compunit_symtab *result = find_pc_sect_compunit_symtab_includes
     (dw2_instantiate_symtab (data, per_objfile, false), pc);
 
   if (warn_if_readin && result == nullptr)

base-commit: 637751817dd2e9a5dff9fc03b5917ecc4faeaf4a
-- 
2.53.0

Re: [PATCH] gdb/dwarf2: don't search included symtabs recursively

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> It therefore seems unnecessary to do a recursive search, in
Simon> recursively_find_pc_sect_compunit_symtab, it will search some symtabs
Simon> multiple times.

Agreed, thanks for finding this.

We discussed this elsewhere but probably what we eventually want is a
mechanism to avoid searching included symtabs more than once across an
entire search.

Simon> I am not sure how blockvectors work exactly, whether the blockvector of
Simon> the includer CU is a superset of the blockvectors of the includees.  I
Simon> ask that because it seems like in practice, the requested PC always
Simon> seems to be found in the first searched CU.  I haven't investigated this
Simon> point more than that though.

I think each compunit_symtab gets a blockvector that describes exactly
the functions defined locally in that CU.

It would be very unusual for a partial unit to really contain code.  The
reason for this is that the normal reason to make a partial unit is to
share common things between CUs, but two CUs would not normally describe
the same function with the same address ranges.  It wouldn't be invalid
to do this, just not very useful.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH] gdb/dwarf2: don't search included symtabs recursively

[Simon Marchi]

On 2/5/26 1:53 PM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> It therefore seems unnecessary to do a recursive search, in
> Simon> recursively_find_pc_sect_compunit_symtab, it will search some symtabs
> Simon> multiple times.
> 
> Agreed, thanks for finding this.
> 
> We discussed this elsewhere but probably what we eventually want is a
> mechanism to avoid searching included symtabs more than once across an
> entire search.
> 
> Simon> I am not sure how blockvectors work exactly, whether the blockvector of
> Simon> the includer CU is a superset of the blockvectors of the includees.  I
> Simon> ask that because it seems like in practice, the requested PC always
> Simon> seems to be found in the first searched CU.  I haven't investigated this
> Simon> point more than that though.
> 
> I think each compunit_symtab gets a blockvector that describes exactly
> the functions defined locally in that CU.
> 
> It would be very unusual for a partial unit to really contain code.  The
> reason for this is that the normal reason to make a partial unit is to
> share common things between CUs, but two CUs would not normally describe
> the same function with the same address ranges.  It wouldn't be invalid
> to do this, just not very useful.
> 
> Approved-By: Tom Tromey <tom@tromey.com>
> 
> Tom

Ok, so this "problem" wasn't really a problem in practice, because a PC
would always be found in the first (top-level) compunit_symtab then
(unless it's not found at all).

Pushed, thanks.

Simon

Re: [PATCH 3/3] gdb/block: bool-ify some parameters

[Tom Tromey]

>>>>> "Simon" == simon marchi <simon.marchi@polymtl.ca> writes:

Simon> From: Simon Marchi <simon.marchi@efficios.com>
Simon> Change-Id: Ia89872ea4388c909bc2bf1eaf3bf900a68e1e53b

Thanks.

Approved-By: Tom Tromey <tom@tromey.com>

Every once in a while I poke at making the block iterator stuff more
c++-ish but it's a real pain.

Tom

Re: [PATCH 1/3] gdb/block: make find_iterator_compunit_symtab a method of block_iterator

[Tom Tromey]

>>>>> "Simon" == simon marchi <simon.marchi@polymtl.ca> writes:

Simon> From: Simon Marchi <simon.marchi@efficios.com>
Simon> I noticed some code in bkscm_print_block_syms_progress_smob peeking a
Simon> bit in the implementation details of block_iterator, and doing
Simon> essentially the same thing as the existing static function
Simon> find_iterator_compunit_symtab.

Simon> Change find_iterator_compunit_symtab to become
Simon> block_iterator::compunit_symtab, and use it in
Simon> bkscm_print_block_syms_progress_smob.

Thanks.  One nit.

Simon> +/* Return the compunit over whose static or global block the iterator currently
Simon> +   iterates.  Return nullptr is the iteration is finished.  */
Simon> +  struct compunit_symtab *compunit_symtab () const;

Typo in the comment, s/is/if/ in the second sentence.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 1/3] gdb/block: make find_iterator_compunit_symtab a method of block_iterator

[Simon Marchi]

On 2/5/26 12:41 PM, Tom Tromey wrote:
>>>>>> "Simon" == simon marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> From: Simon Marchi <simon.marchi@efficios.com>
> Simon> I noticed some code in bkscm_print_block_syms_progress_smob peeking a
> Simon> bit in the implementation details of block_iterator, and doing
> Simon> essentially the same thing as the existing static function
> Simon> find_iterator_compunit_symtab.
> 
> Simon> Change find_iterator_compunit_symtab to become
> Simon> block_iterator::compunit_symtab, and use it in
> Simon> bkscm_print_block_syms_progress_smob.
> 
> Thanks.  One nit.
> 
> Simon> +/* Return the compunit over whose static or global block the iterator currently
> Simon> +   iterates.  Return nullptr is the iteration is finished.  */
> Simon> +  struct compunit_symtab *compunit_symtab () const;
> 
> Typo in the comment, s/is/if/ in the second sentence.
> 
> Approved-By: Tom Tromey <tom@tromey.com>
> 
> Tom

Thanks, pushed the series with that fixed.

Simon

Re: [PATCH 1/3] gdb/block: make find_iterator_compunit_symtab a method of block_iterator

[Tom Tromey]

>>>>> "Simon" == simon marchi <simon.marchi@polymtl.ca> writes:

Simon> +compunit_symtab *
Simon> +block_iterator::compunit_symtab () const
Simon>  {
Simon> -  if (iterator->idx == -1)
Simon> -    return iterator->d.compunit_symtab;
Simon> -  return iterator->d.compunit_symtab->includes[iterator->idx];
Simon> +  if (this->idx == -1)
Simon> +    return this->d.compunit_symtab;
Simon> +  return this->d.compunit_symtab->includes[this->idx];

I just recalled that block iterators work in a funny way -- they can
either iterate over a single block or a symtab.

And, if iterating over a single block, then it's invalid to refer to
d.compunit_symtab.

So I think this new method should probably assert like

    gdb_assert (which != FIRST_LOCAL_BLOCK);

See the comments in block_iterator in block.h.

thanks,
Tom

Re: [PATCH 1/3] gdb/block: make find_iterator_compunit_symtab a method of block_iterator

[Simon Marchi]

On 2/5/26 1:46 PM, Tom Tromey wrote:
>>>>>> "Simon" == simon marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> +compunit_symtab *
> Simon> +block_iterator::compunit_symtab () const
> Simon>  {
> Simon> -  if (iterator->idx == -1)
> Simon> -    return iterator->d.compunit_symtab;
> Simon> -  return iterator->d.compunit_symtab->includes[iterator->idx];
> Simon> +  if (this->idx == -1)
> Simon> +    return this->d.compunit_symtab;
> Simon> +  return this->d.compunit_symtab->includes[this->idx];
> 
> I just recalled that block iterators work in a funny way -- they can
> either iterate over a single block or a symtab.
> 
> And, if iterating over a single block, then it's invalid to refer to
> d.compunit_symtab.
> 
> So I think this new method should probably assert like
> 
>     gdb_assert (which != FIRST_LOCAL_BLOCK);
> 
> See the comments in block_iterator in block.h.

Yeah I saw that, but I didn't dig too much.  Because it's a functional
change, I will send it as a separate patch.

Simon