diff mbox series

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

Message ID 20230118153025.342512-3-tromey@adacore.com
State Committed
Commit b8a6e9878141f5f1ac6c790e85257eadd51fff8f
Headers
Series Change how symbol section indices are set |

Commit Message

Tom Tromey Jan. 18, 2023, 3:30 p.m. UTC 
  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(-)

Comments

Simon Marchi Jan. 18, 2023, 9:43 p.m. UTC | #1 
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
Tom Tromey Jan. 18, 2023, 10 p.m. UTC | #2 
>>>>> "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
diff mbox series

Patch

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);
+	}
     }
 }
 
@@ -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));