diff mbox

[RFA,3/6] Make PSYMBOL_VALUE_ADDRESS take objfile argument

Message ID 20180503223621.22544-4-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey May 3, 2018, 10:36 p.m. UTC 
  This changes PSYMBOL_VALUE_ADDRESS to take an objfile argument.  This
is necessary so that we can later relocate a partial symbol at its
point of use.  It also adds a "raw" form of the accessor, to compute
the unrelocated value; and a special macro to be used for setting the
field.

Note that the macro doesn't actually perform any relocation -- it uses
the argument, to ensure that any possible syntax errors are caught,
but it multiplies it by 0 so that it has no effect.

ChangeLog
2018-05-03  Tom Tromey  <tromey@redhat.com>

	* psympriv.h (SET_PSYMBOL_VALUE_ADDRESS)
	(PSYMBOL_VALUE_RAW_ADDRESS): New macros.
	(PSYMBOL_VALUE_ADDRESS): Add 'objfile' parameter.
	* psymtab.c (find_pc_sect_psymtab_closer, find_pc_sect_psymbol)
	(fixup_psymbol_section, relocate_psymtabs): Update.
	(print_partial_symbols): Add 'objfile' parameter.  Update.
	(dump_psymtab, add_psymbol_to_bcache, psym_fill_psymbol_map):
	Update.
---
 gdb/ChangeLog  | 11 +++++++++++
 gdb/psympriv.h |  7 ++++++-
 gdb/psymtab.c  | 50 ++++++++++++++++++++++++++++----------------------
 3 files changed, 45 insertions(+), 23 deletions(-)

Comments

Keith Seitz June 1, 2018, 8:06 p.m. UTC | #1 
On 05/03/2018 03:36 PM, Tom Tromey wrote:
>
> Note that the macro doesn't actually perform any relocation -- it uses
> the argument, to ensure that any possible syntax errors are caught,
> but it multiplies it by 0 so that it has no effect.

Just a word of warning: ANOFFSET will assert if PSYMBOL_SECTION is -1. [Setting that field isn't added until the last patch.] As a result, I am seeing a lot of temporary regressions. I build with -O0, so maybe these won't show up during a buildbot test run?

Example from gdb.base/reread.exp:

run
Starting program: /home/keiths/work/gdb/branches/tromey-psymbols-progspace.patch
/linux/gdb/testsuite/outputs/gdb.base/reread/reread
../../src/gdb/psymtab.c:493: internal-error: Section index is uninitialized
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) FAIL: gdb.base/reread.exp: opts= "-fPIE" "ldflags=-pie" : run to foo() (GDB internal error)
Resyncing due to internal error.

> ChangeLog
> 2018-05-03  Tom Tromey  <tromey@redhat.com>
> 
> 	* psympriv.h (SET_PSYMBOL_VALUE_ADDRESS)
> 	(PSYMBOL_VALUE_RAW_ADDRESS): New macros.
> 	(PSYMBOL_VALUE_ADDRESS): Add 'objfile' parameter.
> 	* psymtab.c (find_pc_sect_psymtab_closer, find_pc_sect_psymbol)
> 	(fixup_psymbol_section, relocate_psymtabs): Update.
> 	(print_partial_symbols): Add 'objfile' parameter.  Update.
> 	(dump_psymtab, add_psymbol_to_bcache, psym_fill_psymbol_map):
> 	Update.

With that caveat, (and IANAM), this LGTM.

Keith
Tom Tromey June 1, 2018, 9:03 p.m. UTC | #2 
>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> Just a word of warning: ANOFFSET will assert if PSYMBOL_SECTION is
Keith> -1. [Setting that field isn't added until the last patch.] As a
Keith> result, I am seeing a lot of temporary regressions. I build with -O0,
Keith> so maybe these won't show up during a buildbot test run?

Maybe they would, and anyway it seems like bad form to rely on this
being optimized out.

Since the goal here is just to do some syntax checking maybe there is
another form that could be used, like

(0 ? 0 : (ANOFFSET ((objfile)->section_offsets, ((symbol)->pginfo.section))))

Not sure if that will trigger warnings, but I can find out.

If that doesn't work one idea would be to just drop it from the patch,
since it has done its job already.

Tom
Simon Marchi June 5, 2018, 8:49 p.m. UTC | #3 
On 2018-06-01 17:03, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
> 
> Keith> Just a word of warning: ANOFFSET will assert if PSYMBOL_SECTION 
> is
> Keith> -1. [Setting that field isn't added until the last patch.] As a
> Keith> result, I am seeing a lot of temporary regressions. I build with 
> -O0,
> Keith> so maybe these won't show up during a buildbot test run?
> 
> Maybe they would, and anyway it seems like bad form to rely on this
> being optimized out.
> 
> Since the goal here is just to do some syntax checking maybe there is
> another form that could be used, like
> 
> (0 ? 0 : (ANOFFSET ((objfile)->section_offsets, 
> ((symbol)->pginfo.section))))
> 
> Not sure if that will trigger warnings, but I can find out.
> 
> If that doesn't work one idea would be to just drop it from the patch,
> since it has done its job already.
> 
> Tom

Or change these macros to be functions :).  Types will be properly 
checked even though the parameter is unused.

Simon
diff mbox

Patch

diff --git a/gdb/psympriv.h b/gdb/psympriv.h
index 31f2a2b3e9..8daaa377b7 100644
--- a/gdb/psympriv.h
+++ b/gdb/psympriv.h
@@ -51,7 +51,12 @@  struct partial_symbol
 };
 
 #define PSYMBOL_VALUE(symbol)		(symbol)->pginfo.value.ivalue
