[PATCH 0/6] Change how symbol section indices are set

[PATCH 0/6] Change how symbol section indices are set

[Tom Tromey]

An otherwise innocuous internal AdaCore test case started failing when
linked with 'mold'.  Investigation revealed that the problem was
actually in gdb -- gdb decided that a certain symbol was in the
.interp section.

Digging deeper, I found that most calls to fixup_symbol_section are
incorrect, and that if a section is chosen, it's frequently the wrong
one.

This series attempts to clean up this area.  Regression tested on
x86-64 Fedora 36.

Tom

[PATCH 1/6] Use default section indexes in fixup_symbol_section

[Tom Tromey]

If fixup_section does not find a matching section, it arbitrarily
chooses the first one.  However, it seems better to make this default
depend on the type of the symbol -- i.e., default data symbols to
.data and text symbols to .text.

I've also made fixup_section static, as it only has one caller.
---
 gdb/symtab.c | 16 ++++++++++++----
 gdb/symtab.h |  3 ---
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index b3445133c8c..fe247ab70eb 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -1704,9 +1704,10 @@ symtab_free_objfile_observer (struct objfile *objfile)
 /* Debug symbols usually don't have section information.  We need to dig that
    out of the minimal symbols and stash that in the debug symbol.  */
 
-void
+static void
 fixup_section (struct general_symbol_info *ginfo,
-	       CORE_ADDR addr, struct objfile *objfile)
+	       CORE_ADDR addr, struct objfile *objfile,
+	       int default_section)
 {
   struct minimal_symbol *msym;
 
@@ -1757,7 +1758,7 @@ fixup_section (struct general_symbol_info *ginfo,
 	 a search of the section table.  */
 
       struct obj_section *s;
-      int fallback = -1;
+      int fallback = default_section;
 
       ALL_OBJFILE_OSECTIONS (objfile, s)
 	{
@@ -1808,9 +1809,16 @@ fixup_symbol_section (struct symbol *sym, struct objfile *objfile)
   /* We should have an objfile by now.  */
   gdb_assert (objfile);
 
+  /* Note that if this ends up as -1, fixup_section will handle that
+     reasonably well.  So, it's fine to use the objfile's section
+     index without doing the check that is done by the wrapper macros
+     like SECT_OFF_TEXT.  */
+  int default_section = objfile->sect_index_text;
   switch (sym->aclass ())
     {
     case LOC_STATIC:
+      default_section = objfile->sect_index_data;
+      /* FALLTHROUGH.  */
     case LOC_LABEL:
       addr = sym->value_address ();
       break;
@@ -1824,7 +1832,7 @@ fixup_symbol_section (struct symbol *sym, struct objfile *objfile)
       return sym;
     }
 
-  fixup_section (sym, addr, objfile);
+  fixup_section (sym, addr, objfile, default_section);
 
   return sym;
 }
diff --git a/gdb/symtab.h b/gdb/symtab.h
index ae3a81991df..ac3f6391dc3 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2607,9 +2607,6 @@ extern struct block_symbol
    compiler (armcc).  */
 bool producer_is_realview (const char *producer);
 
-void fixup_section (struct general_symbol_info *ginfo,
-		    CORE_ADDR addr, struct objfile *objfile);
-
 extern unsigned int symtab_create_debug;
 
 /* Print a "symtab-create" debug statement.  */
-- 
2.38.1

Re: [PATCH 1/6] Use default section indexes in fixup_symbol_section

[Simon Marchi]

On 1/18/23 10:30, Tom Tromey via Gdb-patches wrote:
> If fixup_section does not find a matching section, it arbitrarily
> chooses the first one.  However, it seems better to make this default
> depend on the type of the symbol -- i.e., default data symbols to
> .data and text symbols to .text.
> 
> I've also made fixup_section static, as it only has one caller.

I don't really know the big picture to tell whether this is right, but
one small comment:

> ---
>  gdb/symtab.c | 16 ++++++++++++----
>  gdb/symtab.h |  3 ---
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index b3445133c8c..fe247ab70eb 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -1704,9 +1704,10 @@ symtab_free_objfile_observer (struct objfile *objfile)
>  /* Debug symbols usually don't have section information.  We need to dig that
>     out of the minimal symbols and stash that in the debug symbol.  */
>  
> -void
> +static void
>  fixup_section (struct general_symbol_info *ginfo,
> -	       CORE_ADDR addr, struct objfile *objfile)
> +	       CORE_ADDR addr, struct objfile *objfile,
> +	       int default_section)

Could you expand the comment above to describe default_section?

Simon

Re: [PATCH 1/6] Use default section indexes in fixup_symbol_section

[Tom Tromey]

>> -void
>> +static void
>> fixup_section (struct general_symbol_info *ginfo,
>> -	       CORE_ADDR addr, struct objfile *objfile)
>> +	       CORE_ADDR addr, struct objfile *objfile,
>> +	       int default_section)

Simon> Could you expand the comment above to describe default_section?

I did this, but note the comment is deleted again in patch #6 when the
two functions are merged.

Tom

[PATCH 2/6] Set section indices when symbols are made

[Tom Tromey]

Most places in gdb that create a new symbol will apply a section
offset to the address.  It seems to me that the choice of offset here
is also an implicit choice of the section.  This is particularly true
if you examine fixup_section, which notes that it must be called
before such offsets are applied -- meaning that if any such call has
an effect, it's purely by accident.

This patch cleans up this area by tracking the section index and
applying it to a symbol when the address is set.  This is done for
nearly every case -- the remaining cases will be handled in later
patches.
---
 gdb/dbxread.c     | 21 ++++++++++++++++++++-
 gdb/dwarf2/read.c |  1 +
 gdb/mdebugread.c  | 18 ++++++++++++++----
 gdb/stabsread.c   | 14 ++++++++------
 4 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/gdb/dbxread.c b/gdb/dbxread.c
