[PATCH 0/6] Some buildsym cleanups

[PATCH 0/6] Some buildsym cleanups

[Tom Tromey]

This patch series cleans up buildsym a bit, in particular the context
pushing and popping code.

Regression tested on x86-64 Fedora 43.

Signed-off-by: Tom Tromey <tromey@adacore.com>
---
Tom Tromey (6):
      Use scoped_restore for dwarf2_cu::list_in_scope
      Remove some dead code from buildsym.c
      Remove context_stack::depth
      Change pop_context to return a block
      Return void from buildsym_compunit::push_context
      Rename context_stack and make it private

 gdb/buildsym.c    |  58 +++++++++++++------------------
 gdb/buildsym.h    | 100 ++++++++++++++++++++++++++++++------------------------
 gdb/dwarf2/cu.h   |   7 +---
 gdb/dwarf2/read.c |  82 +++++++++++++++-----------------------------
 4 files changed, 108 insertions(+), 139 deletions(-)
---
base-commit: bfd118583ed69db365265915d1a48eb719e997f5
change-id: 20260417-list-in-scope-4242b4cf355b

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH 1/6] Use scoped_restore for dwarf2_cu::list_in_scope

[Tom Tromey]

Some functions in the DWARF reader temporarily set
dwarf2_cu::list_in_scope and then reset it when returning.  This patch
changes these spots to use scoped_restore.
---
 gdb/dwarf2/cu.h   |  7 +------
 gdb/dwarf2/read.c | 16 +++++-----------
 2 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/gdb/dwarf2/cu.h b/gdb/dwarf2/cu.h
index 9b90415ed64..5157c6b8fa1 100644
--- a/gdb/dwarf2/cu.h
+++ b/gdb/dwarf2/cu.h
@@ -309,12 +309,7 @@ struct dwarf2_cu
   /* The generic symbol table building routines have separate lists for
      file scope symbols and all all other scopes (local scopes).  So
      we need to select the right one to pass to add_symbol_to_list().
-     We do it by keeping a pointer to the correct list in list_in_scope.
-
-     FIXME: The original dwarf code just treated the file scope as the
-     first local scope, and all other local scopes as nested local
-     scopes, and worked fine.  Check to see if we really need to
-     distinguish these in buildsym.c.  */
+     We do it by keeping a pointer to the correct list in list_in_scope.  */
   std::vector<symbol *> *list_in_scope = nullptr;
 
   /* Storage for things with the same lifetime as this read-in
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 4035bba6e45..ebf7eeb673b 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -7478,9 +7478,8 @@ inherit_abstract_dies (struct die_info *die, struct dwarf2_cu *cu)
 
   /* We're inheriting ORIGIN's children into the scope we'd put DIE's
      symbols in.  */
-  std::vector<symbol *> *origin_previous_list_in_scope
-    = origin_cu->list_in_scope;
-  origin_cu->list_in_scope = cu->list_in_scope;
+  scoped_restore save_scope = make_scoped_restore (&origin_cu->list_in_scope,
+						   cu->list_in_scope);
 
   if (die->tag != origin_die->tag
       && !(die->tag == DW_TAG_inlined_subroutine
@@ -7629,8 +7628,6 @@ inherit_abstract_dies (struct die_info *die, struct dwarf2_cu *cu)
 	}
     }
 
-  origin_cu->list_in_scope = origin_previous_list_in_scope;
-
   if (cu != origin_cu)
     compute_delayed_physnames (origin_cu);
 }
@@ -7826,7 +7823,9 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
       attr_to_dynamic_prop (attr, die, cu, static_link, cu->addr_type ());
     }
 
