[PATCH 0/7] [gdb] Add superblocks range loops

[PATCH 0/7] [gdb] Add superblocks range loops

[Tom de Vries]

This series attempts to rewrite code like:
...
  while (block != NULL)
    {
      ...
      block = block->superblock ();
    }
...
using range loops.

I'm having problems with the naming, and would like some feedback on that,
hence this is an RFC.

Given these problems, I didn't rewrite a lot of code, just enough to
demonstrate the range loops.

Tom de Vries (7):
  [gdb] Use block::function_block
  [gdbsupport] Add parameterless iterator_range constructor
  [gdbsupport] Factor out base_next_iterator
  [gdb] Add block::superblocks
  [gdb] Use block::super_blocks
  [gdb] Add block::function_blocks
  [gdb] Use block::function_blocks

 gdb/ada-lang.c                           | 52 +++++++---------
 gdb/block.h                              | 78 ++++++++++++++++++++++++
 gdb/blockframe.c                         |  5 +-
 gdb/symmisc.c                            |  9 ++-
 gdb/unittests/iterator-range-selftests.c |  2 +-
 gdbsupport/iterator-range.h              |  5 ++
 gdbsupport/next-iterator.h               | 59 ++++++++++++++----
 7 files changed, 159 insertions(+), 51 deletions(-)


base-commit: 34fc62a27e1d8827aa2e64610f4e4b03e5825fc3
-- 
2.51.0

[PATCH 1/7] [gdb] Use block::function_block

[Tom de Vries]

Use block::function_block to simplify get_frame_function.
---
 gdb/blockframe.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/gdb/blockframe.c b/gdb/blockframe.c
index ce28d99c7e4..8e99da11d22 100644
--- a/gdb/blockframe.c
+++ b/gdb/blockframe.c
@@ -120,10 +120,9 @@ get_frame_function (const frame_info_ptr &frame)
   if (bl == NULL)
     return NULL;
 
-  while (bl->function () == NULL && bl->superblock () != NULL)
-    bl = bl->superblock ();
+  bl = bl->function_block ();
 
-  return bl->function ();
+  return bl == nullptr ? nullptr : bl->function ();
 }
 \f
 
-- 
2.51.0

[PATCH 2/7] [gdbsupport] Add parameterless iterator_range constructor

[Tom de Vries]

I noticed that there's no convenient way to create an empty range in
iterator_range.  Add this.
---
 gdb/unittests/iterator-range-selftests.c | 2 +-
 gdbsupport/iterator-range.h              | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/unittests/iterator-range-selftests.c b/gdb/unittests/iterator-range-selftests.c
index a05531d95e0..7c0f7f57484 100644
--- a/gdb/unittests/iterator-range-selftests.c
+++ b/gdb/unittests/iterator-range-selftests.c
@@ -67,7 +67,7 @@ test_iterator_range ()
 
   {
     /* Empty range.  */
-    auto r3 = int_array_iterator_range (end, end);
+    auto r3 = int_array_iterator_range ();
     test_iterator_range_1 (r3, nullptr, 0, end, end);
   }
 
diff --git a/gdbsupport/iterator-range.h b/gdbsupport/iterator-range.h
index 42185546f5a..98be1e67152 100644
--- a/gdbsupport/iterator-range.h
+++ b/gdbsupport/iterator-range.h
@@ -27,6 +27,11 @@ struct iterator_range
 {
   using iterator = IteratorType;
 
+  /* Create an empty range.  */
+  explicit iterator_range ()
+    : iterator_range (IteratorType {}, IteratorType {})
+  {}
+
   /* Create an iterator_range using BEGIN as the begin iterator.
 
      Assume that the end iterator can be default-constructed.  */
-- 
2.51.0

[PATCH 3/7] [gdbsupport] Factor out base_next_iterator

[Tom de Vries]

Template struct next_iterator<T> allows iterating over the "next" field of T.

I decided to generalize this to be able to use any field.

A first thought was to use macros, but nowadays we try to do things more in
native c++, so I didn't explore that further.

Instead, this patch factors out base_next_iterator, which contains all parts
not specific to "next", in other words, everything except the operator++.

Finally, I explored using a pointer-to-member template argument T::next, but
that meant that using an incomplete T was no longer allowed, so it's not
a drop-in replacement.  I also didn't explore this option further, and made
a note of it in the base_next_iterator comment.
---
 gdbsupport/next-iterator.h | 59 ++++++++++++++++++++++++++++++--------
 1 file changed, 47 insertions(+), 12 deletions(-)

diff --git a/gdbsupport/next-iterator.h b/gdbsupport/next-iterator.h
index 0c90428d349..1dee941a252 100644
--- a/gdbsupport/next-iterator.h
+++ b/gdbsupport/next-iterator.h
@@ -21,27 +21,43 @@
 
 #include "gdbsupport/iterator-range.h"
 
-/* An iterator that uses the 'next' field of a type to iterate.  This
-   can be used with various GDB types that are stored as linked
-   lists.  */
+/* An iterator base class for iterating over a field of a type.  In order to
+   form a functioning iterator, classes inheriting this should define an
+   operator++, which determines the actual field that is iterated over.
+
+   Instead of factoring out a base class, we could use something like this:
+
+     template<typename T, auto F = &T::next>
+     struct next_iterator
+     {
+       ...
+       self_type &operator++ ()
+       {
+	 m_item = m_item->*F;
+	 return *this;
+       }
+       ...
+     }
+
+  but that has the drawback that it doesn't work with incomplete T.  */
 
 template<typename T>
-struct next_iterator
+struct base_next_iterator
 {
-  typedef next_iterator self_type;
+  typedef base_next_iterator self_type;
   typedef T *value_type;
   typedef T *&reference;
   typedef T **pointer;
   typedef std::forward_iterator_tag iterator_category;
   typedef int difference_type;
 
-  explicit next_iterator (T *item)
+  explicit base_next_iterator (T *item)
     : m_item (item)
   {
   }
 
   /* Create a one-past-the-end iterator.  */
-  next_iterator ()
+  base_next_iterator ()
     : m_item (nullptr)
   {
   }
@@ -61,15 +77,34 @@ struct next_iterator
     return m_item != other.m_item;
   }
 
-  self_type &operator++ ()
+protected:
+
+  T *m_item;
+};
+
+/* An iterator that uses the 'next' field of a type to iterate.  This
+   can be used with various GDB types that are stored as linked
+   lists.  */
+
+template<typename T>
+struct next_iterator : base_next_iterator<T> {
+  typedef next_iterator self_type;
+  typedef T *value_type;
+  typedef T *&reference;
+  typedef T **pointer;
+
+  explicit next_iterator (T *item)
+    : base_next_iterator<T> (item)
   {
-    m_item = m_item->next;
-    return *this;
   }
 
-private:
+  next_iterator () = default;
 
-  T *m_item;
+  self_type &operator++ ()
+  {
+    this->m_item = this->m_item->next;
+    return *this;
+  }
 };
 
 /* A convenience wrapper to make a range type around a next_iterator.  */