index ab0734f0218..dc11f0bb6a9 100644
--- a/gdb/dbxread.c
+++ b/gdb/dbxread.c
@@ -2414,6 +2414,9 @@ process_one_symbol (int type, int desc, CORE_ADDR valu, const char *name,
      source file.  Used to detect the SunPRO solaris compiler.  */
   static int n_opt_found;
 
+  /* The section index for this symbol.  */
+  int section_index = -1;
+
   /* Something is wrong if we see real data before seeing a source
      file name.  */
 
@@ -2477,6 +2480,7 @@ process_one_symbol (int type, int desc, CORE_ADDR valu, const char *name,
       sline_found_in_function = 0;
 
       /* Relocate for dynamic loading.  */
+      section_index = SECT_OFF_TEXT (objfile);
       valu += section_offsets[SECT_OFF_TEXT (objfile)];
       valu = gdbarch_addr_bits_remove (gdbarch, valu);
       last_function_start = valu;
@@ -2565,6 +2569,7 @@ process_one_symbol (int type, int desc, CORE_ADDR valu, const char *name,
     case N_FN_SEQ:
       /* This kind of symbol indicates the start of an object file.
 	 Relocate for dynamic loading.  */
+      section_index = SECT_OFF_TEXT (objfile);
       valu += section_offsets[SECT_OFF_TEXT (objfile)];
       break;
 
@@ -2573,6 +2578,7 @@ process_one_symbol (int type, int desc, CORE_ADDR valu, const char *name,
 	 source file.  Finish the symbol table of the previous source
 	 file (if any) and start accumulating a new symbol table.
 	 Relocate for dynamic loading.  */
+      section_index = SECT_OFF_TEXT (objfile);
       valu += section_offsets[SECT_OFF_TEXT (objfile)];
 
       n_opt_found = 0;
@@ -2609,6 +2615,7 @@ process_one_symbol (int type, int desc, CORE_ADDR valu, const char *name,
 	 sub-source-file, one whose contents were copied or included
 	 in the compilation of the main source file (whose name was
 	 given in the N_SO symbol).  Relocate for dynamic loading.  */
+      section_index = SECT_OFF_TEXT (objfile);
       valu += section_offsets[SECT_OFF_TEXT (objfile)];
       start_subfile (name);
       break;
@@ -2709,6 +2716,7 @@ process_one_symbol (int type, int desc, CORE_ADDR valu, const char *name,
 		   symbol_file_add as addr (this is known to affect
 		   SunOS 4, and I suspect ELF too).  Since there is no
 		   Ttext.text symbol, we can get addr from the text offset.  */
+		section_index = SECT_OFF_TEXT (objfile);
 		valu += section_offsets[SECT_OFF_TEXT (objfile)];
 		goto define_a_symbol;
 	      }
@@ -2730,21 +2738,25 @@ process_one_symbol (int type, int desc, CORE_ADDR valu, const char *name,
 
     case_N_STSYM:		/* Static symbol in data segment.  */
     case N_DSLINE:		/* Source line number, data segment.  */
+      section_index = SECT_OFF_DATA (objfile);
       valu += section_offsets[SECT_OFF_DATA (objfile)];
       goto define_a_symbol;
 
     case_N_LCSYM:		/* Static symbol in BSS segment.  */
     case N_BSLINE:		/* Source line number, BSS segment.  */
       /* N_BROWS: overlaps with N_BSLINE.  */
+      section_index = SECT_OFF_BSS (objfile);
       valu += section_offsets[SECT_OFF_BSS (objfile)];
       goto define_a_symbol;
 
     case_N_ROSYM:		/* Static symbol in read-only data segment.  */
+      section_index = SECT_OFF_RODATA (objfile);
       valu += section_offsets[SECT_OFF_RODATA (objfile)];
       goto define_a_symbol;
 
     case N_ENTRY:		/* Alternate entry point.  */
       /* Relocate for dynamic loading.  */
+      section_index = SECT_OFF_TEXT (objfile);
       valu += section_offsets[SECT_OFF_TEXT (objfile)];
       goto define_a_symbol;
 
@@ -2836,10 +2848,17 @@ process_one_symbol (int type, int desc, CORE_ADDR valu, const char *name,
 
 	      newobj = push_context (0, valu);
 	      newobj->name = define_symbol (valu, name, desc, type, objfile);
+	      if (newobj->name != nullptr)
+		newobj->name->set_section_index (section_index);
 	      break;
 
 	    default:
-	      define_symbol (valu, name, desc, type, objfile);
+	      {
+		struct symbol *sym = define_symbol (valu, name, desc, type,
+						    objfile);
+		if (sym != nullptr)
+		  sym->set_section_index (section_index);
+	      }
 	      break;
 	    }
 	}
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 44b54f77de9..6a7412ce834 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -20871,6 +20871,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 
 	      addr = attr->as_address ();
 	      addr = gdbarch_adjust_dwarf2_addr (gdbarch, addr + baseaddr);
+	      sym->set_section_index (SECT_OFF_TEXT (objfile));
 	      sym->set_value_address (addr);
 	      sym->set_aclass_index (LOC_LABEL);
 	    }
diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
index 8dc836a0f6e..4feee39035e 100644
--- a/gdb/mdebugread.c
+++ b/gdb/mdebugread.c
@@ -597,6 +597,7 @@ parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
   else
     name = debug_info->ss + cur_fdr->issBase + sh->iss;
 
+  int section_index = -1;
   switch (sh->sc)
     {
     case scText:
@@ -607,21 +608,24 @@ parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
 	 The value of a stBlock symbol is the displacement from the
 	 procedure address.  */
       if (sh->st != stEnd && sh->st != stBlock)
-	sh->value += section_offsets[SECT_OFF_TEXT (objfile)];
+	section_index = SECT_OFF_TEXT (objfile);
       break;
     case scData:
     case scSData:
     case scRData:
     case scPData:
     case scXData:
-      sh->value += section_offsets[SECT_OFF_DATA (objfile)];
+      section_index = SECT_OFF_DATA (objfile);
       break;
     case scBss:
     case scSBss:
-      sh->value += section_offsets[SECT_OFF_BSS (objfile)];
+      section_index = SECT_OFF_BSS (objfile);
       break;
     }
 
+  if (section_index != -1)
+    sh->value += section_offsets[section_index];
+
   switch (sh->st)
     {
     case stNil:
@@ -630,6 +634,7 @@ parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
     case stGlobal:		/* External symbol, goes into global block.  */
       b = top_stack->cur_st->compunit ()->blockvector ()->global_block ();
       s = new_symbol (name);
+      s->set_section_index (section_index);
       s->set_value_address (sh->value);
       add_data_symbol (sh, ax, bigend, s, LOC_STATIC, b, objfile, name);
       break;
@@ -647,7 +652,10 @@ parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
 	  global_sym_chain[bucket] = s;
 	}
       else
-	s->set_value_address (sh->value);
+	{
+	  s->set_section_index (section_index);
+	  s->set_value_address (sh->value);
+	}
       add_data_symbol (sh, ax, bigend, s, LOC_STATIC, b, objfile, name);
       break;
 
@@ -704,6 +712,7 @@ parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
       s = new_symbol (name);
       s->set_domain (VAR_DOMAIN);	/* So that it can be used */
       s->set_aclass_index (LOC_LABEL);	/* but not misused.  */
+      s->set_section_index (section_index);
       s->set_value_address (sh->value);
       s->set_type (objfile_type (objfile)->builtin_int);
       add_symbol (s, top_stack->cur_st, top_stack->cur_block);
@@ -745,6 +754,7 @@ parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
       s = new_symbol (name);
       s->set_domain (VAR_DOMAIN);
       s->set_aclass_index (LOC_BLOCK);
+      s->set_section_index (section_index);
       /* Type of the return value.  */
       if (SC_IS_UNDEF (sh->sc) || sh->sc == scNil)
 	t = objfile_type (objfile)->builtin_int;
diff --git a/gdb/stabsread.c b/gdb/stabsread.c
index 8d1b998354f..ca9132b37d0 100644
--- a/gdb/stabsread.c
+++ b/gdb/stabsread.c
@@ -107,8 +107,6 @@ static void
 patch_block_stabs (struct pending *, struct pending_stabs *,
 		   struct objfile *);
 
-static void fix_common_block (struct symbol *, CORE_ADDR);
-
 static int read_type_number (const char **, int *);
 
 static struct type *read_type (const char **, struct objfile *);
@@ -4305,7 +4303,7 @@ common_block_end (struct objfile *objfile)
    the common block name).  */
 
 static void
-fix_common_block (struct symbol *sym, CORE_ADDR valu)
+fix_common_block (struct symbol *sym, CORE_ADDR valu, int section_index)
 {
   struct pending *next = (struct pending *) sym->type ();
 
@@ -4314,8 +4312,11 @@ fix_common_block (struct symbol *sym, CORE_ADDR valu)
       int j;
 
       for (j = next->nsyms - 1; j >= 0; j--)
-	next->symbol[j]->set_value_address
-	  (next->symbol[j]->value_address () + valu);
+	{
+	  next->symbol[j]->set_value_address
+	    (next->symbol[j]->value_address () + valu);
+	  next->symbol[j]->set_section_index (section_index);
+	}
     }
 }
 \f
@@ -4585,7 +4586,8 @@ scan_file_globals (struct objfile *objfile)
 		    {
 		      if (sym->aclass () == LOC_BLOCK)
 			fix_common_block
-			  (sym, msymbol->value_address (resolve_objfile));
+			  (sym, msymbol->value_address (resolve_objfile),
+			   msymbol->section_index ());
 		      else
 			sym->set_value_address
 			  (msymbol->value_address (resolve_objfile));
-- 
2.38.1

Re: [PATCH 2/6] Set section indices when symbols are made

[Simon Marchi]

On 1/18/23 10:30, Tom Tromey via Gdb-patches wrote:
> Most places in gdb that create a new symbol will apply a section
> offset to the address.  It seems to me that the choice of offset here
> is also an implicit choice of the section.  This is particularly true
> if you examine fixup_section, which notes that it must be called
> before such offsets are applied -- meaning that if any such call has
> an effect, it's purely by accident.
> 
> This patch cleans up this area by tracking the section index and
> applying it to a symbol when the address is set.  This is done for
> nearly every case -- the remaining cases will be handled in later
> patches.

So, if I rephrase it to make sure I understand: it's not logical to
apply the relocation to the symbol's address, without also setting the
symbol's section index, because the symbol's relocation value comes from
the section.  Does that sound right?

Simon

Re: [PATCH 2/6] Set section indices when symbols are made

[Tom Tromey]

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

Simon> On 1/18/23 10:30, Tom Tromey via Gdb-patches wrote:
>> Most places in gdb that create a new symbol will apply a section
>> offset to the address.  It seems to me that the choice of offset here
>> is also an implicit choice of the section.  This is particularly true
>> if you examine fixup_section, which notes that it must be called
>> before such offsets are applied -- meaning that if any such call has
>> an effect, it's purely by accident.
>> 
>> This patch cleans up this area by tracking the section index and
>> applying it to a symbol when the address is set.  This is done for
>> nearly every case -- the remaining cases will be handled in later
>> patches.

Simon> So, if I rephrase it to make sure I understand: it's not logical to
Simon> apply the relocation to the symbol's address, without also setting the
Simon> symbol's section index, because the symbol's relocation value comes from
Simon> the section.  Does that sound right?

Yes, that's right.

Hypothetically, if you had a runtime linker that applied different
offsets to different sections (like, text gets +0x1000, data gets
+0x2000, bss gets +0x3000), then the code that applies these offsets
could sometimes be wrong.  But, since it's been this way a long time
and, AFAIK, there aren't problems with it, then IMO it follows that
setting the section from these choices is fine.

One other oddity I found here is that I don't think any code uses
anything other than SECT_OFF_TEXT for functions.  So maybe the code to
deal with the section index in a compunit symtab is not actually needed.

Tom

[PATCH 3/6] Pass section index to start_compunit_symtab

[Tom Tromey]

Right now, the appropriate section index (apparently always
SECT_OFF_TEXT) is passed to end_compunit_symtab.  However, it's more
convenient to have this section index available when setting the
symbol's block.  So, this patch rearranges the code to pass the
section index to start_compunit_symtab, and then to store the index in
the buildsym_compunit object.
---
 gdb/buildsym-legacy.c |  9 +++++----
 gdb/buildsym-legacy.h |  6 +++---
 gdb/buildsym.c        | 29 +++++++++++++----------------
 gdb/buildsym.h        | 28 ++++++++++++++++++----------
 gdb/coffread.c        |  5 +++--
 gdb/ctfread.c         | 18 +++++++++---------
 gdb/dbxread.c         |  8 ++++----
 gdb/dwarf2/cu.c       |  3 ++-
 gdb/dwarf2/read.c     |  5 ++---
 gdb/mdebugread.c      |  5 ++---
 gdb/xcoffread.c       | 20 ++++++++++----------
 11 files changed, 71 insertions(+), 65 deletions(-)

diff --git a/gdb/buildsym-legacy.c b/gdb/buildsym-legacy.c
index e4194b69403..c4a767951f8 100644
--- a/gdb/buildsym-legacy.c
+++ b/gdb/buildsym-legacy.c
@@ -171,11 +171,11 @@ free_buildsym_compunit (void)
 }
 
 struct compunit_symtab *
-end_compunit_symtab (CORE_ADDR end_addr, int section)
+end_compunit_symtab (CORE_ADDR end_addr)
 {
   gdb_assert (buildsym_compunit != nullptr);
   struct compunit_symtab *result
-    = buildsym_compunit->end_compunit_symtab (end_addr, section);
+    = buildsym_compunit->end_compunit_symtab (end_addr);
   free_buildsym_compunit ();
   return result;
 }
@@ -228,14 +228,15 @@ record_line (struct subfile *subfile, int line, CORE_ADDR pc)
 struct compunit_symtab *
 start_compunit_symtab (struct objfile *objfile, const char *name,
 		       const char *comp_dir, CORE_ADDR start_addr,
-		       enum language language)
+		       enum language language, int section_index)
 {
   /* These should have been reset either by successful completion of building
      a symtab, or by the scoped_free_pendings destructor.  */
   gdb_assert (buildsym_compunit == nullptr);
 
   buildsym_compunit = new struct buildsym_compunit (objfile, name, comp_dir,
-						    language, start_addr);
+						    language, start_addr,
+						    section_index);
 
   return buildsym_compunit->get_compunit_symtab ();
 }