-  cu->list_in_scope = &cu->get_builder ()->get_local_symbols ();
+  scoped_restore save_scope
+    = make_scoped_restore (&cu->list_in_scope,
+			   &cu->get_builder ()->get_local_symbols ());
 
   for (die_info *child_die : die->children ())
     {
@@ -7912,11 +7911,6 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
      back to building a containing block's symbol lists.  */
   cu->get_builder ()->get_local_symbols () = std::move (cstk.locals);
   cu->get_builder ()->set_local_using_directives (cstk.local_using_directives);
-
-  /* If we've finished processing a top-level function, subsequent
-     symbols go in the file symbol list.  */
-  if (cu->get_builder ()->outermost_context_p ())
-    cu->list_in_scope = &cu->get_builder ()->get_file_symbols ();
 }
 
 /* Process all the DIES contained within a lexical block scope.  Start

-- 
2.53.0

[PATCH 2/6] Remove some dead code from buildsym.c

[Tom Tromey]

This patch removes some code from buildsym.c that, according to the
comment, was only used for some SCO or maybe COFF thing.  This code is
dead now, and it was a hack anyway and probably should never have been
allowed.
---
 gdb/buildsym.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index aa95889424b..f6cc5c153c2 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -635,18 +635,8 @@ buildsym_compunit::end_compunit_symtab_get_static_block (CORE_ADDR end_addr,
       /* Make a block for the local symbols within.  */
       finish_block (cstk.name, cstk.old_blocks, NULL,
 		    cstk.start_addr, end_addr);
-
-      if (!m_context_stack.empty ())
-	{
-	  /* This is said to happen with SCO.  The old coffread.c
-	     code simply emptied the context stack, so we do the
-	     same.  FIXME: Find out why it is happening.  This is not
-	     believed to happen in most cases (even for coffread.c);
-	     it used to be an abort().  */
-	  complaint (_("Context stack not empty in end_compunit_symtab"));
-	  m_context_stack.clear ();
-	}
     }
+  gdb_assert (m_context_stack.empty ());
 
   /* Executables may have out of order pending blocks; sort the
      pending blocks.  */

-- 
2.53.0

[PATCH 3/6] Remove context_stack::depth

[Tom Tromey]

context_stack::depth is never used, and I think it's not really
useful, so this removes it.
---
 gdb/buildsym.c    |  7 +++----
 gdb/buildsym.h    | 10 +++-------
 gdb/dwarf2/read.c |  4 ++--
 3 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index f6cc5c153c2..6d719cb25a0 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -933,17 +933,16 @@ buildsym_compunit::augment_type_symtab ()
     }
 }
 
-/* Push a context block.  Args are an identifying nesting level
-   (checkable when you pop it), and the starting PC address of this
+/* Push a context block.  VALU is the starting PC address of this
    context.  */
 
 context_stack &
-buildsym_compunit::push_context (int desc, CORE_ADDR valu)
+buildsym_compunit::push_context (CORE_ADDR valu)
 {
   context_stack &ctx
     = m_context_stack.emplace_back (std::move (m_local_symbols),
 				    m_local_using_directives,
-				    m_pending_blocks, valu, desc);
+				    m_pending_blocks, valu);
 
   m_local_using_directives = nullptr;
 
diff --git a/gdb/buildsym.h b/gdb/buildsym.h
index 06d8d3c691f..70d22229f53 100644
--- a/gdb/buildsym.h
+++ b/gdb/buildsym.h
@@ -74,12 +74,11 @@ using subfile_up = std::unique_ptr<subfile>;
 struct context_stack
 {
   context_stack (std::vector<symbol *> locals, using_direct *local_using_directives,
-		 pending_block *old_blocks, CORE_ADDR start_addr, int depth)
+		 pending_block *old_blocks, CORE_ADDR start_addr)
     : locals (std::move (locals)),
       local_using_directives (local_using_directives),
       old_blocks (old_blocks),
-      start_addr (start_addr),
-      depth (depth)
+      start_addr (start_addr)
   {}
 
   /* Outer locals at the time we entered.  */
@@ -96,9 +95,6 @@ struct context_stack
 
   /* PC where this context starts.  */
   CORE_ADDR start_addr;
-
-  /* For error-checking matching push/pop.  */
-  int depth;
 };
 
 /* Flags associated with a linetable entry.  */
@@ -245,7 +241,7 @@ struct buildsym_compunit
     m_producer = producer;
   }
 
