[PATCH] Make blockvector a little more self-contained

[PATCH] Make blockvector a little more self-contained

[Tom Tromey]

This patch changes blockvector to be a little more self-contained.

The idea here is that code outside of blockvector shouldn't really
know how it operates.  After this patch, this still doesn't fully
happen -- a couple spots check the result of map() and make decisions
based on that -- but this is a step toward making that happen.  The
longer term idea here is that this is needed to enable lazier CU
expansion.

Meanwhile, this patch seems like a simple cleanup.  Relocation is now
handled by the blockvector itself and the non-const map() method can
be removed.

There wasn't a great spot to move the section_offsets typedef.  I
chose defs.h.  I've also updated the comment there as it has been out
of date for a long time.  I've also removed an obsolete comment from
the symbol-relocation code.

Regression tested on x86-64 Fedora 40.
---
 gdb/block.c    | 37 +++++++++++++++++++++++++++++++++++++
 gdb/block.h    | 14 ++++++++++----
 gdb/defs.h     |  4 ++++
 gdb/objfiles.c | 45 ++-------------------------------------------
 gdb/symtab.c   | 10 ++++++++++
 gdb/symtab.h   | 12 +++++-------
 6 files changed, 68 insertions(+), 54 deletions(-)

diff --git a/gdb/block.c b/gdb/block.c
index 9fb04635975..e959adc968c 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -366,6 +366,28 @@ block::static_link () const
   return (struct dynamic_prop *) objfile_lookup_static_link (objfile, this);
 }
 
+/* See block.h.  */
+
+void
+block::relocate (struct objfile *objfile, const section_offsets &offsets)
+{
+  int block_line_section = SECT_OFF_TEXT (objfile);
+
+  set_start (start () + offsets[block_line_section]);
+  set_end (end () + offsets[block_line_section]);
+
+  for (blockrange &r : ranges ())
+    {
+      r.set_start (r.start () + offsets[block_line_section]);
+      r.set_end (r.end () + offsets[block_line_section]);
+    }
+
+  /* We only want to iterate over the local symbols, not any
+     symbols in included symtabs.  */
+  for (struct symbol *sym : multidict_symbols ())
+    sym->relocate (objfile, offsets);
+}
+
 /* Initialize a block iterator, either to iterate over a single block,
    or, for static and global blocks, all the included symtabs as
    well.  */
@@ -865,6 +887,21 @@ blockvector::~blockvector ()
     mdict_free (bl->multidict ());
 }
 
+/* See block.h.  */
+
+void
+blockvector::relocate (struct objfile *objfile,
+		       const section_offsets &offsets)
+{
+  int block_line_section = SECT_OFF_TEXT (objfile);
+
+  if (m_map != nullptr)
+    m_map->relocate (offsets[block_line_section]);
+
+  for (struct block *b : m_blocks)
+    b->relocate (objfile, offsets);
+}
+
 /* Implement 'maint info blocks' command.  If passed an argument then
    print a list of all blocks at the given address.  With no arguments
    then list all blocks at the current address of the current inferior.  */
diff --git a/gdb/block.h b/gdb/block.h
index df03231ca4c..ceff97d79e3 100644
--- a/gdb/block.h
+++ b/gdb/block.h
@@ -315,6 +315,11 @@ struct block : public allocate_on_obstack<block>
 
   bool contains (const struct block *a, bool allow_nested = false) const;
 
+  /* Relocate this block and all contained blocks.  OBJFILE is the
+     objfile holding this block, and OFFSETS is the relocation offsets
+     to use.  */
+  void relocate (struct objfile *objfile, const section_offsets &offsets);
+
 private:
 
   /* Return the default entry-pc of this block.  The default is the address
@@ -482,10 +487,6 @@ struct blockvector
   const struct block *static_block () const
   { return this->block (STATIC_BLOCK); }
 
-  /* Return the address -> block map of this blockvector.  */
-  addrmap_fixed *map ()
-  { return m_map; }
-
   /* Const version of the above.  */
   const addrmap_fixed *map () const
   { return m_map; }
@@ -513,6 +514,11 @@ struct blockvector
      for ADDR are considered.  */
   struct symbol *symbol_at_address (CORE_ADDR addr) const;
 