diff --git a/gdb/buildsym-legacy.h b/gdb/buildsym-legacy.h
index b553eb7e054..438d9ccb381 100644
--- a/gdb/buildsym-legacy.h
+++ b/gdb/buildsym-legacy.h
@@ -70,8 +70,7 @@ extern void push_subfile ();
 
 extern const char *pop_subfile ();
 
-extern struct compunit_symtab *end_compunit_symtab (CORE_ADDR end_addr,
-						    int section);
+extern struct compunit_symtab *end_compunit_symtab (CORE_ADDR end_addr);
 
 extern struct context_stack *push_context (int desc, CORE_ADDR valu);
 
@@ -83,7 +82,8 @@ extern struct compunit_symtab *start_compunit_symtab (struct objfile *objfile,
 						      const char *name,
 						      const char *comp_dir,
 						      CORE_ADDR start_addr,
-						      enum language language);
+						      enum language language,
+						      int section_index);
 
 /* Record the name of the debug format in the current pending symbol
    table.  FORMAT must be a string with a lifetime at least as long as
diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index adab927235c..8c61223b1a0 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -54,12 +54,14 @@ buildsym_compunit::buildsym_compunit (struct objfile *objfile_,
 				      const char *comp_dir_,
 				      const char *name_for_id,
 				      enum language language_,
-				      CORE_ADDR last_addr)
+				      CORE_ADDR last_addr,
+				      int section_index)
   : m_objfile (objfile_),
     m_last_source_file (name == nullptr ? nullptr : xstrdup (name)),
     m_comp_dir (comp_dir_ == nullptr ? "" : comp_dir_),
     m_language (language_),
-    m_last_source_start_addr (last_addr)
+    m_last_source_start_addr (last_addr),
+    m_section_index (section_index)
 {
   /* Allocate the compunit symtab now.  The caller needs it to allocate
      non-primary symtabs.  It is also needed by get_macro_table.  */
@@ -854,7 +856,7 @@ buildsym_compunit::end_compunit_symtab_get_static_block (CORE_ADDR end_addr,
 
 struct compunit_symtab *
 buildsym_compunit::end_compunit_symtab_with_blockvector
-  (struct block *static_block, int section, int expandable)
+  (struct block *static_block, int expandable)
 {
   struct compunit_symtab *cu = m_compunit_symtab;
   struct blockvector *blockvector;
@@ -974,7 +976,7 @@ buildsym_compunit::end_compunit_symtab_with_blockvector
     set_block_compunit_symtab (b, cu);
   }
 
-  cu->set_block_line_section (section);
+  cu->set_block_line_section (m_section_index);
 
   cu->set_macro_table (release_macros ());
 
@@ -1014,15 +1016,12 @@ buildsym_compunit::end_compunit_symtab_with_blockvector
 /* Implementation of the second part of end_compunit_symtab.  Pass STATIC_BLOCK
    as value returned by end_compunit_symtab_get_static_block.
 
-   SECTION is the same as for end_compunit_symtab: the section number
-   (in objfile->section_offsets) of the blockvector and linetable.
-
    If EXPANDABLE is non-zero the GLOBAL_BLOCK dictionary is made
    expandable.  */
 
 struct compunit_symtab *
 buildsym_compunit::end_compunit_symtab_from_static_block
-  (struct block *static_block, int section, int expandable)
+  (struct block *static_block, int expandable)
 {
   struct compunit_symtab *cu;
 
@@ -1040,7 +1039,7 @@ buildsym_compunit::end_compunit_symtab_from_static_block
       cu = NULL;
     }
   else
-    cu = end_compunit_symtab_with_blockvector (static_block, section, expandable);
+    cu = end_compunit_symtab_with_blockvector (static_block, expandable);
 
   return cu;
 }
@@ -1050,9 +1049,7 @@ buildsym_compunit::end_compunit_symtab_from_static_block
    them), then make the struct symtab for that file and put it in the
    list of all such.
 
