Re: Overlay support broken (Re: [patch] [2/2] Discontiguous PSYMTABs (psymtabs->symtabs by addrmap))

[Pedro Alves <pedro at codesourcery dot com>]

A Tuesday 13 May 2008 18:00:48, Ulrich Weigand wrote:
> Jan Kratochvil wrote:
> > I generally considered the overlays to be no longer supported and just
> > still not removed from the code.  While reading other GDB code I was
> > feeling overlays are no longer supported on multiple places but sure I
> > may be wrong.
> >
> > Could you please name which current arch is dependent on overlaying?
>
> As Daniel mentioned, I noticed this on spu-elf, where we do make use of
> overlays (at least code overlays -- data overlays are not currently used).
>
> Which parts of GDB do you think do not support overlays?  I'd be interested
> in fixing such problems ...
>
> For now, I'm using the patch below that simply falls back to the
> non-addrmap case when debugging overlays and the addrmap returned the wrong
> section.
>
>
> In testing this, I noticed an independent overlay regression introduced by
> a patch of mine:
> http://sourceware.org/ml/gdb-patches/2008-05/msg00120.html
> which added code to fixup_section to verify the msymbol address.
>
> This patch exposed a pre-existing bug in fixup_section: it uses
>   ginfo->value.address
> to access the address of a symbol or psymbol.  This is mostly correct,
> but in one case it is not: when handling a LOC_BLOCK symbol, in which
> case ginfo->value.address is undefined, and the start address needs to
> be retrieved from the block at ginfo->value.block.
>
> The following patch fixes this regression as well by having the callers
> of fixup_section pass in the proper address.
>

FWIW, we have been using something very similar in our trees that I
had never submitted because of not being able to test on an arch
that uses overlays.

>
> Tested on spu-elf, powerpc-linux and powerpc64-linux with no regressions;
> fixes overlays.exp on spu-elf.
>
> If there is no objection, I'd like to commit this to fix the present
> regression until we have a proper implementation of addrmaps for the
> overlay case.
>

Please also make sure that you're setting SYMBOL_CLASS
before calling fixup_*symbol_section everywhere.  This will break
dwarf2read.c, for example:

 if (attr_form_is_block (attr)
      && DW_BLOCK (attr)->size == 1 + cu_header->addr_size
      && DW_BLOCK (attr)->data[0] == DW_OP_addr)
    {
      unsigned int dummy;

      SYMBOL_VALUE_ADDRESS (sym) =
	read_address (objfile->obfd, DW_BLOCK (attr)->data + 1, cu, &dummy);
      fixup_symbol_section (sym, objfile);
      SYMBOL_VALUE_ADDRESS (sym) += ANOFFSET (objfile->section_offsets,
					      SYMBOL_SECTION (sym));
      SYMBOL_CLASS (sym) = LOC_STATIC;
      return;
    }

The breakage goes unnoticed on linux, as all loadable sections will
have the same ANOFFSET, but will break in systems loading .text and
.data with different offsets, as uclinux, for example.

Attached is what we're using, as an example.  The testcase no longer
fails on head, I suspect because of your other patch that checks
the address.  The fix was for GDB fixing up the wrong section, due
to the lookup by name finding the wrong minsym.