+  /* Relocate this blockvector and all contained blocks.  OBJFILE is
+     the objfile holding this blockvector, and OFFSETS is the
+     relocation offsets to use.  */
+  void relocate (struct objfile *objfile, const section_offsets &offsets);
+
 private:
   /* An address map mapping addresses to blocks in this blockvector.
      This pointer is zero if the blocks' start and end addresses are
diff --git a/gdb/defs.h b/gdb/defs.h
index 8bf9d3dcbda..bb047d9cb4c 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -411,4 +411,8 @@ DEF_ENUM_FLAGS_TYPE (enum user_selected_what_flag, user_selected_what);
   extern void _initialize_ ## NAME (); \
   void _initialize_ ## NAME ()
 
+/* How to relocate the symbols from each section in a symbol file.
+   This is indexed by section numbers.  */
+typedef std::vector<CORE_ADDR> section_offsets;
+
 #endif /* GDB_DEFS_H */
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index f131bab1dd5..5a7bda7821a 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -508,24 +508,6 @@ objfile::~objfile ()
 }
 
 \f
-/* A helper function for objfile_relocate1 that relocates a single
-   symbol.  */
-
-static void
-relocate_one_symbol (struct symbol *sym, struct objfile *objfile,
-		     const section_offsets &delta)
-{
-  /* The RS6000 code from which this was taken skipped
-     any symbols in STRUCT_DOMAIN or UNDEF_DOMAIN.
-     But I'm leaving out that test, on the theory that
-     they can't possibly pass the tests below.  */
-  if ((sym->loc_class () == LOC_LABEL
-       || sym->loc_class () == LOC_STATIC)
-      && sym->section_index () >= 0)
-    sym->set_value_address (sym->value_address ()
-			    + delta[sym->section_index ()]);
-}
-
 /* Relocate OBJFILE to NEW_OFFSETS.  There should be OBJFILE->NUM_SECTIONS
    entries in new_offsets.  SEPARATE_DEBUG_OBJFILE is not touched here.
    Return non-zero iff any change happened.  */
