[PATCH 1/2] gdb/block: remove block_iterator::d::block, remove union

[PATCH 1/2] gdb/block: remove block_iterator::d::block, remove union

[simon.marchi]

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

The block field is never used.  The block is only used to grab a
reference to the mdict, which is stored in the mdict_iterator.

Remove it and remove the union.  Leave the compunit_symtab field, whose
name unfortunately conflicts with the method.

Change-Id: I09dbc42f937eaba6c70598acca8ff355c4e5bdb9
---
 gdb/block.c |  9 +++------
 gdb/block.h | 13 ++++++-------
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/gdb/block.c b/gdb/block.c
index 64ca59d44a1d..5aa58545393b 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -407,8 +407,6 @@ initialize_block_iterator (const struct block *block,
     which = STATIC_BLOCK;
   else
     {
-      iter->d.block = block;
-
       /* A signal value meaning that we're iterating over a single
 	 block.  */
       iter->which = FIRST_LOCAL_BLOCK;
@@ -428,14 +426,13 @@ initialize_block_iterator (const struct block *block,
      directly.  */
   if (cu->includes.empty ())
     {
-      iter->d.block = block;
       /* A signal value meaning that we're iterating over a single
 	 block.  */
       iter->which = FIRST_LOCAL_BLOCK;
     }
   else
     {
-      iter->d.compunit_symtab = cu;
+      iter->compunit_symtab_ = cu;
       iter->which = which;
     }
 }
@@ -446,9 +443,9 @@ compunit_symtab *
 block_iterator::compunit_symtab () const
 {
   if (this->idx == -1)
-    return this->d.compunit_symtab;
+    return this->compunit_symtab_;
 
-  auto &includes = this->d.compunit_symtab->includes;
+  auto &includes = this->compunit_symtab_->includes;
 
   if (this->idx < includes.size ())
     return includes[this->idx];
diff --git a/gdb/block.h b/gdb/block.h
index d9a8c4b6ff89..b84ca12c35a6 100644
--- a/gdb/block.h
+++ b/gdb/block.h
@@ -557,14 +557,13 @@ struct block_iterator
    iterates.  Return nullptr if 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.  */
+  /* If iterating on a global or static blocks, iteration starts from the
+     top-level CU and then continues with the global or static blocks of all
+     the included CUs.  This field holds the compunit of the current block.
 
-  union
-  {
-    struct compunit_symtab *compunit_symtab;
-    const struct block *block;
-  } d;
+     This field is not private because block_iterator must remain trivial,
+     but treat it as private.  */
+  struct compunit_symtab *compunit_symtab_;
 
   /* If we're trying to match a name, this will be non-NULL.  */
   const lookup_name_info *name;

base-commit: 1d688bb5f48a0feddf3ca3c60af2e91f4147dc36
-- 
2.52.0

[PATCH 2/2] gdb/block: add assert in block_iterator::compunit_symtab

[simon.marchi]

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

Verify that the compunit is only accessed when the iterator is in the
mode where that field makes sense.

We could also set compunit_symtab_ even in "single block" mode, it
wouldn't hurt, but it would not be useful either.

Change-Id: I237db16bb485148f9e906b2e7238159e9f0a8585
---
 gdb/block.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/block.c b/gdb/block.c
index 5aa58545393b..730e4580a5b7 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -442,6 +442,10 @@ initialize_block_iterator (const struct block *block,
 compunit_symtab *
 block_iterator::compunit_symtab () const
 {
+  /* The compunit field is only used when iterating over global or static
+     blocks.  */
+  gdb_assert (this->which != FIRST_LOCAL_BLOCK);
+
   if (this->idx == -1)
     return this->compunit_symtab_;
 
-- 
2.52.0

Re: [PATCH 2/2] gdb/block: add assert in block_iterator::compunit_symtab

[Guinevere Larsen]

On 2/5/26 11:52 PM, simon.marchi@polymtl.ca wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
>
> Verify that the compunit is only accessed when the iterator is in the
> mode where that field makes sense.
>
> We could also set compunit_symtab_ even in "single block" mode, it
> wouldn't hurt, but it would not be useful either.
>
> Change-Id: I237db16bb485148f9e906b2e7238159e9f0a8585

Same as the previous patch

Tested-By: Guinevere Larsen <guinevere@redhat.com>

> ---
>   gdb/block.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/gdb/block.c b/gdb/block.c
> index 5aa58545393b..730e4580a5b7 100644
> --- a/gdb/block.c
> +++ b/gdb/block.c
> @@ -442,6 +442,10 @@ initialize_block_iterator (const struct block *block,
>   compunit_symtab *
>   block_iterator::compunit_symtab () const
>   {
> +  /* The compunit field is only used when iterating over global or static
> +     blocks.  */
> +  gdb_assert (this->which != FIRST_LOCAL_BLOCK);
> +
>     if (this->idx == -1)
>       return this->compunit_symtab_;
>   


-- 
Cheers,
Guinevere Larsen
It/she

Re: [PATCH 2/2] gdb/block: add assert in block_iterator::compunit_symtab

[Andrew Burgess]

simon.marchi@polymtl.ca writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> Verify that the compunit is only accessed when the iterator is in the
> mode where that field makes sense.
>
> We could also set compunit_symtab_ even in "single block" mode, it
> wouldn't hurt, but it would not be useful either.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

>
> Change-Id: I237db16bb485148f9e906b2e7238159e9f0a8585
> ---
>  gdb/block.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/gdb/block.c b/gdb/block.c
> index 5aa58545393b..730e4580a5b7 100644
> --- a/gdb/block.c
> +++ b/gdb/block.c
> @@ -442,6 +442,10 @@ initialize_block_iterator (const struct block *block,
>  compunit_symtab *
>  block_iterator::compunit_symtab () const
>  {
> +  /* The compunit field is only used when iterating over global or static
> +     blocks.  */
> +  gdb_assert (this->which != FIRST_LOCAL_BLOCK);
> +
>    if (this->idx == -1)
>      return this->compunit_symtab_;
>  
> -- 
> 2.52.0

Re: [PATCH 2/2] gdb/block: add assert in block_iterator::compunit_symtab

[Tom Tromey]

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

Simon> From: Simon Marchi <simon.marchi@efficios.com>
Simon> Verify that the compunit is only accessed when the iterator is in the
Simon> mode where that field makes sense.

Simon> We could also set compunit_symtab_ even in "single block" mode, it
Simon> wouldn't hurt, but it would not be useful either.

Thank you for doing this.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 2/2] gdb/block: add assert in block_iterator::compunit_symtab

[Simon Marchi]

On 2026-02-06 11:43, Tom Tromey wrote:
>>>>>> "Simon" == simon marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> From: Simon Marchi <simon.marchi@efficios.com>
> Simon> Verify that the compunit is only accessed when the iterator is in the
> Simon> mode where that field makes sense.
> 
> Simon> We could also set compunit_symtab_ even in "single block" mode, it
> Simon> wouldn't hurt, but it would not be useful either.
> 
> Thank you for doing this.
> Approved-By: Tom Tromey <tom@tromey.com>
> 
> Tom

Thanks to all, I will push both patches.

Simon

Re: [PATCH 1/2] gdb/block: remove block_iterator::d::block, remove union

[Guinevere Larsen]

On 2/5/26 11:52 PM, simon.marchi@polymtl.ca wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
>
> The block field is never used.  The block is only used to grab a
> reference to the mdict, which is stored in the mdict_iterator.
>
> Remove it and remove the union.  Leave the compunit_symtab field, whose
> name unfortunately conflicts with the method.
>
> Change-Id: I09dbc42f937eaba6c70598acca8ff355c4e5bdb9

This change looks harmless enough, but since I don't feel like I 
understand it well enough to properly review, best I can say is, no 
issues running the testsuite or rebuilding, so

Tested-By: Guinevere Larsen <guinevere@redhat.com>

> ---
>   gdb/block.c |  9 +++------
>   gdb/block.h | 13 ++++++-------
>   2 files changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/gdb/block.c b/gdb/block.c
> index 64ca59d44a1d..5aa58545393b 100644
> --- a/gdb/block.c
> +++ b/gdb/block.c
> @@ -407,8 +407,6 @@ initialize_block_iterator (const struct block *block,
>       which = STATIC_BLOCK;
>     else
>       {
> -      iter->d.block = block;
> -
>         /* A signal value meaning that we're iterating over a single
>   	 block.  */
>         iter->which = FIRST_LOCAL_BLOCK;
> @@ -428,14 +426,13 @@ initialize_block_iterator (const struct block *block,
>        directly.  */
>     if (cu->includes.empty ())
>       {
> -      iter->d.block = block;
>         /* A signal value meaning that we're iterating over a single
>   	 block.  */
>         iter->which = FIRST_LOCAL_BLOCK;
>       }
>     else
>       {
> -      iter->d.compunit_symtab = cu;
> +      iter->compunit_symtab_ = cu;
>         iter->which = which;
>       }
>   }
> @@ -446,9 +443,9 @@ compunit_symtab *
>   block_iterator::compunit_symtab () const
>   {
>     if (this->idx == -1)
> -    return this->d.compunit_symtab;
> +    return this->compunit_symtab_;
>   
> -  auto &includes = this->d.compunit_symtab->includes;
> +  auto &includes = this->compunit_symtab_->includes;
>   
>     if (this->idx < includes.size ())
>       return includes[this->idx];
> diff --git a/gdb/block.h b/gdb/block.h
> index d9a8c4b6ff89..b84ca12c35a6 100644
> --- a/gdb/block.h
> +++ b/gdb/block.h
> @@ -557,14 +557,13 @@ struct block_iterator
>      iterates.  Return nullptr if 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.  */
> +  /* If iterating on a global or static blocks, iteration starts from the
> +     top-level CU and then continues with the global or static blocks of all
> +     the included CUs.  This field holds the compunit of the current block.
>   
> -  union
> -  {
> -    struct compunit_symtab *compunit_symtab;
> -    const struct block *block;
> -  } d;
> +     This field is not private because block_iterator must remain trivial,
> +     but treat it as private.  */
> +  struct compunit_symtab *compunit_symtab_;
>   
>     /* If we're trying to match a name, this will be non-NULL.  */
>     const lookup_name_info *name;
>
> base-commit: 1d688bb5f48a0feddf3ca3c60af2e91f4147dc36


-- 
Cheers,
Guinevere Larsen
It/she

Re: [PATCH 1/2] gdb/block: remove block_iterator::d::block, remove union

[Simon Marchi]

On 2026-02-06 11:08, Guinevere Larsen wrote:
> On 2/5/26 11:52 PM, simon.marchi@polymtl.ca wrote:
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> The block field is never used.  The block is only used to grab a
>> reference to the mdict, which is stored in the mdict_iterator.
>>
>> Remove it and remove the union.  Leave the compunit_symtab field, whose
>> name unfortunately conflicts with the method.
>>
>> Change-Id: I09dbc42f937eaba6c70598acca8ff355c4e5bdb9
> 
> This change looks harmless enough, but since I don't feel like I understand it well enough to properly review, best I can say is, no issues running the testsuite or rebuilding, so
> 
> Tested-By: Guinevere Larsen <guinevere@redhat.com>

Thank you for checking.

Simon

Re: [PATCH 1/2] gdb/block: remove block_iterator::d::block, remove union

[Andrew Burgess]

simon.marchi@polymtl.ca writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> The block field is never used.  The block is only used to grab a
> reference to the mdict, which is stored in the mdict_iterator.

Isn't there a contradiction between these two sentences?  Either the
block is not used, or it's used to access the mdict?

Thanks,
Andrew

>
> Remove it and remove the union.  Leave the compunit_symtab field, whose
> name unfortunately conflicts with the method.
>
> Change-Id: I09dbc42f937eaba6c70598acca8ff355c4e5bdb9
> ---
>  gdb/block.c |  9 +++------
>  gdb/block.h | 13 ++++++-------
>  2 files changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/gdb/block.c b/gdb/block.c
> index 64ca59d44a1d..5aa58545393b 100644
> --- a/gdb/block.c
> +++ b/gdb/block.c
> @@ -407,8 +407,6 @@ initialize_block_iterator (const struct block *block,
>      which = STATIC_BLOCK;
>    else
>      {
> -      iter->d.block = block;
> -
>        /* A signal value meaning that we're iterating over a single
>  	 block.  */
>        iter->which = FIRST_LOCAL_BLOCK;
> @@ -428,14 +426,13 @@ initialize_block_iterator (const struct block *block,
>       directly.  */
>    if (cu->includes.empty ())
>      {
> -      iter->d.block = block;
>        /* A signal value meaning that we're iterating over a single
>  	 block.  */
>        iter->which = FIRST_LOCAL_BLOCK;
>      }
>    else
>      {
> -      iter->d.compunit_symtab = cu;
> +      iter->compunit_symtab_ = cu;
>        iter->which = which;
>      }
>  }
> @@ -446,9 +443,9 @@ compunit_symtab *
>  block_iterator::compunit_symtab () const
>  {
>    if (this->idx == -1)
> -    return this->d.compunit_symtab;
> +    return this->compunit_symtab_;
>  
> -  auto &includes = this->d.compunit_symtab->includes;
> +  auto &includes = this->compunit_symtab_->includes;
>  
>    if (this->idx < includes.size ())
>      return includes[this->idx];
> diff --git a/gdb/block.h b/gdb/block.h
> index d9a8c4b6ff89..b84ca12c35a6 100644
> --- a/gdb/block.h
> +++ b/gdb/block.h
> @@ -557,14 +557,13 @@ struct block_iterator
>     iterates.  Return nullptr if 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.  */
> +  /* If iterating on a global or static blocks, iteration starts from the
> +     top-level CU and then continues with the global or static blocks of all
> +     the included CUs.  This field holds the compunit of the current block.
>  
> -  union
> -  {
> -    struct compunit_symtab *compunit_symtab;
> -    const struct block *block;
> -  } d;
> +     This field is not private because block_iterator must remain trivial,
> +     but treat it as private.  */
> +  struct compunit_symtab *compunit_symtab_;
>  
>    /* If we're trying to match a name, this will be non-NULL.  */
>    const lookup_name_info *name;
>
> base-commit: 1d688bb5f48a0feddf3ca3c60af2e91f4147dc36
> -- 
> 2.52.0

Re: [PATCH 1/2] gdb/block: remove block_iterator::d::block, remove union

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> simon.marchi@polymtl.ca writes:
>> From: Simon Marchi <simon.marchi@efficios.com>
>> 
>> The block field is never used.  The block is only used to grab a
>> reference to the mdict, which is stored in the mdict_iterator.

Andrew> Isn't there a contradiction between these two sentences?  Either the
Andrew> block is not used, or it's used to access the mdict?

It's used when initializing the iterator but not afterward, so there's
no need to store it.

Tom

Re: [PATCH 1/2] gdb/block: remove block_iterator::d::block, remove union

[Simon Marchi]

On 2026-02-06 11:45, Tom Tromey wrote:
>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
> 
> Andrew> simon.marchi@polymtl.ca writes:
>>> From: Simon Marchi <simon.marchi@efficios.com>
>>>
>>> The block field is never used.  The block is only used to grab a
>>> reference to the mdict, which is stored in the mdict_iterator.
> 
> Andrew> Isn't there a contradiction between these two sentences?  Either the
> Andrew> block is not used, or it's used to access the mdict?
> 
> It's used when initializing the iterator but not afterward, so there's
> no need to store it.
> 
> Tom

I can see why that sentence can be confusing.  I will clarify that I am
talking about the block parameter vs the block union member.

Simon

Re: [PATCH 1/2] gdb/block: remove block_iterator::d::block, remove union

[Tom Tromey]

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

Simon> From: Simon Marchi <simon.marchi@efficios.com>
Simon> The block field is never used.  The block is only used to grab a
Simon> reference to the mdict, which is stored in the mdict_iterator.

Simon> Remove it and remove the union.  Leave the compunit_symtab field, whose
Simon> name unfortunately conflicts with the method.

Seems reasonable to me.

Using an m_ name wouldn't be remiss, we have other cases where we've
done this but where the field is still "public".

Simon>    else
Simon>      {
Simon> -      iter->d.block = block;
Simon> -

I didn't look to see how the initialization is done, but I wonder if
this should assign nullptr to the compunit_symtab field.

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

Tom

Re: [PATCH 1/2] gdb/block: remove block_iterator::d::block, remove union

[Simon Marchi]

On 2026-02-06 11:42, Tom Tromey wrote:
>>>>>> "Simon" == simon marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> From: Simon Marchi <simon.marchi@efficios.com>
> Simon> The block field is never used.  The block is only used to grab a
> Simon> reference to the mdict, which is stored in the mdict_iterator.
> 
> Simon> Remove it and remove the union.  Leave the compunit_symtab field, whose
> Simon> name unfortunately conflicts with the method.
> 
> Seems reasonable to me.
> 
> Using an m_ name wouldn't be remiss, we have other cases where we've
> done this but where the field is still "public".
> 
> Simon>    else
> Simon>      {
> Simon> -      iter->d.block = block;
> Simon> -
> 
> I didn't look to see how the initialization is done, but I wonder if
> this should assign nullptr to the compunit_symtab field.

It could, but I don't think it's necessary.  This code path puts the
iterator in FIRST_LOCAL_BLOCK mode (iterate on a single block), and it's
forbidden (protected by the assert added in the following patch) to
access the compunit when in that mode.  So it's fine if it stays in an
undefined state.

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

Thanks,

Simon

Re: [PATCH 1/2] gdb/block: remove block_iterator::d::block, remove union

[Simon Marchi]

On 2026-02-06 11:42, Tom Tromey wrote:
>>>>>> "Simon" == simon marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> From: Simon Marchi <simon.marchi@efficios.com>
> Simon> The block field is never used.  The block is only used to grab a
> Simon> reference to the mdict, which is stored in the mdict_iterator.
> 
> Simon> Remove it and remove the union.  Leave the compunit_symtab field, whose
> Simon> name unfortunately conflicts with the method.
> 
> Seems reasonable to me.
> 

Forgot to reply about this:

> Using an m_ name wouldn't be remiss, we have other cases where we've
> done this but where the field is still "public".

That is what I did at first, but then I thought that I should also
rename the other fields, which are also "internal", otherwise it's not
consistent... meh.  I thought that it could wait until someone C++ifies
this code properly.

Simon