-   END_ADDR is the address of the end of the file's text.  SECTION is
-   the section number (in objfile->section_offsets) of the blockvector
-   and linetable.
+   END_ADDR is the address of the end of the file's text.
 
    Note that it is possible for end_compunit_symtab() to return NULL.  In
    particular, for the DWARF case at least, it will return NULL when
@@ -1067,24 +1064,24 @@ buildsym_compunit::end_compunit_symtab_from_static_block
    end_compunit_symtab_from_static_block yourself.  */
 
 struct compunit_symtab *
-buildsym_compunit::end_compunit_symtab (CORE_ADDR end_addr, int section)
+buildsym_compunit::end_compunit_symtab (CORE_ADDR end_addr)
 {
   struct block *static_block;
 
   static_block = end_compunit_symtab_get_static_block (end_addr, 0, 0);
-  return end_compunit_symtab_from_static_block (static_block, section, 0);
+  return end_compunit_symtab_from_static_block (static_block, 0);
 }
 
 /* Same as end_compunit_symtab except create a symtab that can be later added
    to.  */
 
 struct compunit_symtab *
-buildsym_compunit::end_expandable_symtab (CORE_ADDR end_addr, int section)
+buildsym_compunit::end_expandable_symtab (CORE_ADDR end_addr)
 {
   struct block *static_block;
 
   static_block = end_compunit_symtab_get_static_block (end_addr, 1, 0);
-  return end_compunit_symtab_from_static_block (static_block, section, 1);
+  return end_compunit_symtab_from_static_block (static_block, 1);
 }
 
 /* Subroutine of augment_type_symtab to simplify it.
diff --git a/gdb/buildsym.h b/gdb/buildsym.h
index 9724607f3d9..0464c738114 100644
--- a/gdb/buildsym.h
+++ b/gdb/buildsym.h
@@ -146,18 +146,23 @@ struct buildsym_compunit
      (or NULL if not known).
 
      NAME and NAME_FOR_ID have the same purpose as for the start_subfile
-     method.  */
+     method.
+
+     SECTION_INDEX is the index of the section for the compunit and
+     for block symbols in this compunit.  Normally SECT_OFF_TEXT.  */
 
   buildsym_compunit (struct objfile *objfile_, const char *name,
 		     const char *comp_dir_, const char *name_for_id,
-		     enum language language_, CORE_ADDR last_addr);
+		     enum language language_, CORE_ADDR last_addr,
+		     int section_index);
 
   /* Same as above, but passes NAME for NAME_FOR_ID.  */
 
   buildsym_compunit (struct objfile *objfile_, const char *name,
 		     const char *comp_dir_, enum language language_,
-		     CORE_ADDR last_addr)
-    : buildsym_compunit (objfile_, name, comp_dir_, name, language_, last_addr)
+		     CORE_ADDR last_addr, int section_index)
+    : buildsym_compunit (objfile_, name, comp_dir_, name, language_, last_addr,
+			 section_index)
   {}
 
   /* Reopen an existing compunit_symtab so that additional symbols can
@@ -172,7 +177,8 @@ struct buildsym_compunit
       m_comp_dir (comp_dir_ == nullptr ? "" : comp_dir_),
       m_compunit_symtab (cust),
       m_language (language_),
-      m_last_source_start_addr (last_addr)
+      m_last_source_start_addr (last_addr),
+      m_section_index (cust->block_line_section ())
   {
   }
 
@@ -327,12 +333,11 @@ struct buildsym_compunit
     (CORE_ADDR end_addr, int expandable, int required);
 
   struct compunit_symtab *end_compunit_symtab_from_static_block
-    (struct block *static_block, int section, int expandable);
+    (struct block *static_block, int expandable);
 
-  struct compunit_symtab *end_compunit_symtab (CORE_ADDR end_addr, int section);
+  struct compunit_symtab *end_compunit_symtab (CORE_ADDR end_addr);
 
-  struct compunit_symtab *end_expandable_symtab (CORE_ADDR end_addr,
-						 int section);
+  struct compunit_symtab *end_expandable_symtab (CORE_ADDR end_addr);
 
   void augment_type_symtab ();
 
@@ -352,7 +357,7 @@ struct buildsym_compunit
   void watch_main_source_file_lossage ();
 
   struct compunit_symtab *end_compunit_symtab_with_blockvector
-    (struct block *static_block, int section, int expandable);
+    (struct block *static_block, int expandable);
 
   /* The objfile we're reading debug info from.  */
   struct objfile *m_objfile;
@@ -401,6 +406,9 @@ struct buildsym_compunit
      DW_AT_low_pc attribute of a DW_TAG_compile_unit DIE.  */
   CORE_ADDR m_last_source_start_addr;
 
+  /* The section index.  */
+  int m_section_index;
+
   /* Stack of subfile names.  */
   std::vector<const char *> m_subfile_stack;
 
diff --git a/gdb/coffread.c b/gdb/coffread.c
index 4950a7324c8..041960d0939 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -376,7 +376,8 @@ coff_start_compunit_symtab (struct objfile *objfile, const char *name)
      set_last_source_start_addr in coff_end_compunit_symtab.  */
 			 0,
   /* Let buildsym.c deduce the language for this symtab.  */
-			 language_unknown);
+			 language_unknown,
+			 SECT_OFF_TEXT (objfile));
   record_debugformat ("COFF");
 }
 
@@ -404,7 +405,7 @@ coff_end_compunit_symtab (struct objfile *objfile)
 {
   set_last_source_start_addr (current_source_start_addr);
 
-  end_compunit_symtab (current_source_end_addr, SECT_OFF_TEXT (objfile));
+  end_compunit_symtab (current_source_end_addr);
 
   /* Reinitialize for beginning of new file.  */
   set_last_source_file (NULL);
diff --git a/gdb/ctfread.c b/gdb/ctfread.c
index 97a0df91a53..eca1bef69b3 100644
--- a/gdb/ctfread.c
+++ b/gdb/ctfread.c
@@ -1246,30 +1246,30 @@ get_objfile_text_range (struct objfile *of, int *tsize)
 
 static void
 ctf_start_compunit_symtab (ctf_psymtab *pst,
-			   struct objfile *of, CORE_ADDR text_offset)
+			   struct objfile *of, CORE_ADDR text_offset,
+			   int section_offset)
 {
   struct ctf_context *ccp;
 
   ccp = &pst->context;
   ccp->builder = new buildsym_compunit
 		       (of, pst->filename, nullptr,
-		       language_c, text_offset);
+			language_c, text_offset, section_offset);
   ccp->builder->record_debugformat ("ctf");
 }
 
 /* Finish reading symbol/type definitions in CTF format.
-   END_ADDR is the end address of the file's text.  SECTION is
-   the .text section number.  */
+   END_ADDR is the end address of the file's text.  */
 
 static struct compunit_symtab *
 ctf_end_compunit_symtab (ctf_psymtab *pst,
-			 CORE_ADDR end_addr, int section)
+			 CORE_ADDR end_addr)
 {
   struct ctf_context *ccp;
 
   ccp = &pst->context;
   struct compunit_symtab *result
-    = ccp->builder->end_compunit_symtab (end_addr, section);
+    = ccp->builder->end_compunit_symtab (end_addr);
   delete ccp->builder;
   ccp->builder = nullptr;
   return result;
@@ -1406,13 +1406,13 @@ ctf_psymtab::read_symtab (struct objfile *objfile)
       int tsize;
 
       offset = get_objfile_text_range (objfile, &tsize);
-      ctf_start_compunit_symtab (this, objfile, offset);
+      ctf_start_compunit_symtab (this, objfile, offset,
+				 SECT_OFF_TEXT (objfile));
       expand_psymtab (objfile);
 
       set_text_low (offset);
       set_text_high (offset + tsize);
-      compunit_symtab = ctf_end_compunit_symtab (this, offset + tsize,
-						 SECT_OFF_TEXT (objfile));
+      compunit_symtab = ctf_end_compunit_symtab (this, offset + tsize);
 
       /* Finish up the debug error message.  */
       if (info_verbose)
diff --git a/gdb/dbxread.c b/gdb/dbxread.c
index dc11f0bb6a9..b2ca4a83933 100644
--- a/gdb/dbxread.c
+++ b/gdb/dbxread.c
@@ -2331,8 +2331,7 @@ read_ofile_symtab (struct objfile *objfile, legacy_psymtab *pst)
   if (get_last_source_start_addr () > text_offset)
     set_last_source_start_addr (text_offset);
 
-  pst->compunit_symtab = end_compunit_symtab (text_offset + text_size,
-					      SECT_OFF_TEXT (objfile));
+  pst->compunit_symtab = end_compunit_symtab (text_offset + text_size);
 
   end_stabs ();
 
@@ -2594,7 +2593,7 @@ process_one_symbol (int type, int desc, CORE_ADDR valu, const char *name,
 	      patch_subfile_names (get_current_subfile (), name);
 	      break;		/* Ignore repeated SOs.  */
 	    }
-	  end_compunit_symtab (valu, SECT_OFF_TEXT (objfile));
+	  end_compunit_symtab (valu);
 	  end_stabs ();
 	}
 