-  context_stack &push_context (int desc, CORE_ADDR valu);
+  context_stack &push_context (CORE_ADDR valu);
 
   context_stack pop_context ();
 
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index ebf7eeb673b..e1d70ea650d 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -7801,7 +7801,7 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
     }
 
   gdb_assert (cu->get_builder () != nullptr);
-  context_stack &ctx = cu->get_builder ()->push_context (0, lowpc);
+  context_stack &ctx = cu->get_builder ()->push_context (lowpc);
   ctx.name = new_symbol (die, read_type_die (die, cu), cu, templ_func);
 
   if (dwarf2_func_is_main_p (die, cu))
@@ -7953,7 +7953,7 @@ read_lexical_block_scope (struct die_info *die, struct dwarf2_cu *cu)
   lowpc = per_objfile->relocate (unrel_low);
   highpc = per_objfile->relocate (unrel_high);
 
-  cu->get_builder ()->push_context (0, lowpc);
+  cu->get_builder ()->push_context (lowpc);
   for (die_info *child_die : die->children ())
     process_die (child_die, cu);
 

-- 
2.53.0

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

[Tom Tromey]

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 ();
+
+  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;
+
   return result;
 }
diff --git a/gdb/buildsym.h b/gdb/buildsym.h
index 70d22229f53..d1199735abb 100644
--- a/gdb/buildsym.h
+++ b/gdb/buildsym.h
@@ -189,11 +189,6 @@ struct buildsym_compunit
     return &m_local_using_directives;
   }
 
-  void set_local_using_directives (struct using_direct *new_local)
-  {
-    m_local_using_directives = new_local;
-  }
-
   struct using_direct **get_global_using_directives ()
   {
     return &m_global_using_directives;
@@ -243,7 +238,15 @@ struct buildsym_compunit
 
   context_stack &push_context (CORE_ADDR valu);
 
-  context_stack pop_context ();
+  /* Pop a context and create the corresponding block.  Returns the
+     block.  END_ADDR is the final address of the block.  STATIC_LINK,
+     if provided, is the static link.  REQUIRED controls whether the
+     block is required.  When false, if the block does not contain any
+     variables or 'using' directives, this method will return
+     nullptr.  */
+  block *pop_context (CORE_ADDR end_addr,
+		      const struct dynamic_prop *static_link = nullptr,
+		      bool required = true);
 
   struct block *end_compunit_symtab_get_static_block
     (CORE_ADDR end_addr, bool expandable, bool required);
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index e1d70ea650d..4447923ae3e 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -7715,7 +7715,6 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
   CORE_ADDR highpc;
   struct attribute *attr, *call_line, *call_file;
   const char *name;
-  struct block *block;
   int inlined_func = (die->tag == DW_TAG_inlined_subroutine);
   std::vector<struct symbol *> template_args;
   struct template_symbol *templ_func = NULL;
@@ -7802,17 +7801,18 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
 
   gdb_assert (cu->get_builder () != nullptr);
   context_stack &ctx = cu->get_builder ()->push_context (lowpc);
-  ctx.name = new_symbol (die, read_type_die (die, cu), cu, templ_func);
+  symbol *func_sym = new_symbol (die, read_type_die (die, cu), cu, templ_func);
+  ctx.name = func_sym;
 
   if (dwarf2_func_is_main_p (die, cu))
-    set_objfile_main_name (objfile, ctx.name->linkage_name (),
+    set_objfile_main_name (objfile, func_sym->linkage_name (),
 			   cu->lang ());
 
   /* If there is a location expression for DW_AT_frame_base, record
      it.  */
   attr = dwarf2_attr (die, DW_AT_frame_base, cu);
   if (attr != nullptr)
-    dwarf2_symbol_mark_computed (attr, ctx.name, cu, 1);
+    dwarf2_symbol_mark_computed (attr, func_sym, cu, 1);
 
   /* If there is a location for the static link, record it.  */
   dynamic_prop *static_link = nullptr;
@@ -7864,11 +7864,7 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
 	}
     }
 