@@ -549,34 +531,11 @@ objfile_relocate1 (struct objfile *objfile,
 
   /* OK, get all the symtabs.  */
   for (compunit_symtab &cust : objfile->compunits ())
-    {
-      struct blockvector *bv = cust.blockvector ();
-      int block_line_section = SECT_OFF_TEXT (objfile);
-
-      if (bv->map () != nullptr)
-	bv->map ()->relocate (delta[block_line_section]);
-
-      for (block *b : bv->blocks ())
-	{
-	  b->set_start (b->start () + delta[block_line_section]);
-	  b->set_end (b->end () + delta[block_line_section]);
-
-	  for (blockrange &r : b->ranges ())
-	    {
-	      r.set_start (r.start () + delta[block_line_section]);
-	      r.set_end (r.end () + delta[block_line_section]);
-	    }
-
-	  /* We only want to iterate over the local symbols, not any
-	     symbols in included symtabs.  */
-	  for (struct symbol *sym : b->multidict_symbols ())
-	    relocate_one_symbol (sym, objfile, delta);
-	}
-    }
+    cust.blockvector ()->relocate (objfile, delta);
 
   /* Relocate isolated symbols.  */
   for (symbol *iter = objfile->template_symbols; iter; iter = iter->hash_next)
-    relocate_one_symbol (iter, objfile, delta);
+    iter->relocate (objfile, delta);
 
   for (int i = 0; i < objfile->section_offsets.size (); ++i)
     objfile->section_offsets[i] = new_offsets[i];
diff --git a/gdb/symtab.c b/gdb/symtab.c
index cc6dbcf7aab..93c91c43bf3 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -6610,6 +6610,16 @@ symbol::value_block () const
 
 /* See symtab.h.  */
 
+void
+symbol::relocate (struct objfile *objfile, const section_offsets &delta)
+{
+  if ((loc_class () == LOC_LABEL || loc_class () == LOC_STATIC)
+      && section_index () >= 0)
+    set_value_address (value_address () + delta[section_index ()]);
+}
+
+/* See symtab.h.  */
+
 CORE_ADDR
 minimal_symbol::get_maybe_copied_address (objfile *objf) const
 {
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 845c49e057d..4f2b88859a5 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1458,6 +1458,11 @@ struct symbol : public general_symbol_info, public allocate_on_obstack<symbol>
 
   void set_symtab (struct symtab *symtab);
 
+  /* Relocate this symbol.  OBJFILE is the objfile holding this
+     blockvector, and OFFSETS is the relocation offsets to use.  */
+
+  void relocate (struct objfile *objfile, const section_offsets &offsets);
+
   /* Data type of value */
 
   struct type *m_type = nullptr;
@@ -1675,13 +1680,6 @@ struct linetable
   struct linetable_entry item[1];
 };
 
-/* How to relocate the symbols from each section in a symbol file.
-   The ordering and meaning of the offsets is file-type-dependent;
-   typically it is indexed by section numbers or symbol types or
-   something like that.  */
-
-typedef std::vector<CORE_ADDR> section_offsets;
-
 /* Each source file or header is represented by a struct symtab.
    The name "symtab" is historical, another name for it is "filetab".
    These objects are chained through the `next' field.  */
-- 
2.49.0

Re: [PATCH] Make blockvector a little more self-contained

[Simon Marchi]

On 2025-11-18 22:30, Tom Tromey wrote:
> This patch changes blockvector to be a little more self-contained.
> 
> The idea here is that code outside of blockvector shouldn't really
> know how it operates.  After this patch, this still doesn't fully
> happen -- a couple spots check the result of map() and make decisions
> based on that -- but this is a step toward making that happen.  The
> longer term idea here is that this is needed to enable lazier CU
> expansion.
> 
> Meanwhile, this patch seems like a simple cleanup.  Relocation is now
> handled by the blockvector itself and the non-const map() method can
> be removed.

Overall, LGTM.

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

See minor comments below.

> 
> There wasn't a great spot to move the section_offsets typedef.  I
> chose defs.h.  I've also updated the comment there as it has been out
> of date for a long time.  I've also removed an obsolete comment from
> the symbol-relocation code.

I honestly think that this typedef could be removed, that it would be
clearer to see "std::vector<CORE_ADDR>" in the code.

> diff --git a/gdb/block.c b/gdb/block.c
> index 9fb04635975..e959adc968c 100644
> --- a/gdb/block.c
> +++ b/gdb/block.c
> @@ -366,6 +366,28 @@ block::static_link () const
>    return (struct dynamic_prop *) objfile_lookup_static_link (objfile, this);
>  }
>  
> +/* See block.h.  */
> +
> +void
> +block::relocate (struct objfile *objfile, const section_offsets &offsets)
> +{
> +  int block_line_section = SECT_OFF_TEXT (objfile);
> +
> +  set_start (start () + offsets[block_line_section]);
> +  set_end (end () + offsets[block_line_section]);
> +
> +  for (blockrange &r : ranges ())
> +    {
> +      r.set_start (r.start () + offsets[block_line_section]);
> +      r.set_end (r.end () + offsets[block_line_section]);
> +    }
> +
> +  /* We only want to iterate over the local symbols, not any
> +     symbols in included symtabs.  */
> +  for (struct symbol *sym : multidict_symbols ())
> +    sym->relocate (objfile, offsets);

Do you know what the comment above refers to?  What are we doing special
to avoid iterating on the symbols in included symtabs?

> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index 845c49e057d..4f2b88859a5 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -1458,6 +1458,11 @@ struct symbol : public general_symbol_info, public allocate_on_obstack<symbol>
>  
>    void set_symtab (struct symtab *symtab);
>  
> +  /* Relocate this symbol.  OBJFILE is the objfile holding this
> +     blockvector, and OFFSETS is the relocation offsets to use.  */
> +
> +  void relocate (struct objfile *objfile, const section_offsets &offsets);

The comment says "this blockvector", looks like a copy-pasto.

Also, a struct symbol already knows its objfile (it has an objfile()
method) if it is objfile-owned, which should be the case here.  So it
seems superfluous to pass the objfile here.  Actually, the objfile
parameter is not even used in symbol::relocate.

Simon

Re: [PATCH] Make blockvector a little more self-contained

[Tom Tromey]

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

Simon> I honestly think that this typedef could be removed, that it would be
Simon> clearer to see "std::vector<CORE_ADDR>" in the code.

I'll send a separate patch to remove it.
Some spots should be using an array view anyway IMO.

>> +  /* We only want to iterate over the local symbols, not any
>> +     symbols in included symtabs.  */
>> +  for (struct symbol *sym : multidict_symbols ())
>> +    sym->relocate (objfile, offsets);

Simon> Do you know what the comment above refers to?  What are we doing special
Simon> to avoid iterating on the symbols in included symtabs?

I do not know.  I've removed this comment.

>> +  void relocate (struct objfile *objfile, const section_offsets &offsets);

Simon> The comment says "this blockvector", looks like a copy-pasto.

Yep.

Simon> Also, a struct symbol already knows its objfile (it has an objfile()
Simon> method) if it is objfile-owned, which should be the case here.  So it
Simon> seems superfluous to pass the objfile here.  Actually, the objfile
Simon> parameter is not even used in symbol::relocate.

I removed the parameter.

I'll check this in soon.

Tom