> Bye,
> Ulrich
>
>
> ChangeLog:
>
> 	* symtab.c (fixup_section): Remove prototype.  Add ADDR parameter;
> 	use it instead of ginfo->value.address.
> 	(fixup_symbol_section): Pass in correct symbol address.
> 	(fixup_psymbol_section): Pass in correct symbol address.
>
> 	(find_pc_sect_psymtab): Fall back to non-addrmap case when debugging
> 	overlays and the addrmap returned the wrong section.
>
>
> diff -urNp gdb-orig/gdb/symtab.c gdb-head/gdb/symtab.c
> --- gdb-orig/gdb/symtab.c	2008-05-11 23:41:56.000000000 +0200
> +++ gdb-head/gdb/symtab.c	2008-05-13 15:48:55.626975367 +0200
> @@ -110,8 +110,6 @@ struct symbol *lookup_symbol_aux_psymtab
>  					   const domain_enum domain,
>  					   struct symtab **symtab);
>
> -static void fixup_section (struct general_symbol_info *, struct objfile
> *); -
>  static int file_matches (char *, char **, int);
>
>  static void print_symbol_info (domain_enum,
> @@ -878,6 +876,23 @@ find_pc_sect_psymtab (CORE_ADDR pc, asec
>  	pst = addrmap_find (objfile->psymtabs_addrmap, pc);
>  	if (pst != NULL)
>  	  {
> +	    /* FIXME: addrmaps currently do not handle overlayed sections,
> +	       so fall back to the non-addrmap case if we're debugging
> +	       overlays and the addrmap returned the wrong section.  */
> +	    if (overlay_debugging && msymbol && section)
> +	      {
> +		struct partial_symbol *p;
> +		/* NOTE: This assumes that every psymbol has a
> +		   corresponding msymbol, which is not necessarily
> +		   true; the debug info might be much richer than the
> +		   object's symbol table.  */
> +		p = find_pc_sect_psymbol (pst, pc, section);
> +		if (!p
> +		    || SYMBOL_VALUE_ADDRESS (p)
> +		       != SYMBOL_VALUE_ADDRESS (msymbol))
> +		  continue;
> +	      }
> +
>  	    /* We do not try to call FIND_PC_SECT_PSYMTAB_CLOSER as
>  	       PSYMTABS_ADDRMAP we used has already the best 1-byte
>  	       granularity and FIND_PC_SECT_PSYMTAB_CLOSER may mislead us into
> @@ -1010,7 +1025,8 @@ find_pc_psymbol (struct partial_symtab *
>     out of the minimal symbols and stash that in the debug symbol.  */
>
>  static void
> -fixup_section (struct general_symbol_info *ginfo, struct objfile *objfile)
> +fixup_section (struct general_symbol_info *ginfo, CORE_ADDR addr,
> +	       struct objfile *objfile)
>  {
>    struct minimal_symbol *msym;
>    msym = lookup_minimal_symbol (ginfo->name, NULL, objfile);
> @@ -1020,8 +1036,7 @@ fixup_section (struct general_symbol_inf
>       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.  */
> -  if (msym
> -      && SYMBOL_VALUE_ADDRESS (msym) == ginfo->value.address)
> +  if (msym && SYMBOL_VALUE_ADDRESS (msym) == addr)
>      {
>        ginfo->bfd_section = SYMBOL_BFD_SECTION (msym);
>        ginfo->section = SYMBOL_SECTION (msym);
> @@ -1064,11 +1079,8 @@ fixup_section (struct general_symbol_inf
>  	 this reason, we still attempt a lookup by name prior to doing
>  	 a search of the section table.  */
>
> -      CORE_ADDR addr;
>        struct obj_section *s;
>
> -      addr = ginfo->value.address;
> -
>        ALL_OBJFILE_OSECTIONS (objfile, s)
>  	{
>  	  int idx = s->the_bfd_section->index;
> @@ -1093,7 +1105,22 @@ fixup_symbol_section (struct symbol *sym
>    if (SYMBOL_BFD_SECTION (sym))
>      return sym;
>
> -  fixup_section (&sym->ginfo, objfile);
> +  switch (SYMBOL_CLASS (sym))
> +    {
> +    case LOC_LABEL:
> +    case LOC_STATIC:
> +    case LOC_INDIRECT:
> +      fixup_section (&sym->ginfo, SYMBOL_VALUE_ADDRESS (sym), objfile);
> +      break;
> +
> +    case LOC_BLOCK:
> +      fixup_section (&sym->ginfo,
> +		     BLOCK_START (SYMBOL_BLOCK_VALUE (sym)), objfile);
> +      break;
> +
> +    default:
> +      break;
> +    }
>
>    return sym;
>  }
> @@ -1107,7 +1134,18 @@ fixup_psymbol_section (struct partial_sy
>    if (SYMBOL_BFD_SECTION (psym))
>      return psym;
>
> -  fixup_section (&psym->ginfo, objfile);
> +  switch (SYMBOL_CLASS (psym))
> +    {
> +    case LOC_LABEL:
> +    case LOC_STATIC:
> +    case LOC_INDIRECT:
> +    case LOC_BLOCK:
> +      fixup_section (&psym->ginfo, SYMBOL_VALUE_ADDRESS (psym), objfile);
> +      break;
> +
> +    default:
> +      break;
> +    }
>
>    return psym;
>  }



-- 
Pedro Alves

2008-01-23  Pedro Alves  <pedro@codesourcery.com>

	Match symbol/msymbol/bfd_section by address.

	gdb/
	* symtab.c (fixup_section): Add addr parameter.  If possible,
	lookup the minimal symbol by address.  If that fails, fallback to
	the old by-name method.
	(fixup_symbol_section): Ensure we always have an objfile to look
	into.  Extract and pass to fixup_section the symbol's address that
	will match the minimal symbol's address.
	(fixup_psymbol_section): Likewise.
	* dwarf2read.c (var_decode_location): Set SYMBOL_CLASS before
	calling fixup_symbol_section.

	gdb/testsuite/
	* gdb.base/fixsection.exp: New file.
	* gdb.base/fixsection0.c: New file.
	* gdb.base/fixsection1.c: New file.
---
 gdb/dwarf2read.c                      |    2 
 gdb/symtab.c                          |   70 ++++++++++++++++++++++++++++++---
 gdb/testsuite/gdb.base/fixsection.c   |   35 ++++++++++++++++
 gdb/testsuite/gdb.base/fixsection.exp |   71 ++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/fixsectshr.c   |   10 ++++
 5 files changed, 180 insertions(+), 8 deletions(-)

Index: src/gdb/symtab.c
===================================================================
--- src.orig/gdb/symtab.c	2008-05-13 19:04:29.000000000 +0100
+++ src/gdb/symtab.c	2008-05-13 19:14:31.000000000 +0100
@@ -110,8 +110,6 @@ struct symbol *lookup_symbol_aux_psymtab
 					   const domain_enum domain,
 					   struct symtab **symtab);
 
-static void fixup_section (struct general_symbol_info *, struct objfile *);
-
 static int file_matches (char *, char **, int);
 
 static void print_symbol_info (domain_enum,
@@ -1010,10 +1008,20 @@ find_pc_psymbol (struct partial_symtab *
    out of the minimal symbols and stash that in the debug symbol.  */
 
 static void
-fixup_section (struct general_symbol_info *ginfo, struct objfile *objfile)
+fixup_section (struct general_symbol_info *ginfo,
+	       CORE_ADDR addr, struct objfile *objfile)
 {
-  struct minimal_symbol *msym;
-  msym = lookup_minimal_symbol (ginfo->name, NULL, objfile);
+  struct minimal_symbol *msym = NULL;
+  if (addr != ~(CORE_ADDR) 0)
+    /* If we have an address to lookup, use it.  */
+    msym = lookup_minimal_symbol_by_pc (addr);
+
+  if (!msym
+      || addr != SYMBOL_VALUE_ADDRESS (msym)
+      || strcmp (DEPRECATED_SYMBOL_NAME (msym), ginfo->name) != 0)
+    /* Try by looking up by name.  Not perfect, since it can match the
+       wrong symbol.  */
+    msym = lookup_minimal_symbol (ginfo->name, NULL, objfile);
 
   /* First, check whether a minimal symbol with the same name exists
      and points to the same address.  The address check is required
@@ -1087,13 +1095,45 @@ fixup_section (struct general_symbol_inf
 struct symbol *
 fixup_symbol_section (struct symbol *sym, struct objfile *objfile)
 {
+  CORE_ADDR addr;
+
   if (!sym)
     return NULL;
 
   if (SYMBOL_BFD_SECTION (sym))
     return sym;
 
-  fixup_section (&sym->ginfo, objfile);
+  /* 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 && sym->symtab->objfile));
+
+  if (objfile == NULL)
+    objfile = sym->symtab->objfile;
+
+  /* We should have an objfile by now.  */
+  gdb_assert (objfile);
+
+  switch (SYMBOL_CLASS (sym))
+    {
+    case LOC_UNRESOLVED:
+      addr = ~(CORE_ADDR) 0;
+      break;
+    case LOC_STATIC:
+    case LOC_LABEL:
+    case LOC_INDIRECT:
+      addr = SYMBOL_VALUE_ADDRESS (sym);
+      break;
+    case LOC_BLOCK:
+      addr = BLOCK_START (SYMBOL_BLOCK_VALUE (sym));
+      break;
+
+    default:
+      /* Nothing else will be listed in the minsyms -- no use looking
+	 it up.  */
+      return sym;
+    }
+
+  fixup_section (&sym->ginfo, addr, objfile);
 
   return sym;
 }
@@ -1101,13 +1141,29 @@ fixup_symbol_section (struct symbol *sym
 struct partial_symbol *
 fixup_psymbol_section (struct partial_symbol *psym, struct objfile *objfile)
 {
+  CORE_ADDR addr;
+
   if (!psym)
     return NULL;
 
   if (SYMBOL_BFD_SECTION (psym))
     return psym;
 
-  fixup_section (&psym->ginfo, objfile);
+  gdb_assert (objfile);
+
+  switch (SYMBOL_CLASS (psym))
+    {
+    case LOC_STATIC:
+    case LOC_BLOCK:
+      addr = SYMBOL_VALUE_ADDRESS (psym);
+      break;
+    default:
+      /* Nothing else will be listed in the minsyms -- no use looking
+	 it up.  */
+      return psym;
+    }
+
+  fixup_section (&psym->ginfo, addr, objfile);
 
   return psym;
 }
Index: src/gdb/dwarf2read.c
===================================================================
--- src.orig/gdb/dwarf2read.c	2008-05-13 19:04:29.000000000 +0100
+++ src/gdb/dwarf2read.c	2008-05-13 19:14:31.000000000 +0100
@@ -7323,10 +7323,10 @@ var_decode_location (struct attribute *a
 
       SYMBOL_VALUE_ADDRESS (sym) =
 	read_address (objfile->obfd, DW_BLOCK (attr)->data + 1, cu, &dummy);
+      SYMBOL_CLASS (sym) = LOC_STATIC;
       fixup_symbol_section (sym, objfile);
       SYMBOL_VALUE_ADDRESS (sym) += ANOFFSET (objfile->section_offsets,
 					      SYMBOL_SECTION (sym));
-      SYMBOL_CLASS (sym) = LOC_STATIC;
       return;
     }
 
Index: src/gdb/testsuite/gdb.base/fixsectshr.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.base/fixsectshr.c	2008-05-13 19:14:31.000000000 +0100
@@ -0,0 +1,10 @@
+#include <stdio.h>
+#include <stdlib.h>
+
+static FILE *static_fun = NULL;
+
+FILE *
+force_static_fun (void)
+{
+  return static_fun;
+}
Index: src/gdb/testsuite/gdb.base/fixsection.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.base/fixsection.exp	2008-05-13 19:14:31.000000000 +0100
@@ -0,0 +1,71 @@
+# Copyright (C) 2008 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+set prms_id 0
+set bug_id 0
+
+if {[skip_shlib_tests]} {
+    return 0
+}
+
+set testfile "fixsection"
+set srcfile ${srcdir}/${subdir}/${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+set libfile "fixsectshr"
+set libsrc ${srcdir}/${subdir}/${libfile}.c
+set lib_sl ${objdir}/${subdir}/${libfile}.sl
+
+set lib_opts [list debug nowarnings]
+set exec_opts [list debug nowarnings shlib=$lib_sl]
+
+if [get_compiler_info ${binfile}] {
+    return -1
+}
+
+if { [gdb_compile_shlib $libsrc $lib_sl $lib_opts] != ""
+     || [gdb_compile $srcfile $binfile executable $exec_opts] != ""} {
+    untested "Could not compile either $libsrc or $srcfile."
+    return -1
+}
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+gdb_load_shlibs ${lib_sl}
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return 1;
+}
+
+#
+# set breakpoint at static function static_fun
+#
+gdb_test "break static_fun" \
+    "Breakpoint.*at.* file .*${srcfile}, line.*" \
+    "breakpoint at static_fun"
+
+#
+# exit gdb
+#
+gdb_exit
Index: src/gdb/testsuite/gdb.base/fixsection.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.base/fixsection.c	2008-05-13 19:14:31.000000000 +0100
@@ -0,0 +1,35 @@
+/* Copyright (C) 2008 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+   This file was written by Pedro Alves (pedro@codesourcery.com).  */
+
+#include <stdio.h>
+#include <stdlib.h>
+
+extern FILE *force_static_fun (void);
+
+static void *
+static_fun (void *arg)
+{
+    return NULL;
+}
+
+int
+main (int argc, char **argv)
+{
+  static_fun (NULL);
+  force_static_fun ();
+  return 0;
+}