-#define PSYMBOL_VALUE_ADDRESS(symbol)	(symbol)->pginfo.value.address
+#define SET_PSYMBOL_VALUE_ADDRESS(symbol, addr) \
+  (((symbol)->pginfo.value.address) = (addr))
+#define PSYMBOL_VALUE_RAW_ADDRESS(symbol) ((symbol)->pginfo.value.address + 0)
+#define PSYMBOL_VALUE_ADDRESS(objfile, symbol)	\
+  ((symbol)->pginfo.value.address \
+   + (0 * (ANOFFSET ((objfile)->section_offsets, ((symbol)->pginfo.section)))))
 #define PSYMBOL_LANGUAGE(symbol)	(symbol)->pginfo.language
 #define PSYMBOL_SECTION(symbol)		(symbol)->pginfo.section
 #define PSYMBOL_OBJ_SECTION(objfile, symbol)			\
diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index e9a6a23b9d..3b6f0d53b5 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -267,7 +267,7 @@  find_pc_sect_psymtab_closer (struct objfile *objfile,
 	     object's symbol table.  */
 	  p = find_pc_sect_psymbol (objfile, tpst, pc, section);
 	  if (p != NULL
-	      && (PSYMBOL_VALUE_ADDRESS (p)
+	      && (PSYMBOL_VALUE_ADDRESS (objfile, p)
 		  == BMSYMBOL_VALUE_ADDRESS (msymbol)))
 	    return tpst;
 
@@ -276,7 +276,7 @@  find_pc_sect_psymtab_closer (struct objfile *objfile,
 	     symbol tables with line information but no debug
 	     symbols (e.g. those produced by an assembler).  */
 	  if (p != NULL)
-	    this_addr = PSYMBOL_VALUE_ADDRESS (p);
+	    this_addr = PSYMBOL_VALUE_ADDRESS (objfile, p);
 	  else
 	    this_addr = tpst->textlow;
 
@@ -334,7 +334,7 @@  find_pc_sect_psymtab (struct objfile *objfile, CORE_ADDR pc,
 		 object's symbol table.  */
 	      p = find_pc_sect_psymbol (objfile, pst, pc, section);
 	      if (p == NULL
-		  || (PSYMBOL_VALUE_ADDRESS (p)
+		  || (PSYMBOL_VALUE_ADDRESS (objfile, p)
 		      != BMSYMBOL_VALUE_ADDRESS (msymbol)))
 		goto next;
 	    }
@@ -427,10 +427,11 @@  find_pc_sect_psymbol (struct objfile *objfile,
 
       if (SYMBOL_DOMAIN (p) == VAR_DOMAIN
 	  && PSYMBOL_CLASS (p) == LOC_BLOCK
-	  && pc >= PSYMBOL_VALUE_ADDRESS (p)
-	  && (PSYMBOL_VALUE_ADDRESS (p) > best_pc
+	  && pc >= PSYMBOL_VALUE_ADDRESS (objfile, p)
+	  && (PSYMBOL_VALUE_ADDRESS (objfile, p) > best_pc
 	      || (psymtab->textlow == 0
-		  && best_pc == 0 && PSYMBOL_VALUE_ADDRESS (p) == 0)))
+		  && best_pc == 0
+		  && PSYMBOL_VALUE_ADDRESS (objfile, p) == 0)))
 	{
 	  if (section != NULL)  /* Match on a specific section.  */
 	    {
@@ -439,7 +440,7 @@  find_pc_sect_psymbol (struct objfile *objfile,
 					  section))
 		continue;
 	    }
-	  best_pc = PSYMBOL_VALUE_ADDRESS (p);
+	  best_pc = PSYMBOL_VALUE_ADDRESS (objfile, p);
 	  best = p;
 	}
     }
@@ -450,10 +451,11 @@  find_pc_sect_psymbol (struct objfile *objfile,
 
       if (SYMBOL_DOMAIN (p) == VAR_DOMAIN
 	  && PSYMBOL_CLASS (p) == LOC_BLOCK
-	  && pc >= PSYMBOL_VALUE_ADDRESS (p)
-	  && (PSYMBOL_VALUE_ADDRESS (p) > best_pc
+	  && pc >= PSYMBOL_VALUE_ADDRESS (objfile, p)
+	  && (PSYMBOL_VALUE_ADDRESS (objfile, p) > best_pc
 	      || (psymtab->textlow == 0
-		  && best_pc == 0 && PSYMBOL_VALUE_ADDRESS (p) == 0)))
+		  && best_pc == 0
+		  && PSYMBOL_VALUE_ADDRESS (objfile, p) == 0)))
 	{
 	  if (section != NULL)  /* Match on a specific section.  */
 	    {
@@ -462,7 +464,7 @@  find_pc_sect_psymbol (struct objfile *objfile,
 					  section))
 		continue;
 	    }