@@ -2606,7 +2605,8 @@ process_one_symbol (int type, int desc, CORE_ADDR valu, const char *name,
       function_start_offset = 0;
 
       start_stabs ();
-      start_compunit_symtab (objfile, name, NULL, valu, language);
+      start_compunit_symtab (objfile, name, NULL, valu, language,
+			     SECT_OFF_TEXT (objfile));
       record_debugformat ("stabs");
       break;
 
diff --git a/gdb/dwarf2/cu.c b/gdb/dwarf2/cu.c
index 9c1691c90e9..c4b3d9e8bd1 100644
--- a/gdb/dwarf2/cu.c
+++ b/gdb/dwarf2/cu.c
@@ -78,7 +78,8 @@ dwarf2_cu::start_compunit_symtab (const char *name, const char *comp_dir,
 
   m_builder.reset (new struct buildsym_compunit
 		   (this->per_objfile->objfile,
-		    name, comp_dir, name_for_id, lang (), low_pc));
+		    name, comp_dir, name_for_id, lang (), low_pc,
+		    SECT_OFF_TEXT (this->per_objfile->objfile)));
 
   list_in_scope = get_builder ()->get_file_symbols ();
 
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 6a7412ce834..612b7299c0f 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -8456,7 +8456,7 @@ process_full_comp_unit (dwarf2_cu *cu, enum language pretend_language)
   dwarf2_record_block_ranges (cu->dies, static_block, baseaddr, cu);
 
   cust = cu->get_builder ()->end_compunit_symtab_from_static_block