-- 
2.51.0

[PATCH 4/7] [gdb] Add block::superblocks

[Tom de Vries]

Add a function block::superblocks that can be used to transform:
...
  while (block != NULL)
    {
      ...
      block = block->superblock ();
    }
...
into:
...
  for (auto b : block::super_blocks (block))
    {
      ...
    }
...

I'm not sure about the name.  It might suggest that block is not included in
the iteration, but in fact it is.

I considered block::block_and_supers () instead, but it seems a bit awkward.
---
 gdb/block.h | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/gdb/block.h b/gdb/block.h
index 091120ae2b8..4a1db79d7ed 100644
--- a/gdb/block.h
+++ b/gdb/block.h
@@ -108,6 +108,27 @@ struct blockranges
 
 struct block : public allocate_on_obstack<block>
 {
+  /* Variant of next_iterator using the superblock field instead of next.  */
+  struct superblock_iterator : base_next_iterator<const block>
+  {
+    typedef superblock_iterator self_type;
+
+    explicit superblock_iterator (value_type item)
+      : base_next_iterator (item)
+    {
+    }
+
+    superblock_iterator () = default;
+
+    self_type &operator++ ()
+    {
+      this->m_item = this->m_item->superblock ();
+      return *this;
+    }
+  };
+
+  using superblock_range = iterator_range<superblock_iterator>;
+
   /* Return this block's start address.  */
   CORE_ADDR start () const
   { return m_start; }
@@ -306,6 +327,21 @@ struct block : public allocate_on_obstack<block>
 
   struct dynamic_prop *static_link () const;
 
+  superblock_range super_blocks () const
+  {
+    superblock_range::iterator begin (this);
+
+    return superblock_range (std::move (begin));
+  }
+
+  static superblock_range super_blocks (const block *b)
+  {
+    if (b == nullptr)
+      return superblock_range ();
+
+    return b->super_blocks ();
+  }
+
   /* Return true if block A is lexically nested within this block, or
      if A and this block have the same pc range.  Return false
      otherwise.  If ALLOW_NESTED is true, then block A is considered
-- 
2.51.0

[PATCH 5/7] [gdb] Use block::super_blocks

[Tom de Vries]

Use block::super_blocks in a few locations to simplify code.
---
 gdb/ada-lang.c | 9 +++------
 gdb/symmisc.c  | 9 ++++-----
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 71a338ce17e..da6c3b04dd9 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -13825,9 +13825,7 @@ class ada_language : public language_defn
     /* Search upwards from currently selected frame (so that we can
        complete on local vars.  */
 
-    for (const block *b = get_selected_block (0);
-	 b != nullptr;
-	 b = b->superblock ())
+    for (auto b : block::super_blocks (get_selected_block (0)))
       {
 	if (b->is_static_block ())
 	  surrounding_static_block = b;   /* For elmin of dups */
@@ -13852,9 +13850,8 @@ class ada_language : public language_defn
 	auto callback = [&] (compunit_symtab *s)
 	  {
 	    QUIT;
-	    for (const block *b = s->blockvector ()->static_block ();
-		 b != nullptr;
-		 b = b->superblock ())
+	    for (auto b
+		   : block::super_blocks (s->blockvector ()->static_block ()))
 	      {
 		/* Don't do this block twice.  */
 		if (b == surrounding_static_block)
diff --git a/gdb/symmisc.c b/gdb/symmisc.c
index 89374bd8a2f..5a34986ebc2 100644
--- a/gdb/symmisc.c
+++ b/gdb/symmisc.c
@@ -961,11 +961,10 @@ block_depth (const struct block *block)
 {
   int i = 0;
 
-  while ((block = block->superblock ()) != NULL)
-    {
-      i++;
-    }
-  return i;
+  for (auto b [[maybe_unused]]: block::super_blocks (block))
+    i++;
+
+  return i - 1;
 }
 \f
 
-- 
2.51.0

[PATCH 6/7] [gdb] Add block::function_blocks

[Tom de Vries]

Add a function block::function_blocks that can be used to transform:
...
  while (block != NULL)
    {
      ...
      if (block->function () != nullptr)
        break;
      block = block->superblock ();
    }
...
into:
...
  for (auto b : block::function_blocks (block))
    {
      ...
    }
...

I'm not sure about the name.  It might suggest that it iterates over all
function blocks, but that's not the case, it only does so for the inner
function.

I considered function block::inner_function_blocks instead, but it seems a bit
awkward.
---
 gdb/block.h | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/gdb/block.h b/gdb/block.h
index 4a1db79d7ed..53cc5435576 100644
--- a/gdb/block.h
+++ b/gdb/block.h
@@ -127,7 +127,34 @@ struct block : public allocate_on_obstack<block>
     }
   };
 
+  /* Variant of next_iterator using the superblock field instead of next.  */
+  struct function_block_iterator : base_next_iterator<const block>
+  {
+    typedef function_block_iterator self_type;
+
+    explicit function_block_iterator (value_type item)
+      : base_next_iterator (item->is_global_block () || item->is_static_block ()
+			    ? nullptr : item)
+    {
+    }
+
+    function_block_iterator () = default;
+
+    self_type &operator++ ()
+    {
+      if (this->m_item->function () != nullptr)
+	{
+	  this->m_item = nullptr;
+	  return *this;
+	}
+
+      this->m_item = this->m_item->superblock ();
+      return *this;
+    }
+  };
+
   using superblock_range = iterator_range<superblock_iterator>;
+  using function_block_range = iterator_range<function_block_iterator>;
 
   /* Return this block's start address.  */
   CORE_ADDR start () const
@@ -334,6 +361,13 @@ struct block : public allocate_on_obstack<block>
     return superblock_range (std::move (begin));
   }
 
+  function_block_range function_blocks () const
+  {
+    function_block_range::iterator begin (this);
+
+    return function_block_range (std::move (begin));
+  }
+
   static superblock_range super_blocks (const block *b)
   {
     if (b == nullptr)
@@ -342,6 +376,14 @@ struct block : public allocate_on_obstack<block>
     return b->super_blocks ();
   }
 
+  static function_block_range function_blocks (const block *b)
+  {
+    if (b == nullptr)
+      return function_block_range ();
+
+    return b->function_blocks ();
+  }
+
   /* Return true if block A is lexically nested within this block, or
      if A and this block have the same pc range.  Return false
      otherwise.  If ALLOW_NESTED is true, then block A is considered
-- 
2.51.0

[PATCH 7/7] [gdb] Use block::function_blocks

[Tom de Vries]

Add a use of block::function_blocks
---
 gdb/ada-lang.c | 43 +++++++++++++++++++------------------------
 1 file changed, 19 insertions(+), 24 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index da6c3b04dd9..fa465d2439b 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -13120,32 +13120,27 @@ ada_add_exceptions_from_frame (compiled_regex *preg,
 			       const frame_info_ptr &frame,
 			       std::vector<ada_exc_info> *exceptions)
 {
-  const struct block *block = get_frame_block (frame, 0);
+  const struct block *frame_block = get_frame_block (frame, 0);
 
-  while (block != 0)
-    {
-      for (struct symbol *sym : block_iterator_range (block))
-	{
-	  switch (sym->loc_class ())
-	    {
-	    case LOC_TYPEDEF:
-	    case LOC_BLOCK:
-	    case LOC_CONST:
-	      break;
-	    default:
-	      if (ada_is_exception_sym (sym))
-		{
-		  struct ada_exc_info info = {sym->print_name (),
-					      sym->value_address ()};
+  for (auto block : block::function_blocks (frame_block))
+    for (struct symbol *sym : block_iterator_range (block))
+      {
+	switch (sym->loc_class ())
+	  {
+	  case LOC_TYPEDEF:
+	  case LOC_BLOCK:
+	  case LOC_CONST:
+	    break;
+	  default:
+	    if (ada_is_exception_sym (sym))
+	      {
+		struct ada_exc_info info = {sym->print_name (),
+					    sym->value_address ()};
 
-		  exceptions->push_back (info);
-		}
-	    }
-	}
-      if (block->function () != NULL)
-	break;
-      block = block->superblock ();
-    }
+		exceptions->push_back (info);
+	      }
+	  }
+      }
 }
 
 /* Add all exceptions defined globally whose name name match
-- 
2.51.0

Re: [PATCH 1/7] [gdb] Use block::function_block

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Use block::function_block to simplify get_frame_function.

It would be even simpler to have it use block::containing_function,
which AFAICT is identical to the current code.

Tom

Re: [PATCH 2/7] [gdbsupport] Add parameterless iterator_range constructor

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> I noticed that there's no convenient way to create an empty range in
Tom> iterator_range.  Add this.
 
Tom> +  /* Create an empty range.  */
Tom> +  explicit iterator_range ()
Tom> +    : iterator_range (IteratorType {}, IteratorType {})
Tom> +  {}

I don't think the 'explicit' is needed here.

Also there's no real need to use constructor delegation here but OTOH it
doesn't really matter I suppose.

Tom

Re: [PATCH 1/7] [gdb] Use block::function_block

[Tom de Vries]

On 4/23/26 4:33 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Use block::function_block to simplify get_frame_function.
> 
> It would be even simpler to have it use block::containing_function,
> which AFAICT is identical to the current code.
> 

Hi,

thanks for the review.

I've:
- update the patch to use block::containing_function,
- updated $subject to "[gdb] Simplify get_frame_function", and
- pushed.

Thanks,
- Tom


> Tom

Re: [PATCH 2/7] [gdbsupport] Add parameterless iterator_range constructor

[Tom de Vries]

On 4/23/26 4:34 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> I noticed that there's no convenient way to create an empty range in
> Tom> iterator_range.  Add this.
>   
> Tom> +  /* Create an empty range.  */
> Tom> +  explicit iterator_range ()
> Tom> +    : iterator_range (IteratorType {}, IteratorType {})
> Tom> +  {}
> 
> I don't think the 'explicit' is needed here.
> 
> Also there's no real need to use constructor delegation here but OTOH it
> doesn't really matter I suppose.

Thanks for the review.

I ended up committing:
...
+  /* Create an empty range.  */
+  iterator_range () = default;
...

Thanks,
- Tom

> 
> Tom

Re: [PATCH 5/7] [gdb] Use block::super_blocks

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Use block::super_blocks in a few locations to simplify code.

Tom> -    for (const block *b = get_selected_block (0);
Tom> -	 b != nullptr;
Tom> -	 b = b->superblock ())
Tom> +    for (auto b : block::super_blocks (get_selected_block (0)))

Why not "get_selected_block(0)->super_blocks ()" here?

Tom

Re: [PATCH 5/7] [gdb] Use block::super_blocks

[Tom de Vries]

On 4/24/26 6:27 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Use block::super_blocks in a few locations to simplify code.
> 
> Tom> -    for (const block *b = get_selected_block (0);
> Tom> -	 b != nullptr;
> Tom> -	 b = b->superblock ())
> Tom> +    for (auto b : block::super_blocks (get_selected_block (0)))
> 
> Why not "get_selected_block(0)->super_blocks ()" here?

Because get_selected_block (0) might be nullptr.

I actually checked with an assert:
...
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 71a338ce17e..76639685977 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -13825,6 +13825,7 @@ class ada_language : public language_defn
      /* Search upwards from currently selected frame (so that we can
         complete on local vars.  */

+    gdb_assert (get_selected_block (0) != nullptr);
      for (const block *b = get_selected_block (0);
  	 b != nullptr;
  	 b = b->superblock ())
...
and it triggers in gdb.dwarf2/ada-cold-name.exp.

Thanks,
- Tom

Re: [PATCH 4/7] [gdb] Add block::superblocks

[Jan Vrany]

> Add a function block::superblocks that can be used to transform:

Hi,

a nit: shouldn't the commit summary and message say super_blocks (with underscore)?


On Thu, 2026-04-23 at 08:35 +0200, Tom de Vries wrote:
> Add a function block::superblocks that can be used to transform:
> ...
>   while (block != NULL)
>     {
>       ...
>       block = block->superblock ();
>     }
> ...
> into:
> ...
>   for (auto b : block::super_blocks (block))
>     {
>       ...
>     }
> ...
> 
> I'm not sure about the name.  It might suggest that block is not included in
> the iteration, but in fact it is.


> I considered block::block_and_supers () instead, but it seems a bit awkward.

I'd personally prefer block::block_and_supers over block::super_blocks. It's bit
awkward, but better awkward than misleading.

> ---
>  gdb/block.h | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/gdb/block.h b/gdb/block.h
> index 091120ae2b8..4a1db79d7ed 100644
> --- a/gdb/block.h
> +++ b/gdb/block.h
> @@ -108,6 +108,27 @@ struct blockranges
> 
>  struct block : public allocate_on_obstack<block>
>  {
> +  /* Variant of next_iterator using the superblock field instead of next.  */
> +  struct superblock_iterator : base_next_iterator<const block>
> +  {
> +    typedef superblock_iterator self_type;
> +
> +    explicit superblock_iterator (value_type item)
> +      : base_next_iterator (item)
> +    {
> +    }
> +
> +    superblock_iterator () = default;
> +
> +    self_type &operator++ ()
> +    {
> +      this->m_item = this->m_item->superblock ();
> +      return *this;
> +    }
> +  };
> +
> +  using superblock_range = iterator_range<superblock_iterator>;
> +
>    /* Return this block's start address.  */
>    CORE_ADDR start () const
>    { return m_start; }
> @@ -306,6 +327,21 @@ struct block : public allocate_on_obstack<block>
> 
>    struct dynamic_prop *static_link () const;
> 
> 

I think comment would be useful here, especially as "super_blocks"
returns this block AND all its superblocks. 

> +  superblock_range super_blocks () const
> +  {
> +    superblock_range::iterator begin (this);
> +
> +    return superblock_range (std::move (begin));
> +  }
> +
> +  static superblock_range super_blocks (const block *b)
> +  {
> +    if (b == nullptr)
> +      return superblock_range ();
> +
> +    return b->super_blocks ();
> +  }
> +


Thanks! 

Jan

Re: [PATCH 3/7] [gdbsupport] Factor out base_next_iterator

[Tom de Vries]

On 4/24/26 6:21 PM, Tom Tromey wrote:
> Tom> +   Instead of factoring out a base class, we could use something like this:
> Tom> +
> Tom> +     template<typename T, auto F = &T::next>
> Tom> +     struct next_iterator
> Tom> +     {
> Tom> +       ...
> Tom> +       self_type &operator++ ()
> Tom> +       {
> Tom> +	 m_item = m_item->*F;
> Tom> +	 return *this;
> Tom> +       }
> Tom> +       ...
> Tom> +     }
> Tom> +
> Tom> +  but that has the drawback that it doesn't work with incomplete T.  */
>   
> I am curious about this because I wonder how the current code could work
> with an incomplete T -- since the code references T::next directly.

Yeah, that's a good question.

I wrote the following stand-alone demonstrator:
...
$ cat test.c
#include <cstddef>
#include <utility>
#include <iterator>
#include <cstdio>

#include "gdbsupport/next-iterator.h"

struct list;

void
print_pointer (list *l)
{
   auto begin = next_iterator<struct list> (l);
   for (auto elem : next_range<struct list> (begin))
     printf ("%p\n", elem);
}

struct list
{
   int i;
   struct list *next;
};

void
print_int (list *l)
{
   auto begin = next_iterator<struct list> (l);
   for (auto elem : next_range<struct list> (begin))
     printf ("%d\n", elem->i);
}

struct list a = { 1, nullptr };
struct list b = { 2, &a };
struct list c = { 3, &b };

int
main ()
{
   print_pointer (&c);
   print_int (&c);

   return 0;
}
$ g++ test.c -I .
$ ./a.out
0x404040
0x404030
0x404020
3
2
1
$
...

I asked an LLM about this, and it explained that this is deferred or 
delayed template instantiation at work.

Basically the operator++ is used in print_pointer, but not yet 
instantiated, so we don't have to know the details of the incomplete 
type at that point.

Thanks,
- Tom

Re: [PATCH 3/7] [gdbsupport] Factor out base_next_iterator

[Simon Marchi]

On 4/23/26 2:35 AM, Tom de Vries wrote:
> diff --git a/gdbsupport/next-iterator.h b/gdbsupport/next-iterator.h
> index 0c90428d349..1dee941a252 100644
> --- a/gdbsupport/next-iterator.h
> +++ b/gdbsupport/next-iterator.h
> @@ -21,27 +21,43 @@
>  
>  #include "gdbsupport/iterator-range.h"
>  
> -/* An iterator that uses the 'next' field of a type to iterate.  This
> -   can be used with various GDB types that are stored as linked
> -   lists.  */
> +/* An iterator base class for iterating over a field of a type.  In order to
> +   form a functioning iterator, classes inheriting this should define an
> +   operator++, which determines the actual field that is iterated over.
> +
> +   Instead of factoring out a base class, we could use something like this:
> +
> +     template<typename T, auto F = &T::next>
> +     struct next_iterator
> +     {
> +       ...
> +       self_type &operator++ ()
> +       {
> +	 m_item = m_item->*F;
> +	 return *this;
> +       }
> +       ...
> +     }
> +
> +  but that has the drawback that it doesn't work with incomplete T.  */

I'm of the opinion that this information is not really relevant here, it
belongs to the commit message.

I was initially skeptic that the version with the pointer-to-member
didn't work, so I tried it for myself and indeed, I don't think it would
work with the block and superblock_iterator relationship (at least, if
we want to have a method of block returning a
superblock_iterator/range).

While next_iterator is "iterate using the raw field `next`",
base_next_iterator is more generic just for "iterating over a field of a
type".  It looks like you defined a pretty generic class where the
derived class only needs to fill in operator++ (and perhaps a bit more
boilerplate).  But operator++ doesn't have to just follow a field, it
can have any logic in there, as function_block_iterator proves, which is
nice.  Following the raw `next` field (or another raw field of any other
name) is just one specific case.

Given that all the concrete iterate needs to provide is "given a
reference to the current element, give me the next element", I think we
could reduce the boilerplate needed at each site by having the concrete
iterator provide a functor that does the increment (much like you
defined an std::map by providing a "hash" functor, not by deriving from
std::map).

Rather than copy paste what I mean here, I pushed what I think it could
look like to the users/simark/next-iterator branch.

https://sourceware.org/git/?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/simark/next-iterator
https://sourceware.org/cgit/binutils-gdb/log/?h=users/simark/next-iterator

But just to illustrate here is how the function block iterator and
range are implemented:

  /* Iterator and range type to iterate over this block and its superblocks
     within a function.  */

  struct function_block_incrementer
  {
    const block *operator() (const block &b) const noexcept
    {
      /* If the current item is the function-defining block, we're done.  */
      if (b.function () != nullptr)
	return nullptr;

      return b.superblock ();
    }
  };

  using function_block_iterator
    = base_next_iterator<const block, function_block_incrementer>;
  using function_block_range = iterator_range<function_block_iterator>;

I kept the name `base_next_iterator`, we could perhaps find a better
name, since it's not realted to `next_iterator` more than any other
concrete instance.  Perhaps `base_iterator`, or `simple_iterator`?

> @@ -61,15 +77,34 @@ struct next_iterator
>      return m_item != other.m_item;
>    }
>  
> -  self_type &operator++ ()
> +protected:
> +
> +  T *m_item;
> +};
> +
> +/* An iterator that uses the 'next' field of a type to iterate.  This
> +   can be used with various GDB types that are stored as linked
> +   lists.  */
> +
> +template<typename T>
> +struct next_iterator : base_next_iterator<T> {
> +  typedef next_iterator self_type;
> +  typedef T *value_type;
> +  typedef T *&reference;
> +  typedef T **pointer;

I think we should use `using` instead of typedef everywhere now.

> +
> +  explicit next_iterator (T *item)
> +    : base_next_iterator<T> (item)
>    {
> -    m_item = m_item->next;
> -    return *this;
>    }
>  
> -private:
> +  next_iterator () = default;

Just noting that instead of those two constructors, you can use:

  using base_next_iterator<T>::base_next_iterator;

Simon

Re: [PATCH 4/7] [gdb] Add block::superblocks

[Simon Marchi]

On 4/23/26 2:35 AM, Tom de Vries wrote:
> +
> +    self_type &operator++ ()
> +    {
> +      this->m_item = this->m_item->superblock ();

IMO we don't need to use `this->` on members prefixed with `m_`, because
it's already clear that they are fields of this class.  I do use
`this->` on fields that are not prefixed with `m_` though, otherwise
it's not clear where it comes from.

Simon

Re: [PATCH 3/7] [gdbsupport] Factor out base_next_iterator

[Simon Marchi]

On 4/30/26 12:24 PM, Simon Marchi wrote:
> Rather than copy paste what I mean here, I pushed what I think it could
> look like to the users/simark/next-iterator branch.
> 
> https://sourceware.org/git/?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/simark/next-iterator
> https://sourceware.org/cgit/binutils-gdb/log/?h=users/simark/next-iterator
> 
> But just to illustrate here is how the function block iterator and
> range are implemented:
> 
>   /* Iterator and range type to iterate over this block and its superblocks
>      within a function.  */
> 
>   struct function_block_incrementer
>   {
>     const block *operator() (const block &b) const noexcept
>     {
>       /* If the current item is the function-defining block, we're done.  */
>       if (b.function () != nullptr)
> 	return nullptr;
> 
>       return b.superblock ();
>     }
>   };
> 
>   using function_block_iterator
>     = base_next_iterator<const block, function_block_incrementer>;
>   using function_block_range = iterator_range<function_block_iterator>;

Just a note: in your version, if you pass a global or static block to
function_block_iterator, it sets the item to nullptr right away, so you
iterate on nothing.  I couldn't do that in my version, so I chose to
assert that the block isn't global or static, which puts the
responsibillity on the caller not to call this on a global or static
block.  The only caller is in ada-lang.c, it passes the result of
get_frame_block().  I guess that a block from a frame is always in a
function (is not the global or static block), but if that assumption
isn't true, then a check would be needed in the caller.

Simon

Re: [PATCH 3/7] [gdbsupport] Factor out base_next_iterator

[Tom de Vries]

On 4/30/26 6:49 AM, Tom de Vries wrote:
> On 4/24/26 6:21 PM, Tom Tromey wrote:
>> Tom> +   Instead of factoring out a base class, we could use something 
>> like this:
>> Tom> +
>> Tom> +     template<typename T, auto F = &T::next>
>> Tom> +     struct next_iterator
>> Tom> +     {
>> Tom> +       ...
>> Tom> +       self_type &operator++ ()
>> Tom> +       {
>> Tom> +     m_item = m_item->*F;
>> Tom> +     return *this;
>> Tom> +       }
>> Tom> +       ...
>> Tom> +     }
>> Tom> +
>> Tom> +  but that has the drawback that it doesn't work with incomplete 
>> T.  */
>> I am curious about this because I wonder how the current code could work
>> with an incomplete T -- since the code references T::next directly.
> 
> Yeah, that's a good question.
> 
> I wrote the following stand-alone demonstrator:
> ...
> $ cat test.c
> #include <cstddef>
> #include <utility>
> #include <iterator>
> #include <cstdio>
> 
> #include "gdbsupport/next-iterator.h"
> 
> struct list;
> 
> void
> print_pointer (list *l)
> {
>    auto begin = next_iterator<struct list> (l);
>    for (auto elem : next_range<struct list> (begin))
>      printf ("%p\n", elem);
> }
> 
> struct list
> {
>    int i;
>    struct list *next;
> };
> 
> void
> print_int (list *l)
> {
>    auto begin = next_iterator<struct list> (l);
>    for (auto elem : next_range<struct list> (begin))
>      printf ("%d\n", elem->i);
> }
> 
> struct list a = { 1, nullptr };
> struct list b = { 2, &a };
> struct list c = { 3, &b };
> 
> int
> main ()
> {
>    print_pointer (&c);
>    print_int (&c);
> 
>    return 0;
> }
> $ g++ test.c -I .
> $ ./a.out
> 0x404040
> 0x404030
> 0x404020
> 3
> 2
> 1
> $
> ...
> 
> I asked an LLM about this, and it explained that this is deferred or 
> delayed template instantiation at work.
> 
> Basically the operator++ is used in print_pointer, but not yet 
> instantiated, so we don't have to know the details of the incomplete 
> type at that point.


I've written a next_iterator unit test, and added (part of) this 
demonstrator ( 
https://sourceware.org/pipermail/gdb-patches/2026-May/227065.html ).

Thanks,
- Tom

Re: [PATCH 3/7] [gdbsupport] Factor out base_next_iterator

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon>   The only caller is in ada-lang.c, it passes the result of
Simon> get_frame_block().  I guess that a block from a frame is always in a
Simon> function (is not the global or static block), but if that assumption
Simon> isn't true, then a check would be needed in the caller.

I think it would be a bug if a frame's block were the static block, but
OTOH we had to preserve that hack in the blockvector lookup code, so who
knows.

Tom

Re: [PATCH 3/7] [gdbsupport] Factor out base_next_iterator

[Tom de Vries]

On 4/24/26 6:21 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> Tom> Instead, this patch factors out base_next_iterator, which contains all parts
> Tom> not specific to "next", in other words, everything except the operator++.
> 
> This looks pretty reasonable to me, though I wonder if it would be
> improved using the CRTP idiom.
> 
> It's not really needed but it could possibly be cleaner.
> 
> https://en.cppreference.com/cpp/language/crtp
> 

Thanks, I've done that in a v1 ( 
https://sourceware.org/pipermail/gdb-patches/2026-May/227066.html ).

> Tom> +   Instead of factoring out a base class, we could use something like this:
> Tom> +
> Tom> +     template<typename T, auto F = &T::next>
> Tom> +     struct next_iterator
> Tom> +     {
> Tom> +       ...
> Tom> +       self_type &operator++ ()
> Tom> +       {
> Tom> +	 m_item = m_item->*F;
> Tom> +	 return *this;
> Tom> +       }
> Tom> +       ...
> Tom> +     }
> Tom> +
> Tom> +  but that has the drawback that it doesn't work with incomplete T.  */
>   
> I am curious about this because I wonder how the current code could work
> with an incomplete T -- since the code references T::next directly.
> 
> Tom> -  explicit next_iterator (T *item)
> Tom> +  explicit base_next_iterator (T *item)
> Tom>      : m_item (item)
> Tom>    {
> Tom>    }
>   
> Tom>    /* Create a one-past-the-end iterator.  */
> Tom> -  next_iterator ()
> Tom> +  base_next_iterator ()
> Tom>      : m_item (nullptr)
> Tom>    {
> Tom>    }
> 
> These should probably be protected now because it doesn't make sense to
> directly instantiate a base_next_iterator.
> 

I did that initially, but Simon suggested importing the constructor from 
base_next_iterator, and that doesn't work if it's protected.


> Tom> +template<typename T>
> Tom> +struct next_iterator : base_next_iterator<T> {
> 
> Brace placement.
> 

Fixed.

> Tom> +  typedef next_iterator self_type;
> Tom> +  typedef T *value_type;
> Tom> +  typedef T *&reference;
> Tom> +  typedef T **pointer;
> 
> I would have thought most of these would be inherited.
> (I guess with CRTP this could all be in the base class)

That's indeed what happened, these are gone now.

Thanks,
- Tom

Re: [PATCH 4/7] [gdb] Add block::superblocks

[Tom de Vries]

On 4/27/26 1:11 PM, Jan Vrany wrote:
>> Add a function block::superblocks that can be used to transform:
> 
> Hi,
> 
> a nit: shouldn't the commit summary and message say super_blocks (with underscore)?
> 
> 

Hi Jan,

yes, though now I've renamed it to blocks::block_and_superblocks, so 
that's fixed in the v1 ( 
https://sourceware.org/pipermail/gdb-patches/2026-May/227068.html ).

> On Thu, 2026-04-23 at 08:35 +0200, Tom de Vries wrote:
>> Add a function block::superblocks that can be used to transform:
>> ...
>>    while (block != NULL)
>>      {
>>        ...
>>        block = block->superblock ();
>>      }
>> ...
>> into:
>> ...
>>    for (auto b : block::super_blocks (block))
>>      {
>>        ...
>>      }
>> ...
>>
>> I'm not sure about the name.  It might suggest that block is not included in
>> the iteration, but in fact it is.
> 
> 
>> I considered block::block_and_supers () instead, but it seems a bit awkward.
> 
> I'd personally prefer block::block_and_supers over block::super_blocks. It's bit
> awkward, but better awkward than misleading.
> 

Thanks for weighing in.  As mentioned, I've gone with 
blocks::block_and_superblocks, so I hope that's clear enough.

>> ---
>>   gdb/block.h | 36 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 36 insertions(+)
>>
>> diff --git a/gdb/block.h b/gdb/block.h
>> index 091120ae2b8..4a1db79d7ed 100644
>> --- a/gdb/block.h
>> +++ b/gdb/block.h
>> @@ -108,6 +108,27 @@ struct blockranges
>>
>>   struct block : public allocate_on_obstack<block>
>>   {
>> +  /* Variant of next_iterator using the superblock field instead of next.  */
>> +  struct superblock_iterator : base_next_iterator<const block>
>> +  {
>> +    typedef superblock_iterator self_type;
>> +
>> +    explicit superblock_iterator (value_type item)
>> +      : base_next_iterator (item)
>> +    {
>> +    }
>> +
>> +    superblock_iterator () = default;
>> +
>> +    self_type &operator++ ()
>> +    {
>> +      this->m_item = this->m_item->superblock ();
>> +      return *this;
>> +    }
>> +  };
>> +
>> +  using superblock_range = iterator_range<superblock_iterator>;
>> +
>>     /* Return this block's start address.  */
>>     CORE_ADDR start () const
>>     { return m_start; }
>> @@ -306,6 +327,21 @@ struct block : public allocate_on_obstack<block>
>>
>>     struct dynamic_prop *static_link () const;
>>
>>
> 
> I think comment would be useful here, especially as "super_blocks"
> returns this block AND all its superblocks.
> 

I've added comments in the v1, thanks for pointing this out.

Thanks,
- Tom

>> +  superblock_range super_blocks () const
>> +  {
>> +    superblock_range::iterator begin (this);
>> +
>> +    return superblock_range (std::move (begin));
>> +  }
>> +
>> +  static superblock_range super_blocks (const block *b)
>> +  {
>> +    if (b == nullptr)
>> +      return superblock_range ();
>> +
>> +    return b->super_blocks ();
>> +  }
>> +
> 
> 
> Thanks!
> 
> Jan
> 
> 

Re: [PATCH 3/7] [gdbsupport] Factor out base_next_iterator

[Tom de Vries]

On 4/30/26 6:24 PM, Simon Marchi wrote:
> On 4/23/26 2:35 AM, Tom de Vries wrote:
>> diff --git a/gdbsupport/next-iterator.h b/gdbsupport/next-iterator.h
>> index 0c90428d349..1dee941a252 100644
>> --- a/gdbsupport/next-iterator.h
>> +++ b/gdbsupport/next-iterator.h
>> @@ -21,27 +21,43 @@
>>   
>>   #include "gdbsupport/iterator-range.h"
>>   
>> -/* An iterator that uses the 'next' field of a type to iterate.  This
>> -   can be used with various GDB types that are stored as linked
>> -   lists.  */
>> +/* An iterator base class for iterating over a field of a type.  In order to
>> +   form a functioning iterator, classes inheriting this should define an
>> +   operator++, which determines the actual field that is iterated over.
>> +
>> +   Instead of factoring out a base class, we could use something like this:
>> +
>> +     template<typename T, auto F = &T::next>
>> +     struct next_iterator
>> +     {
>> +       ...
>> +       self_type &operator++ ()
>> +       {
>> +	 m_item = m_item->*F;
>> +	 return *this;
>> +       }
>> +       ...
>> +     }
>> +
>> +  but that has the drawback that it doesn't work with incomplete T.  */
> 

Hi Simon,

thanks for the review.

> I'm of the opinion that this information is not really relevant here, it
> belongs to the commit message.
> 

Ack, I've moved that to the commit message in a v1 ( 
https://sourceware.org/pipermail/gdb-patches/2026-May/227066.html ).

> I was initially skeptic that the version with the pointer-to-member
> didn't work, so I tried it for myself and indeed, I don't think it would
> work with the block and superblock_iterator relationship (at least, if
> we want to have a method of block returning a
> superblock_iterator/range).
> 

That was not what I was getting at.  The problem is that the gdb build 
breaks with the pointer-to-member approach because we exploit this 
incomplete type behavior in the sources.

> While next_iterator is "iterate using the raw field `next`",
> base_next_iterator is more generic just for "iterating over a field of a
> type".  It looks like you defined a pretty generic class where the
> derived class only needs to fill in operator++ (and perhaps a bit more
> boilerplate).  But operator++ doesn't have to just follow a field, it
> can have any logic in there, as function_block_iterator proves, which is
> nice.  Following the raw `next` field (or another raw field of any other
> name) is just one specific case.
> 
> Given that all the concrete iterate needs to provide is "given a
> reference to the current element, give me the next element", I think we
> could reduce the boilerplate needed at each site by having the concrete
> iterator provide a functor that does the increment (much like you
> defined an std::map by providing a "hash" functor, not by deriving from
> std::map).
> 

Tom Tromey suggested using CRTP, and doing so has I think brought it 
closer to what you suggest here.

> Rather than copy paste what I mean here, I pushed what I think it could
> look like to the users/simark/next-iterator branch.
> 
> https://sourceware.org/git/?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/simark/next-iterator
> https://sourceware.org/cgit/binutils-gdb/log/?h=users/simark/next-iterator
> 
> But just to illustrate here is how the function block iterator and
> range are implemented:
> 
>    /* Iterator and range type to iterate over this block and its superblocks
>       within a function.  */
> 
>    struct function_block_incrementer
>    {
>      const block *operator() (const block &b) const noexcept
>      {
>        /* If the current item is the function-defining block, we're done.  */
>        if (b.function () != nullptr)
> 	return nullptr;
> 
>        return b.superblock ();
>      }
>    };
> 
>    using function_block_iterator
>      = base_next_iterator<const block, function_block_incrementer>;
>    using function_block_range = iterator_range<function_block_iterator>;
> 
> I kept the name `base_next_iterator`, we could perhaps find a better
> name, since it's not realted to `next_iterator` more than any other
> concrete instance.  Perhaps `base_iterator`, or `simple_iterator`?
> 

Yeah, agreed.  I kept it as is in v1 thought.

>> @@ -61,15 +77,34 @@ struct next_iterator
>>       return m_item != other.m_item;
>>     }
>>   
>> -  self_type &operator++ ()
>> +protected:
>> +
>> +  T *m_item;
>> +};
>> +
>> +/* An iterator that uses the 'next' field of a type to iterate.  This
>> +   can be used with various GDB types that are stored as linked
>> +   lists.  */
>> +
>> +template<typename T>
>> +struct next_iterator : base_next_iterator<T> {
>> +  typedef next_iterator self_type;
>> +  typedef T *value_type;
>> +  typedef T *&reference;
>> +  typedef T **pointer;
> 
> I think we should use `using` instead of typedef everywhere now.
> 

I've added a patch to the series doing this transformation in 
next-iterator.h and one other file.

>> +
>> +  explicit next_iterator (T *item)
>> +    : base_next_iterator<T> (item)
>>     {
>> -    m_item = m_item->next;
>> -    return *this;
>>     }
>>   
>> -private:
>> +  next_iterator () = default;
> 
> Just noting that instead of those two constructors, you can use:
> 
>    using base_next_iterator<T>::base_next_iterator;

Yeah, that's neat, thanks, I've used that in the v1.

Thanks,
- Tom

Re: [PATCH 4/7] [gdb] Add block::superblocks

[Tom de Vries]

On 4/30/26 6:24 PM, Simon Marchi wrote:
> On 4/23/26 2:35 AM, Tom de Vries wrote:
>> +
>> +    self_type &operator++ ()
>> +    {
>> +      this->m_item = this->m_item->superblock ();
> 
> IMO we don't need to use `this->` on members prefixed with `m_`, because
> it's already clear that they are fields of this class.  I do use
> `this->` on fields that are not prefixed with `m_` though, otherwise
> it's not clear where it comes from.

I've fixed this.

In next_iterator, there was a similar pattern, but I couldn't just drop 
the this prefix.  I managed to fix this by adding "using ...::m_item":
...
template<typename T>
struct next_iterator : base_next_iterator<T, next_iterator<T>>
{
   using base_next_iterator<T, next_iterator<T>>::base_next_iterator;
   using base_next_iterator<T, next_iterator<T>>::m_item;

   T *next ()
   {
     return m_item->next;
   }
};
...

Thanks,
- Tom

Re: [PATCH 3/7] [gdbsupport] Factor out base_next_iterator

[Tom de Vries]

On 4/30/26 9:09 PM, Simon Marchi wrote:
> On 4/30/26 12:24 PM, Simon Marchi wrote:
>> Rather than copy paste what I mean here, I pushed what I think it could
>> look like to the users/simark/next-iterator branch.
>>
>> https://sourceware.org/git/?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/simark/next-iterator
>> https://sourceware.org/cgit/binutils-gdb/log/?h=users/simark/next-iterator
>>
>> But just to illustrate here is how the function block iterator and
>> range are implemented:
>>
>>    /* Iterator and range type to iterate over this block and its superblocks
>>       within a function.  */
>>
>>    struct function_block_incrementer
>>    {
>>      const block *operator() (const block &b) const noexcept
>>      {
>>        /* If the current item is the function-defining block, we're done.  */
>>        if (b.function () != nullptr)
>> 	return nullptr;
>>
>>        return b.superblock ();
>>      }
>>    };
>>
>>    using function_block_iterator
>>      = base_next_iterator<const block, function_block_incrementer>;
>>    using function_block_range = iterator_range<function_block_iterator>;
> 
> Just a note: in your version, if you pass a global or static block to
> function_block_iterator, it sets the item to nullptr right away, so you
> iterate on nothing.  I couldn't do that in my version, so I chose to
> assert that the block isn't global or static, which puts the
> responsibillity on the caller not to call this on a global or static
> block.  The only caller is in ada-lang.c, it passes the result of
> get_frame_block().  I guess that a block from a frame is always in a
> function (is not the global or static block), but if that assumption
> isn't true, then a check would be needed in the caller.

I think letting the iterator sort this out is the better option.

Thanks,
- Tom