-  struct context_stack cstk = cu->get_builder ()->pop_context ();
-
-  /* Make a block for the local symbols within.  */
-  block = cu->get_builder ()->finish_block (cstk.name, cstk.old_blocks,
-					    static_link, lowpc, highpc);
+  block *block = cu->get_builder ()->pop_context (highpc, static_link);
 
   /* For C++, set the block's scope.  */
   if ((cu->lang () == language_cplus
@@ -7882,7 +7878,7 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
   /* If we have address ranges, record them.  */
   dwarf2_record_block_ranges (die, block, cu);
 
-  gdbarch_make_symbol_special (gdbarch, cstk.name, objfile);
+  gdbarch_make_symbol_special (gdbarch, func_sym, objfile);
 
   /* Attach template arguments to function.  */
   if (!template_args.empty ())
@@ -7904,13 +7900,6 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
       for (symbol *sym : template_args)
 	sym->set_symtab (templ_func->symtab ());
     }
-
-  /* In C++, we can have functions nested inside functions (e.g., when
-     a function declares a class that has methods).  This means that
-     when we finish processing a function scope, we may need to go
-     back to building a containing block's symbol lists.  */
-  cu->get_builder ()->get_local_symbols () = std::move (cstk.locals);
-  cu->get_builder ()->set_local_using_directives (cstk.local_using_directives);
 }
 
 /* Process all the DIES contained within a lexical block scope.  Start
@@ -7958,29 +7947,21 @@ read_lexical_block_scope (struct die_info *die, struct dwarf2_cu *cu)
     process_die (child_die, cu);
 
   inherit_abstract_dies (die, cu);
-  struct context_stack cstk = cu->get_builder ()->pop_context ();
 
-  if (!cu->get_builder ()->get_local_symbols ().empty ()
-      || (*cu->get_builder ()->get_local_using_directives ()) != NULL)
-    {
-      struct block *block
-	= cu->get_builder ()->finish_block (0, cstk.old_blocks, NULL,
-				     cstk.start_addr, highpc);
+  block *block = cu->get_builder ()->pop_context (highpc, nullptr, false);
 
-      /* Note that recording ranges after traversing children, as we
-	 do here, means that recording a parent's ranges entails
-	 walking across all its children's ranges as they appear in
-	 the address map, which is quadratic behavior.
+  /* Note that recording ranges after traversing children, as we
+     do here, means that recording a parent's ranges entails
+     walking across all its children's ranges as they appear in
+     the address map, which is quadratic behavior.
 
-	 It would be nicer to record the parent's ranges before
-	 traversing its children, simply overriding whatever you find
-	 there.  But since we don't even decide whether to create a
-	 block until after we've traversed its children, that's hard
-	 to do.  */
-      dwarf2_record_block_ranges (die, block, cu);
-    }
-  cu->get_builder ()->get_local_symbols () = std::move (cstk.locals);
-  cu->get_builder ()->set_local_using_directives (cstk.local_using_directives);
+     It would be nicer to record the parent's ranges before
+     traversing its children, simply overriding whatever you find
+     there.  But since we don't even decide whether to create a
+     block until after we've traversed its children, that's hard
+     to do.  */
+  if (block != nullptr)
+    dwarf2_record_block_ranges (die, block, cu);
 }
 
 static void dwarf2_ranges_read_low_addrs

-- 
2.53.0

[PATCH 5/6] Return void from buildsym_compunit::push_context

[Tom Tromey]

There is one caller that uses the result of
buildsym_compunit::push_context.  This patch changes this method to
return void and changes that spot to instead call a new methods on
buildsym_compunit.

This patch also removes the get_current_context_stack method in favor
of a new method that checks the exact condition needed by the one
caller.

This patch enables a subsequent cleanup; in particular now the
'context_stack' type isn't used outside of buildsym.
---
 gdb/buildsym.c    | 12 ++++--------
 gdb/buildsym.h    | 21 ++++++++++++++++-----
 gdb/dwarf2/read.c | 15 +++++++--------
 3 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index 592bcf8bcf0..f5cc7de324d 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -930,17 +930,13 @@ buildsym_compunit::augment_type_symtab ()
 /* Push a context block.  VALU is the starting PC address of this
    context.  */
 