-    (static_block, SECT_OFF_TEXT (objfile), 0);
+    (static_block, 0);
 
   if (cust != NULL)
     {
@@ -8507,7 +8507,6 @@ process_full_type_unit (dwarf2_cu *cu,
 			enum language pretend_language)
 {
   dwarf2_per_objfile *per_objfile = cu->per_objfile;
-  struct objfile *objfile = per_objfile->objfile;
   struct compunit_symtab *cust;
   struct signatured_type *sig_type;
 
@@ -8541,7 +8540,7 @@ process_full_type_unit (dwarf2_cu *cu,
   if (tug_unshare->compunit_symtab == NULL)
     {
       buildsym_compunit *builder = cu->get_builder ();
-      cust = builder->end_expandable_symtab (0, SECT_OFF_TEXT (objfile));
+      cust = builder->end_expandable_symtab (0);
       tug_unshare->compunit_symtab = cust;
 
       if (cust != NULL)
diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
index 4feee39035e..609b51727aa 100644
--- a/gdb/mdebugread.c
+++ b/gdb/mdebugread.c
@@ -3968,7 +3968,7 @@ mdebug_expand_psymtab (legacy_psymtab *pst, struct objfile *objfile)
 		    {
 		      valu += section_offsets[SECT_OFF_TEXT (objfile)];
 		      previous_stab_code = N_SO;
-		      cust = end_compunit_symtab (valu, SECT_OFF_TEXT (objfile));
+		      cust = end_compunit_symtab (valu);
 		      end_stabs ();
 		      last_symtab_ended = 1;
 		    }
@@ -4028,8 +4028,7 @@ mdebug_expand_psymtab (legacy_psymtab *pst, struct objfile *objfile)
 
       if (! last_symtab_ended)
 	{
-	  cust = end_compunit_symtab (pst->raw_text_high (),
-				      SECT_OFF_TEXT (objfile));
+	  cust = end_compunit_symtab (pst->raw_text_high ());
 	  end_stabs ();
 	}
 
diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
index 52ae3aecb97..3b990d660f0 100644
--- a/gdb/xcoffread.c
+++ b/gdb/xcoffread.c
@@ -958,7 +958,7 @@ read_xcoff_symtab (struct objfile *objfile, legacy_psymtab *pst)
 
   start_stabs ();
   start_compunit_symtab (objfile, filestring, NULL, file_start_addr,
-			 pst_symtab_language);
+			 pst_symtab_language, SECT_OFF_TEXT (objfile));
   record_debugformat (debugfmt);
   symnum = ((struct xcoff_symloc *) pst->read_symtab_private)->first_symnum;
   max_symnum =
@@ -1045,14 +1045,14 @@ read_xcoff_symtab (struct objfile *objfile, legacy_psymtab *pst)
 	{
 	  if (get_last_source_file ())
 	    {
-	      pst->compunit_symtab = end_compunit_symtab
-		(cur_src_end_addr, SECT_OFF_TEXT (objfile));
+	      pst->compunit_symtab = end_compunit_symtab (cur_src_end_addr);
 	      end_stabs ();
 	    }
 
 	  start_stabs ();
 	  start_compunit_symtab (objfile, "_globals_", NULL,
-				 0, pst_symtab_language);
+				 0, pst_symtab_language,
+				 SECT_OFF_TEXT (objfile));
 	  record_debugformat (debugfmt);
 	  cur_src_end_addr = first_object_file_end;
 	  /* Done with all files, everything from here on is globals.  */
@@ -1136,14 +1136,14 @@ read_xcoff_symtab (struct objfile *objfile, legacy_psymtab *pst)
 			{
 			  complete_symtab (filestring, file_start_addr);
 			  cur_src_end_addr = file_end_addr;
-			  end_compunit_symtab (file_end_addr,
-					       SECT_OFF_TEXT (objfile));
+			  end_compunit_symtab (file_end_addr);
 			  end_stabs ();
 			  start_stabs ();
 			  /* Give all csects for this source file the same
 			     name.  */
 			  start_compunit_symtab (objfile, filestring, NULL,
-					0, pst_symtab_language);
+						 0, pst_symtab_language,
+						 SECT_OFF_TEXT (objfile));
 			  record_debugformat (debugfmt);
 			}
 
@@ -1243,7 +1243,7 @@ read_xcoff_symtab (struct objfile *objfile, legacy_psymtab *pst)
 
 	  complete_symtab (filestring, file_start_addr);
 	  cur_src_end_addr = file_end_addr;
-	  end_compunit_symtab (file_end_addr, SECT_OFF_TEXT (objfile));
+	  end_compunit_symtab (file_end_addr);
 	  end_stabs ();
 
 	  /* XCOFF, according to the AIX 3.2 documentation, puts the
@@ -1263,7 +1263,7 @@ read_xcoff_symtab (struct objfile *objfile, legacy_psymtab *pst)
 
 	  start_stabs ();
 	  start_compunit_symtab (objfile, filestring, NULL, 0,
-				 pst_symtab_language);
+				 pst_symtab_language, SECT_OFF_TEXT (objfile));
 	  record_debugformat (debugfmt);
 	  last_csect_name = 0;
 
@@ -1431,7 +1431,7 @@ read_xcoff_symtab (struct objfile *objfile, legacy_psymtab *pst)
 
       complete_symtab (filestring, file_start_addr);
       cur_src_end_addr = file_end_addr;
-      cust = end_compunit_symtab (file_end_addr, SECT_OFF_TEXT (objfile));
+      cust = end_compunit_symtab (file_end_addr);
       /* When reading symbols for the last C_FILE of the objfile, try
 	 to make sure that we set pst->compunit_symtab to the symtab for the
 	 file, not to the _globals_ symtab.  I'm not sure whether this
-- 
2.38.1

Re: [PATCH 3/6] Pass section index to start_compunit_symtab

[Simon Marchi]

>  /* Subroutine of augment_type_symtab to simplify it.
> diff --git a/gdb/buildsym.h b/gdb/buildsym.h
> index 9724607f3d9..0464c738114 100644
> --- a/gdb/buildsym.h
> +++ b/gdb/buildsym.h
> @@ -146,18 +146,23 @@ struct buildsym_compunit
>       (or NULL if not known).
>  
>       NAME and NAME_FOR_ID have the same purpose as for the start_subfile
> -     method.  */
> +     method.
> +
> +     SECTION_INDEX is the index of the section for the compunit and
> +     for block symbols in this compunit.  Normally SECT_OFF_TEXT.  */

I don't understand this comment, specifically how a compunit can "have"
a single section.  Don't compunits define data and text symbols, which
have different sections, for instance?

From what I can see, this section index is ultimately use to apply /
unapply a relocation offset when dealing with call sites (e.g.
compunit_symtab::find_call_site), so it makes sense that we want the
text section (call site addresses are code addresses).  So maybe the
comment and / or the field name could reflect that this is the section
index for the code / text section.  Just saying "section index" doesn't
tell much about which section it is.

I'm curious to know why we have to store that and we can't always use
SECT_OFF_TEXT, but I guess that will come with the following patches.

Simon

Re: [PATCH 3/6] Pass section index to start_compunit_symtab

[Tom Tromey]

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

>> +     SECTION_INDEX is the index of the section for the compunit and
>> +     for block symbols in this compunit.  Normally SECT_OFF_TEXT.  */

Simon> I don't understand this comment, specifically how a compunit can "have"
Simon> a single section.  Don't compunits define data and text symbols, which
Simon> have different sections, for instance?

I can reword it, this only refers to the section for blocks.  See
compunit_symtab::m_block_line_section.

Simon> I'm curious to know why we have to store that and we can't always use
Simon> SECT_OFF_TEXT, but I guess that will come with the following patches.

I suspect this is dead code, I just didn't change it in this series.
I could though if you'd prefer.

Tom

Re: [PATCH 3/6] Pass section index to start_compunit_symtab

[Simon Marchi]

On 1/18/23 17:02, Tom Tromey via Gdb-patches wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
>>> +     SECTION_INDEX is the index of the section for the compunit and
>>> +     for block symbols in this compunit.  Normally SECT_OFF_TEXT.  */
> 
> Simon> I don't understand this comment, specifically how a compunit can "have"
> Simon> a single section.  Don't compunits define data and text symbols, which
> Simon> have different sections, for instance?
> 
> I can reword it, this only refers to the section for blocks.  See
> compunit_symtab::m_block_line_section.
> 
> Simon> I'm curious to know why we have to store that and we can't always use
> Simon> SECT_OFF_TEXT, but I guess that will come with the following patches.
> 
> I suspect this is dead code, I just didn't change it in this series.
> I could though if you'd prefer.

I suppose that if it's indeed always the text section that is passed, it
would be easy to spot.

Simon

Re: [PATCH 3/6] Pass section index to start_compunit_symtab

[Tom Tromey]

Simon> I'm curious to know why we have to store that and we can't always use
Simon> SECT_OFF_TEXT, but I guess that will come with the following patches.

>> I suspect this is dead code, I just didn't change it in this series.
>> I could though if you'd prefer.

Simon> I suppose that if it's indeed always the text section that is passed, it
Simon> would be easy to spot.

I've updated patch #3 to just remove the section index from
end_compunit_symtab, and I've added a new patch to the series to remove
compunit_symtab::m_block_line_section.

Tom

Re: [PATCH 3/6] Pass section index to start_compunit_symtab

[Tom Tromey]

>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> I've updated patch #3 to just remove the section index from
Tom> end_compunit_symtab, and I've added a new patch to the series to remove
Tom> compunit_symtab::m_block_line_section.

Here's that patch.  I realized I forgot to send it to the list, but I
thought I should note it here before pushing this series.

Tom

commit d401e7bf04c0949dcc5e3d83143b75efc19d5f1e
Author: Tom Tromey <tromey@adacore.com>
Date:   Thu Jan 19 06:14:49 2023 -0700

    Remove compunit_symtab::m_block_line_section
    
    The previous patch hard-coded SECT_OFF_TEXT into the buildsym code.
    After this, it's clear that there is only one caller of
    compunit_symtab::set_block_line_section, and it always passes
    SECT_OFF_TEXT.  So, remove compunit_symtab::m_block_line_section and
    use SECT_OFF_TEXT instead.

diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index 41df0f061c1..d82c7672f7c 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -974,8 +974,6 @@ buildsym_compunit::end_compunit_symtab_with_blockvector
     set_block_compunit_symtab (b, cu);
   }
 
-  cu->set_block_line_section (SECT_OFF_TEXT (m_objfile));
-
   cu->set_macro_table (release_macros ());
 
   /* Default any symbols without a specified symtab to the primary symtab.  */
diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index fe91d609f19..236ad820462 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -718,9 +718,7 @@ call_site_target::iterate_over_addresses
     case call_site_target::PHYSADDR:
       {
 	dwarf2_per_objfile *per_objfile = call_site->per_objfile;
-	compunit_symtab *cust = per_objfile->get_symtab (call_site->per_cu);
-	int sect_idx = cust->block_line_section ();
-	CORE_ADDR delta = per_objfile->objfile->section_offsets[sect_idx];
+	CORE_ADDR delta = per_objfile->objfile->text_section_offset ();
 
 	callback (m_loc.physaddr + delta);
       }
@@ -729,9 +727,7 @@ call_site_target::iterate_over_addresses
     case call_site_target::ADDRESSES:
       {
 	dwarf2_per_objfile *per_objfile = call_site->per_objfile;
-	compunit_symtab *cust = per_objfile->get_symtab (call_site->per_cu);
-	int sect_idx = cust->block_line_section ();
-	CORE_ADDR delta = per_objfile->objfile->section_offsets[sect_idx];
+	CORE_ADDR delta = per_objfile->objfile->text_section_offset ();
 
 	for (unsigned i = 0; i < m_loc.addresses.length; ++i)
 	  callback (m_loc.addresses.values[i] + delta);
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 6a4c5976f18..45751c9c895 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -6436,9 +6436,7 @@ objfile_type (struct objfile *objfile)
 CORE_ADDR
 call_site::pc () const
 {
-  compunit_symtab *cust = this->per_objfile->get_symtab (this->per_cu);
-  CORE_ADDR delta
-	= this->per_objfile->objfile->section_offsets[cust->block_line_section ()];
+  CORE_ADDR delta = this->per_objfile->objfile->text_section_offset ();
   return m_unrelocated_pc + delta;
 }
 
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index e4fe3562c12..bf5057e723d 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -629,7 +629,7 @@ objfile_relocate1 (struct objfile *objfile,
 	    if (l)
 	      {
 		for (int i = 0; i < l->nitems; ++i)
-		  l->item[i].pc += delta[cust->block_line_section ()];
+		  l->item[i].pc += delta[SECT_OFF_TEXT (objfile)];
 	      }
 	  }
       }
@@ -637,7 +637,7 @@ objfile_relocate1 (struct objfile *objfile,
     for (compunit_symtab *cust : objfile->compunits ())
       {
 	struct blockvector *bv = cust->blockvector ();
-	int block_line_section = cust->block_line_section ();
+	int block_line_section = SECT_OFF_TEXT (objfile);
 
 	if (bv->map () != nullptr)
 	  bv->map ()->relocate (delta[block_line_section]);
diff --git a/gdb/symtab.c b/gdb/symtab.c
index a120c1d5e15..bf3a3e3caaa 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -342,8 +342,7 @@ compunit_symtab::find_call_site (CORE_ADDR pc) const
   if (m_call_site_htab == nullptr)
     return nullptr;
 
-  CORE_ADDR delta
-    = this->objfile ()->section_offsets[this->block_line_section ()];
+  CORE_ADDR delta = this->objfile ()->text_section_offset ();
   CORE_ADDR unrelocated_pc = pc - delta;
 
   struct call_site call_site_local (unrelocated_pc, nullptr, nullptr);
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 484644f6129..50ce7525793 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1786,16 +1786,6 @@ struct compunit_symtab
     m_blockvector = blockvector;
   }
 
-  int block_line_section () const
-  {
-    return m_block_line_section;
-  }
-
-  void set_block_line_section (int block_line_section)
-  {
-    m_block_line_section = block_line_section;
-  }
-
   bool locations_valid () const
   {
     return m_locations_valid;
@@ -1884,10 +1874,6 @@ struct compunit_symtab
      all symtabs in a given compilation unit.  */
   struct blockvector *m_blockvector;
 
-  /* Section in objfile->section_offsets for the blockvector and
-     the linetable.  Probably always SECT_OFF_TEXT.  */
-  int m_block_line_section;
-
   /* Symtab has been compiled with both optimizations and debug info so that
      GDB may stop skipping prologues as variables locations are valid already
      at function entry points.  */

[PATCH 4/6] Set section index when setting a symbol's block

[Tom Tromey]

When a symbol's block is set, the block has the runtime section offset
applied.  So, it seems to me that the symbol implicitly is in the same
section as the block.  Therefore, this patch sets the symbol's section
index at this same spot.
---
 gdb/buildsym.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index 8c61223b1a0..fa55699b40e 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -247,6 +247,7 @@ buildsym_compunit::finish_block_internal
       struct type *ftype = symbol->type ();
       struct mdict_iterator miter;
       symbol->set_value_block (block);
+      symbol->set_section_index (m_section_index);
       block->set_function (symbol);
 
       if (ftype->num_fields () <= 0)
-- 
2.38.1

[PATCH 5/6] Remove most calls to fixup_symbol_section

[Tom Tromey]

Nearly every call to fixup_symbol_section in gdb is incorrect, and if
any such call has an effect, it's purely by happenstance.

fixup_section has a long comment explaining that the call should only
be made before runtime section offsets are applied.  And, the loop in
this code (the fallback loop -- the minsym lookup code is "ok") is
careful to remove these offsets before comparing addresses.

However, aside from a single call in dwarf2/read.c, every call in gdb
is actually done after section offsets have been applied.  So, these
calls are incorrect.

Now, these calls could be made when the symbol is created.  I
considered this approach, but I reasoned that the code has been this
way for many years, seemingly without ill effect.  So, instead I chose
to simply remove the offending calls.
---
 gdb/ada-lang.c   | 28 +++++++---------------------
 gdb/breakpoint.c |  7 ++-----
 gdb/infcmd.c     |  1 -
 gdb/objfiles.c   |  2 --
 gdb/symtab.c     |  8 +-------
 5 files changed, 10 insertions(+), 36 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 40f85914a56..3cd6f73f36f 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -5389,9 +5389,7 @@ match_data::operator() (struct block_symbol *bsym)
   if (sym == NULL)
     {
       if (!found_sym && arg_sym != NULL)
-	add_defn_to_vec (*resultp,
-			 fixup_symbol_section (arg_sym, objfile),
-			 block);
+	add_defn_to_vec (*resultp, arg_sym, block);
       found_sym = false;
       arg_sym = NULL;
     }
@@ -5404,9 +5402,7 @@ match_data::operator() (struct block_symbol *bsym)
       else
 	{
 	  found_sym = true;
-	  add_defn_to_vec (*resultp,
-			   fixup_symbol_section (sym, objfile),
-			   block);
+	  add_defn_to_vec (*resultp, sym, block);
 	}
     }
   return true;
@@ -5808,9 +5804,7 @@ ada_lookup_symbol (const char *name, const struct block *block0,
   if (candidates.empty ())
     return {};
 
-  block_symbol info = candidates[0];
-  info.symbol = fixup_symbol_section (info.symbol, NULL);
-  return info;
+  return candidates[0];
 }
 
 
@@ -6098,9 +6092,7 @@ ada_add_block_symbols (std::vector<struct block_symbol> &result,
 	      else
 		{
 		  found_sym = true;
-		  add_defn_to_vec (result,
-				   fixup_symbol_section (sym, objfile),
-				   block);
+		  add_defn_to_vec (result, sym, block);
 		}
 	    }
 	}
@@ -6113,9 +6105,7 @@ ada_add_block_symbols (std::vector<struct block_symbol> &result,
 
   if (!found_sym && arg_sym != NULL)
     {
-      add_defn_to_vec (result,
-		       fixup_symbol_section (arg_sym, objfile),
-		       block);
+      add_defn_to_vec (result, arg_sym, block);
     }
 
   if (!lookup_name.ada ().wild_match_p ())
@@ -6152,9 +6142,7 @@ ada_add_block_symbols (std::vector<struct block_symbol> &result,
 		    else
 		      {
 			found_sym = true;
-			add_defn_to_vec (result,
-					 fixup_symbol_section (sym, objfile),
-					 block);
+			add_defn_to_vec (result, sym, block);
 		      }
 		  }
 	      }
@@ -6165,9 +6153,7 @@ ada_add_block_symbols (std::vector<struct block_symbol> &result,
 	 They aren't parameters, right?  */
       if (!found_sym && arg_sym != NULL)
 	{
-	  add_defn_to_vec (result,
-			   fixup_symbol_section (arg_sym, objfile),
-			   block);
+	  add_defn_to_vec (result, arg_sym, block);
 	}
     }
 }
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 00cc2ab401c..306c30aefac 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -9196,11 +9196,8 @@ resolve_sal_pc (struct symtab_and_line *sal)
 	{
 	  sym = block_linkage_function (b);
 	  if (sym != NULL)
-	    {
-	      fixup_symbol_section (sym, sal->symtab->compunit ()->objfile ());
-	      sal->section
-		= sym->obj_section (sal->symtab->compunit ()->objfile ());
-	    }
+	    sal->section
+	      = sym->obj_section (sal->symtab->compunit ()->objfile ());
 	  else
 	    {
 	      /* It really is worthwhile to have the section, so we'll
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 978c07f176d..fe100532454 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1106,7 +1106,6 @@ jump_command (const char *arg, int from_tty)
     {
       struct obj_section *section;
 
-      fixup_symbol_section (sfn, 0);
       section = sfn->obj_section (sfn->objfile ());
       if (section_is_overlay (section)
 	  && !section_is_mapped (section))
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 411bf121ede..869f955488e 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -583,8 +583,6 @@ static void
 relocate_one_symbol (struct symbol *sym, struct objfile *objfile,
 		     const section_offsets &delta)
 {
-  fixup_symbol_section (sym, objfile);
-
   /* 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
diff --git a/gdb/symtab.c b/gdb/symtab.c
index fe247ab70eb..2d3aafd140a 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -2248,7 +2248,7 @@ lookup_symbol_in_block (const char *name, symbol_name_match_type match_type,
     {
       symbol_lookup_debug_printf_v ("lookup_symbol_in_block (...) = %s",
 				    host_address_to_string (sym));
-      return fixup_symbol_section (sym, NULL);
+      return sym;
     }
 
   symbol_lookup_debug_printf_v ("lookup_symbol_in_block (...) = NULL");
@@ -2333,7 +2333,6 @@ lookup_symbol_in_objfile_symtabs (struct objfile *objfile,
 	("lookup_symbol_in_objfile_symtabs (...) = %s (block %s)",
 	 host_address_to_string (other.symbol),
 	 host_address_to_string (other.block));
-      other.symbol = fixup_symbol_section (other.symbol, objfile);
       return other;
     }
 
@@ -2439,7 +2438,6 @@ lookup_symbol_via_quick_fns (struct objfile *objfile,
      host_address_to_string (result.symbol),
      host_address_to_string (block));
 
-  result.symbol = fixup_symbol_section (result.symbol, objfile);
   result.block = block;
   return result;
 }
@@ -2921,7 +2919,6 @@ find_pc_sect_compunit_symtab (CORE_ADDR pc, struct obj_section *section)
 		  const struct block *b = bv->block (b_index);
 		  ALL_BLOCK_SYMBOLS (b, iter, sym)
 		    {
-		      fixup_symbol_section (sym, obj_file);
 		      if (matching_obj_sections (sym->obj_section (obj_file),
 						 section))
 			break;
@@ -3664,7 +3661,6 @@ find_function_start_sal (CORE_ADDR func_addr, obj_section *section,
 symtab_and_line
 find_function_start_sal (symbol *sym, bool funfirstline)
 {
-  fixup_symbol_section (sym, NULL);
   symtab_and_line sal
     = find_function_start_sal_1 (sym->value_block ()->entry_pc (),
 				 sym->obj_section (sym->objfile ()),
@@ -3792,8 +3788,6 @@ skip_prologue_sal (struct symtab_and_line *sal)
   sym = find_pc_sect_function (sal->pc, sal->section);
   if (sym != NULL)
     {
-      fixup_symbol_section (sym, NULL);
-
       objfile = sym->objfile ();
       pc = sym->value_block ()->entry_pc ();
       section = sym->obj_section (objfile);
-- 
2.38.1

[PATCH 6/6] Merge fixup_section and fixup_symbol_section

[Tom Tromey]

fixup_symbol_section delegates all its work to fixup_section, so merge
the two.

Because there is only a single caller to fixup_symbol_section, we can
also remove some of the introductory logic.  For example, this will
never be called with a NULL objfile any more.

The LOC_BLOCK case can be removed, because such symbols are handled by
the buildsym code now.

Finally, it a symbol can only appear in a SEC_ALLOC section, so the
loop is modified to skip sections that do not have this flag set.
---
 gdb/symtab.c | 103 +++++++++++++++++++--------------------------------
 gdb/symtab.h |   8 +++-
 2 files changed, 45 insertions(+), 66 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 2d3aafd140a..82d6bc28795 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -1701,14 +1701,39 @@ symtab_free_objfile_observer (struct objfile *objfile)
   symbol_cache_flush (objfile->pspace);
 }
 \f
-/* Debug symbols usually don't have section information.  We need to dig that
-   out of the minimal symbols and stash that in the debug symbol.  */
+/* See symtab.h.  */
 
-static void
-fixup_section (struct general_symbol_info *ginfo,
-	       CORE_ADDR addr, struct objfile *objfile,
-	       int default_section)
+void
+fixup_symbol_section (struct symbol *sym, struct objfile *objfile)
 {
+  gdb_assert (sym != nullptr);
+  gdb_assert (sym->is_objfile_owned ());
+  gdb_assert (objfile != nullptr);
+  gdb_assert (sym->section_index () == -1);
+
+  /* Note that if this ends up as -1, fixup_section will handle that
+     reasonably well.  So, it's fine to use the objfile's section
+     index without doing the check that is done by the wrapper macros
+     like SECT_OFF_TEXT.  */
+  int fallback;
+  switch (sym->aclass ())
+    {
+    case LOC_STATIC:
+      fallback = objfile->sect_index_data;
+      break;
+
+    case LOC_LABEL:
+      fallback = objfile->sect_index_text;
+      break;
+
+    default:
+      /* Nothing else will be listed in the minsyms -- no use looking
+	 it up.  */
+      return;
+    }
+
+  CORE_ADDR addr = sym->value_address ();
+
   struct minimal_symbol *msym;
 
   /* First, check whether a minimal symbol with the same name exists
@@ -1716,10 +1741,10 @@ fixup_section (struct general_symbol_info *ginfo,
      e.g. on PowerPC64, where the minimal symbol for a function will
      point to the function descriptor, while the debug symbol will
      point to the actual function code.  */
-  msym = lookup_minimal_symbol_by_pc_name (addr, ginfo->linkage_name (),
+  msym = lookup_minimal_symbol_by_pc_name (addr, sym->linkage_name (),
 					   objfile);
   if (msym)
-    ginfo->set_section_index (msym->section_index ());
+    sym->set_section_index (msym->section_index ());
   else
     {
       /* Static, function-local variables do appear in the linker
@@ -1758,10 +1783,12 @@ fixup_section (struct general_symbol_info *ginfo,
 	 a search of the section table.  */
 
       struct obj_section *s;
-      int fallback = default_section;
 
       ALL_OBJFILE_OSECTIONS (objfile, s)
 	{
+	  if ((bfd_section_flags (s->the_bfd_section) & SEC_ALLOC) == 0)
+	    continue;
+
 	  int idx = s - objfile->sections;
 	  CORE_ADDR offset = objfile->section_offsets[idx];
 
@@ -1770,7 +1797,7 @@ fixup_section (struct general_symbol_info *ginfo,
 
 	  if (s->addr () - offset <= addr && addr < s->endaddr () - offset)
 	    {
-	      ginfo->set_section_index (idx);
+	      sym->set_section_index (idx);
 	      return;
 	    }
 	}
@@ -1779,64 +1806,12 @@ fixup_section (struct general_symbol_info *ginfo,
 	 section.  If there is no allocated section, then it hardly
 	 matters what we pick, so just pick zero.  */
       if (fallback == -1)
-	ginfo->set_section_index (0);
+	sym->set_section_index (0);
       else
-	ginfo->set_section_index (fallback);
+	sym->set_section_index (fallback);
     }
 }
 
-struct symbol *
-fixup_symbol_section (struct symbol *sym, struct objfile *objfile)
-{
-  CORE_ADDR addr;
-
-  if (!sym)
-    return NULL;
-
-  if (!sym->is_objfile_owned ())
-    return sym;
-
-  /* We either have an OBJFILE, or we can get at it from the sym's
-     symtab.  Anything else is a bug.  */
-  gdb_assert (objfile || sym->symtab ());
-
-  if (objfile == NULL)
-    objfile = sym->objfile ();
-
-  if (sym->obj_section (objfile) != nullptr)
-    return sym;
-
-  /* We should have an objfile by now.  */
-  gdb_assert (objfile);
-
-  /* Note that if this ends up as -1, fixup_section will handle that
-     reasonably well.  So, it's fine to use the objfile's section
-     index without doing the check that is done by the wrapper macros
-     like SECT_OFF_TEXT.  */
-  int default_section = objfile->sect_index_text;
-  switch (sym->aclass ())
-    {
-    case LOC_STATIC:
-      default_section = objfile->sect_index_data;
-      /* FALLTHROUGH.  */
-    case LOC_LABEL:
-      addr = sym->value_address ();
-      break;
-    case LOC_BLOCK:
-      addr = sym->value_block ()->entry_pc ();
-      break;
-
-    default:
-      /* Nothing else will be listed in the minsyms -- no use looking
-	 it up.  */
-      return sym;
-    }
-
-  fixup_section (sym, addr, objfile, default_section);
-
-  return sym;
-}
-
 /* See symtab.h.  */
 
 demangle_for_lookup_info::demangle_for_lookup_info
diff --git a/gdb/symtab.h b/gdb/symtab.h
index ac3f6391dc3..50b801830c7 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2394,8 +2394,12 @@ extern void skip_prologue_sal (struct symtab_and_line *);
 extern CORE_ADDR skip_prologue_using_sal (struct gdbarch *gdbarch,
 					  CORE_ADDR func_addr);
 
-extern struct symbol *fixup_symbol_section (struct symbol *,
-					    struct objfile *);
+/* If SYM requires a section index, find it either via minimal symbols
+   or examining OBJFILE's sections.  Note that SYM's current address
+   must not have any runtime offsets applied.  */
+
+extern void fixup_symbol_section (struct symbol *sym,
+				  struct objfile *objfile);
 
 /* If MSYMBOL is an text symbol, look for a function debug symbol with
    the same address.  Returns NULL if not found.  This is necessary in
-- 
2.38.1

Re: [PATCH 0/6] Change how symbol section indices are set

[Tom Tromey]

>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> An otherwise innocuous internal AdaCore test case started failing when
Tom> linked with 'mold'.  Investigation revealed that the problem was
Tom> actually in gdb -- gdb decided that a certain symbol was in the
Tom> .interp section.

Tom> Digging deeper, I found that most calls to fixup_symbol_section are
Tom> incorrect, and that if a section is chosen, it's frequently the wrong
Tom> one.

Tom> This series attempts to clean up this area.  Regression tested on
Tom> x86-64 Fedora 36.

I'm checking this in now.

Tom