Re: [PATCH 4/6] Change pop_context to return a block

[Simon Marchi <simark@simark.ca>]

On 4/17/26 2:24 PM, Tom Tromey wrote:
> This changes buildsym_compunit::pop_context to create and return the
> block, if needed.  It also arranges to reset some fields in the
> buildsym_compunit object to their saved values.
> 
> This also enables the removal of the set_local_using_directives
> method.
> ---
>  gdb/buildsym.c    | 29 +++++++++++++++++------------
>  gdb/buildsym.h    | 15 +++++++++------
>  gdb/dwarf2/read.c | 55 ++++++++++++++++++-------------------------------------
>  3 files changed, 44 insertions(+), 55 deletions(-)
> 
> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
> index 6d719cb25a0..592bcf8bcf0 100644
> --- a/gdb/buildsym.c
> +++ b/gdb/buildsym.c
> @@ -629,13 +629,7 @@ buildsym_compunit::end_compunit_symtab_get_static_block (CORE_ADDR end_addr,
>       the context stack.  */
>  
>    if (!m_context_stack.empty ())
> -    {
> -      struct context_stack cstk = pop_context ();
> -
> -      /* Make a block for the local symbols within.  */
> -      finish_block (cstk.name, cstk.old_blocks, NULL,
> -		    cstk.start_addr, end_addr);
> -    }
> +    pop_context (end_addr);
>    gdb_assert (m_context_stack.empty ());
>  
>    /* Executables may have out of order pending blocks; sort the
> @@ -949,14 +943,25 @@ buildsym_compunit::push_context (CORE_ADDR valu)
>    return ctx;
>  }
>  
> -/* Pop a context block.  Returns the address of the context block just
> -   popped.  */
> +/* See buildsym.h.  */
>  
> -context_stack
> -buildsym_compunit::pop_context ()
> +block *
> +buildsym_compunit::pop_context (CORE_ADDR end_addr,
> +				const struct dynamic_prop *static_link,
> +				bool required)
>  {
>    gdb_assert (!m_context_stack.empty ());
> -  context_stack result = m_context_stack.back ();
> +  context_stack cstk = m_context_stack.back ();
>    m_context_stack.pop_back ();

A nit about pre-existing code, but I think that we could std::move the
result of `.back ()`:

    context_stack cstk = std::move (m_context_stack.back ());

> +  block *result = nullptr;
> +  if (required || !m_local_symbols.empty ()
> +      || m_local_using_directives != nullptr)
> +    result = finish_block (cstk.name, cstk.old_blocks, static_link,
> +			   cstk.start_addr, end_addr);
> +
> +  m_local_symbols = std::move (cstk.locals);
> +  m_local_using_directives = cstk.local_using_directives;

My understanding is that the "current" locals and local_using_directives
vectors are not held in m_context_stack, they are held in these
m_local_symbols and m_local_using_directives fields.  An entry in
m_context_stack is a saved context, to be restored later.  I wonder if
it would simplify the code if we removed m_local_symbols and
m_local_using_directives, and said that the topmost m_context_stack
entry holds the current locals and local_using_directives.  That would
mean pushing an initial context_stack entry for the global scope, I
guess?

We wouldn't need moving in and out of m_local_symbols and
m_local_using_directives anymore.  I don't think it would simplify that
much in terms of number of lines, but it might make the code a bit
easier to follow.  Just something to explore, I think this patch is
good.

Simon