-	  best_pc = PSYMBOL_VALUE_ADDRESS (p);
+	  best_pc = PSYMBOL_VALUE_ADDRESS (objfile, p);
 	  best = p;
 	}
     }
@@ -488,7 +490,7 @@  fixup_psymbol_section (struct partial_symbol *psym, struct objfile *objfile)
     case LOC_STATIC:
     case LOC_LABEL:
     case LOC_BLOCK:
-      addr = PSYMBOL_VALUE_ADDRESS (psym);
+      addr = PSYMBOL_VALUE_ADDRESS (objfile, psym);
       break;
     default:
       /* Nothing else will be listed in the minsyms -- no use looking
@@ -815,15 +817,17 @@  psym_relocate (struct objfile *objfile,
     {
       fixup_psymbol_section (psym, objfile);
       if (PSYMBOL_SECTION (psym) >= 0)
-	PSYMBOL_VALUE_ADDRESS (psym) += ANOFFSET (delta,
-						  PSYMBOL_SECTION (psym));
+	SET_PSYMBOL_VALUE_ADDRESS (psym,
+				   PSYMBOL_VALUE_RAW_ADDRESS (psym)
+				   + ANOFFSET (delta, PSYMBOL_SECTION (psym)));
     }
   for (partial_symbol *psym : objfile->static_psymbols)
     {
       fixup_psymbol_section (psym, objfile);
       if (PSYMBOL_SECTION (psym) >= 0)
-	PSYMBOL_VALUE_ADDRESS (psym) += ANOFFSET (delta,
-						  PSYMBOL_SECTION (psym));
+	SET_PSYMBOL_VALUE_ADDRESS (psym,
+				   PSYMBOL_VALUE_RAW_ADDRESS (psym)
+				   + ANOFFSET (delta, PSYMBOL_SECTION (psym)));
     }
 
   objfile->psymbol_map.clear ();
@@ -887,7 +891,7 @@  psym_forget_cached_source_info (struct objfile *objfile)
 }
 
 static void
-print_partial_symbols (struct gdbarch *gdbarch,
+print_partial_symbols (struct gdbarch *gdbarch, struct objfile *objfile,
 		       struct partial_symbol **p, int count, const char *what,
 		       struct ui_file *outfile)
 {
@@ -971,7 +975,9 @@  print_partial_symbols (struct gdbarch *gdbarch,
 	  break;
 	}
       fputs_filtered (", ", outfile);
-      fputs_filtered (paddress (gdbarch, PSYMBOL_VALUE_ADDRESS (*p)), outfile);
+      fputs_filtered (paddress (gdbarch,
+				PSYMBOL_VALUE_RAW_ADDRESS (*p)),
+		      outfile);
       fprintf_filtered (outfile, "\n");
       p++;
     }
@@ -1036,13 +1042,13 @@  dump_psymtab (struct objfile *objfile, struct partial_symtab *psymtab,
     }
   if (psymtab->n_global_syms > 0)
     {
-      print_partial_symbols (gdbarch,
+      print_partial_symbols (gdbarch, objfile,
 			     &objfile->global_psymbols[psymtab->globals_offset],
 			     psymtab->n_global_syms, "Global", outfile);
     }
   if (psymtab->n_static_syms > 0)
     {
-      print_partial_symbols (gdbarch,
+      print_partial_symbols (gdbarch, objfile,
 			     &objfile->static_psymbols[psymtab->statics_offset],
 			     psymtab->n_static_syms, "Static", outfile);
     }
@@ -1479,7 +1485,7 @@  psym_fill_psymbol_map (struct objfile *objfile,
 
       if (PSYMBOL_CLASS (psym) == LOC_STATIC)
 	{
-	  CORE_ADDR addr = PSYMBOL_VALUE_ADDRESS (psym);
+	  CORE_ADDR addr = PSYMBOL_VALUE_RAW_ADDRESS (psym);
 	  if (seen_addrs->find (addr) == seen_addrs->end ())
 	    {
 	      seen_addrs->insert (addr);
@@ -1726,7 +1732,7 @@  add_psymbol_to_bcache (const char *name, int namelength, int copy_name,
      holes.  */
   memset (&psymbol, 0, sizeof (psymbol));
 
-  PSYMBOL_VALUE_ADDRESS (&psymbol) = coreaddr;
+  SET_PSYMBOL_VALUE_ADDRESS (&psymbol, coreaddr);
   PSYMBOL_SECTION (&psymbol) = -1;
   PSYMBOL_SET_LANGUAGE (&psymbol, language, &objfile->objfile_obstack);
   PSYMBOL_DOMAIN (&psymbol) = domain;