-context_stack &
+void
 buildsym_compunit::push_context (CORE_ADDR valu)
 {
-  context_stack &ctx
-    = m_context_stack.emplace_back (std::move (m_local_symbols),
-				    m_local_using_directives,
-				    m_pending_blocks, valu);
-
+  m_context_stack.emplace_back (std::move (m_local_symbols),
+				m_local_using_directives,
+				m_pending_blocks, valu);
   m_local_using_directives = nullptr;
-
-  return ctx;
 }
 
 /* See buildsym.h.  */
diff --git a/gdb/buildsym.h b/gdb/buildsym.h
index d1199735abb..02c411b053d 100644
--- a/gdb/buildsym.h
+++ b/gdb/buildsym.h
@@ -199,11 +199,22 @@ struct buildsym_compunit
     return m_context_stack.empty ();
   }
 
-  struct context_stack *get_current_context_stack ()
+  /* Return true if the lexical context currently being constructed
+     has a symbol, false otherwise.  */
+  bool current_context_has_function () const
   {
-    if (m_context_stack.empty ())
-      return nullptr;
-    return &m_context_stack.back ();
+    return (!m_context_stack.empty ()
+	    && m_context_stack.back ().name != nullptr);
+  }
+
+  /* Set the symbol on the lexical context currently being
+     constructed.  */
+  void set_current_context_function (symbol *fun)
+  {
+    gdb_assert (!m_context_stack.empty ());
+    gdb_assert (m_context_stack.back ().name == nullptr);
+    gdb_assert (fun != nullptr);
+    m_context_stack.back ().name = fun;
   }
 
   struct subfile *get_current_subfile ()
@@ -236,7 +247,7 @@ struct buildsym_compunit
     m_producer = producer;
   }
 
-  context_stack &push_context (CORE_ADDR valu);
+  void push_context (CORE_ADDR valu);
 
   /* Pop a context and create the corresponding block.  Returns the
      block.  END_ADDR is the final address of the block.  STATIC_LINK,
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 4447923ae3e..2a330655c48 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -7799,10 +7799,11 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
 	}
     }
 
-  gdb_assert (cu->get_builder () != nullptr);
-  context_stack &ctx = cu->get_builder ()->push_context (lowpc);
+  buildsym_compunit *builder = cu->get_builder ();
+  gdb_assert (builder != nullptr);
+  builder->push_context (lowpc);
   symbol *func_sym = new_symbol (die, read_type_die (die, cu), cu, templ_func);
-  ctx.name = func_sym;
+  builder->set_current_context_function (func_sym);
 
   if (dwarf2_func_is_main_p (die, cu))
     set_objfile_main_name (objfile, func_sym->linkage_name (),
@@ -7825,7 +7826,7 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
 
   scoped_restore save_scope
     = make_scoped_restore (&cu->list_in_scope,
-			   &cu->get_builder ()->get_local_symbols ());
+			   &builder->get_local_symbols ());
 
   for (die_info *child_die : die->children ())
     {
@@ -7864,7 +7865,7 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
 	}
     }
 
-  block *block = cu->get_builder ()->pop_context (highpc, static_link);
+  block *block = builder->pop_context (highpc, static_link);
 
   /* For C++, set the block's scope.  */
   if ((cu->lang () == language_cplus
@@ -15785,9 +15786,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 	       pretend it's a local variable in that case so that the user can
 	       still see it.  */
 	    sym->set_domain (VAR_DOMAIN);
-	    struct context_stack *curr
-	      = cu->get_builder ()->get_current_context_stack ();
-	    if (curr != nullptr && curr->name != nullptr)
+	    if (cu->get_builder ()->current_context_has_function ())
 	      sym->set_is_argument (true);
 	    attr = dwarf2_attr (die, DW_AT_location, cu);
 	    if (attr != nullptr)

-- 
2.53.0

[PATCH 6/6] Rename context_stack and make it private

[Tom Tromey]

"context_stack" has been misnamed at least since the storage was
changed to a std::vector, and arguably even since the very beginning.

This patch renames it to "lexical_context", which is a bit closer to
what is is for.

This type is also no longer used outside of buildsym itself -- callers
can now push and pop contexts, but they don't act on the context
object itself.  So, the type is made private.

One benefit of this approach is that callers no longer need to be
quite as careful -- previously there was at least a possibility that a
context object pointer would be invalidated when pushing and popping
the stack.
---
 gdb/buildsym.c |  2 +-
 gdb/buildsym.h | 60 +++++++++++++++++++++++++++++-----------------------------
 2 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index f5cc7de324d..44621a4c776 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -947,7 +947,7 @@ buildsym_compunit::pop_context (CORE_ADDR end_addr,
 				bool required)
 {
   gdb_assert (!m_context_stack.empty ());
-  context_stack cstk = m_context_stack.back ();
+  lexical_context cstk = m_context_stack.back ();
   m_context_stack.pop_back ();
 
   block *result = nullptr;
diff --git a/gdb/buildsym.h b/gdb/buildsym.h
index 02c411b053d..c9010665bce 100644
--- a/gdb/buildsym.h
+++ b/gdb/buildsym.h
@@ -68,35 +68,6 @@ struct subfile
 
 using subfile_up = std::unique_ptr<subfile>;
 
-/* Stack representing unclosed lexical contexts (that will become
-   blocks, eventually).  */
-
-struct context_stack
-{
-  context_stack (std::vector<symbol *> locals, using_direct *local_using_directives,
-		 pending_block *old_blocks, CORE_ADDR start_addr)
-    : locals (std::move (locals)),
-      local_using_directives (local_using_directives),
-      old_blocks (old_blocks),
-      start_addr (start_addr)
-  {}
-
-  /* Outer locals at the time we entered.  */
-  std::vector<symbol *> locals;
-
-  /* Pending using directives at the time we entered.  */
-  using_direct *local_using_directives;
-
-  /* Pointer into blocklist as of entry.  */
-  pending_block *old_blocks;
-
-  /* Name of function, if any, defining context.  */
-  symbol *name = nullptr;
-
-  /* PC where this context starts.  */
-  CORE_ADDR start_addr;
-};
-
 /* Flags associated with a linetable entry.  */
 
 enum linetable_entry_flag : unsigned
@@ -340,9 +311,38 @@ struct buildsym_compunit
   /* Global "using" directives.  */
   struct using_direct *m_global_using_directives = nullptr;
 
+  /* Unclosed lexical contexts (that will become blocks,
+     eventually).  */
+  struct lexical_context
+  {
+    lexical_context (std::vector<symbol *> locals,
+		     using_direct *local_using_directives,
+		     pending_block *old_blocks, CORE_ADDR start_addr)
+      : locals (std::move (locals)),
+	local_using_directives (local_using_directives),
+	old_blocks (old_blocks),
+	start_addr (start_addr)
+    {}
+
+    /* Outer locals at the time we entered.  */
+    std::vector<symbol *> locals;
+
+    /* Pending using directives at the time we entered.  */
+    using_direct *local_using_directives;
+
+    /* Pointer into blocklist as of entry.  */
+    pending_block *old_blocks;
+
+    /* Name of function, if any, defining context.  */
+    symbol *name = nullptr;
+
+    /* PC where this context starts.  */
+    CORE_ADDR start_addr;
+  };
+
   /* The stack of contexts that are pushed by push_context and popped
      by pop_context.  */
-  std::vector<struct context_stack> m_context_stack;
+  std::vector<lexical_context> m_context_stack;
 
   struct subfile *m_current_subfile = nullptr;
 

-- 
2.53.0

Re: [PATCH 2/6] Remove some dead code from buildsym.c

[Simon Marchi]

On 4/17/26 2:24 PM, Tom Tromey wrote:
> This patch removes some code from buildsym.c that, according to the
> comment, was only used for some SCO or maybe COFF thing.  This code is
> dead now, and it was a hack anyway and probably should never have been
> allowed.
> ---
>  gdb/buildsym.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
> index aa95889424b..f6cc5c153c2 100644
> --- a/gdb/buildsym.c
> +++ b/gdb/buildsym.c
> @@ -635,18 +635,8 @@ buildsym_compunit::end_compunit_symtab_get_static_block (CORE_ADDR end_addr,
>        /* Make a block for the local symbols within.  */
>        finish_block (cstk.name, cstk.old_blocks, NULL,
>  		    cstk.start_addr, end_addr);
> -
> -      if (!m_context_stack.empty ())
> -	{
> -	  /* This is said to happen with SCO.  The old coffread.c
> -	     code simply emptied the context stack, so we do the
> -	     same.  FIXME: Find out why it is happening.  This is not
> -	     believed to happen in most cases (even for coffread.c);
> -	     it used to be an abort().  */
> -	  complaint (_("Context stack not empty in end_compunit_symtab"));
> -	  m_context_stack.clear ();
> -	}
>      }

I wonder if the whole if above is still useful.  It seems to me like
debug readers (pretty much just DWARF nowadays) should not leave a
function scope open.  And I guess that the DWARF reader goes not.  I did
put a gdb_assert_not_reached in there and ran a few tests and it didn't
trigger.  In other words, I wonder if you could remove that if and just
keep the

  gdb_assert (m_context_stack.empty ());

Simon

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

[Simon Marchi]

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

Re: [PATCH 5/6] Return void from buildsym_compunit::push_context

[Simon Marchi]

On 4/17/26 2:24 PM, Tom Tromey wrote:
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 4447923ae3e..2a330655c48 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -7799,10 +7799,11 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>  	}
>      }
>  
> -  gdb_assert (cu->get_builder () != nullptr);
> -  context_stack &ctx = cu->get_builder ()->push_context (lowpc);
> +  buildsym_compunit *builder = cu->get_builder ();
> +  gdb_assert (builder != nullptr);
> +  builder->push_context (lowpc);
>    symbol *func_sym = new_symbol (die, read_type_die (die, cu), cu, templ_func);
> -  ctx.name = func_sym;
> +  builder->set_current_context_function (func_sym);

It might be tricky depending on how new_symbol uses the current context
when reading a DW_TAG_subprogram, but I wonder if we could call
new_symbol first, then pass the resulting symbol to push_context.  Then
we wouldn't need to have a setter.  If that's possible, I think it would
be slightly better, because it would show that the symbol is initialized
when the context is pushed and can't change.  Other callers of
push_context would pass a nullptr symbol.

Simon

Re: [PATCH 0/6] Some buildsym cleanups

[Simon Marchi]

On 4/17/26 2:24 PM, Tom Tromey wrote:
> This patch series cleans up buildsym a bit, in particular the context
> pushing and popping code.
> 
> Regression tested on x86-64 Fedora 43.
> 
> Signed-off-by: Tom Tromey <tromey@adacore.com>
> ---
> Tom Tromey (6):
>       Use scoped_restore for dwarf2_cu::list_in_scope
>       Remove some dead code from buildsym.c
>       Remove context_stack::depth
>       Change pop_context to return a block
>       Return void from buildsym_compunit::push_context
>       Rename context_stack and make it private
> 
>  gdb/buildsym.c    |  58 +++++++++++++------------------
>  gdb/buildsym.h    | 100 ++++++++++++++++++++++++++++++------------------------
>  gdb/dwarf2/cu.h   |   7 +---
>  gdb/dwarf2/read.c |  82 +++++++++++++++-----------------------------
>  4 files changed, 108 insertions(+), 139 deletions(-)
> ---
> base-commit: bfd118583ed69db365265915d1a48eb719e997f5
> change-id: 20260417-list-in-scope-4242b4cf355b
> 
> Best regards,
> -- 
> Tom Tromey <tromey@adacore.com>
> 

I provided some ideas for possible future improvements, but I think that
this series is a clear step forward, and I would hate to delay this, and
any improvement can be made in subsequent patches.  So:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon