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

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

[Ulrich Weigand]

Pedro Alves wrote:

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

Ah, thanks for pointing that out!

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

Hmm, I guess there is the possibility that even though there is
a msymbol at ADDR with name NAME, both 
  lookup_minimal_symbol_by_pc (ADDR)
and
  lookup_minimal_symbol (NAME)

might fail to find it ...   E.g. if there are minimal symbols
  ADDR' NAME
  ADDR  NAME'
  ADDR  NAME
lookup by pc might find NAME', but lookup by name might find ADDR'.

Maybe we need a lookup_minimal_symbol_by_pc_name or so?

> +  /* 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;

Huh.  If that's true, why does fixup_symbol_section even have an
OBJFILE argument?  Is there ever a situation where we cannot use
sym->symtab->objfile?

> +  switch (SYMBOL_CLASS (sym))
> +    {
> +    case LOC_UNRESOLVED:
> +      addr = ~(CORE_ADDR) 0;
> +      break;

Why do we need to fixup the section for an LOC_UNRESOLVED symbol?

I understand that every time we want to use the address of a
LOC_UNRESOLVED, the user needs to look up the msymbol anyway.
They should then use the section from the msymbol too, right?


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

Any reason for not supporting LOC_LABEL or LOC_INDIRECT for psymbols?

(Well, except from the fact that apparently none of the symbol readers
left in GDB will ever generate LOC_INDIRECT ...  But at least mdebugread.c
will generate LOC_LABEL psymbols, it seems.)

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Pedro Alves]

A Tuesday 13 May 2008 22:12:20, Ulrich Weigand wrote:
> Pedro Alves wrote:
> > +  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);
>
> Hmm, I guess there is the possibility that even though there is
> a msymbol at ADDR with name NAME, both
>   lookup_minimal_symbol_by_pc (ADDR)
> and
>   lookup_minimal_symbol (NAME)
>
> might fail to find it ...   E.g. if there are minimal symbols
>   ADDR' NAME
>   ADDR  NAME'
>   ADDR  NAME
> lookup by pc might find NAME', but lookup by name might find ADDR'.
>
> Maybe we need a lookup_minimal_symbol_by_pc_name or so?
>

Ah, good point.  Looks like that would be the best practical
approach, yes.

> > +  /* 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;
>
> Huh.  If that's true, why does fixup_symbol_section even have an
> OBJFILE argument?  Is there ever a situation where we cannot use
> sym->symtab->objfile?
>

Yes, while still reading the debug info, you can get here with
a sym->symtab == NULL, but you'll have a valid objfile to pass
down.  The symtab is then set at the end of end_symtab.  E.g.:

(gdb) p sym->symtab
$2 = (struct symtab *) 0x0

(gdb) bt
#0  fixup_symbol_section (sym=0xb583e0, objfile=0xb336d0) 
at ../../src/gdb/symtab.c:1090
#1  0x0000000000552a1a in var_decode_location (attr=0xb51380, sym=0xb583e0, 
cu=0xb4c7a0)
    at ../../src/gdb/dwarf2read.c:7326
#2  0x00000000005530ce in new_symbol (die=0xb512f0, type=0x0, cu=0xb4c7a0) 
at ../../src/gdb/dwarf2read.c:7462
#3  0x00000000005491a7 in process_die (die=0xb512f0, cu=0xb4c7a0) 
at ../../src/gdb/dwarf2read.c:2777
#4  0x00000000005495a9 in read_file_scope (die=0xb4a630, cu=0xb4c7a0) 
at ../../src/gdb/dwarf2read.c:2895
#5  0x00000000005490c4 in process_die (die=0xb4a630, cu=0xb4c7a0) 
at ../../src/gdb/dwarf2read.c:2713
#6  0x0000000000548fcd in process_full_comp_unit (per_cu=0xb418d0) 
at ../../src/gdb/dwarf2read.c:2680
#7  0x0000000000548a3a in process_queue (objfile=0xb336d0) 
at ../../src/gdb/dwarf2read.c:2476
#8  0x0000000000548c56 in psymtab_to_symtab_1 (pst=0xb41cd0) 
at ../../src/gdb/dwarf2read.c:2556
...

That was reading the the symbol "main" while stopping at "main".

You can also get here with a NULL objfile, for example, by 
means of lookup_symbol_aux_block, when you pass
a NULL symtab output parameter around, fixup_symbol_section
is always called with a NULL objfile, but it's OK, since at
the time lookup_symbol* are called, the symbols should already
have a sym->symtab and a sym->symtab->objfile.

(I actually have no idea why that output *symtab arg is needed
in the lookup_symbol* functions, if a symbol has a symtab
pointer.  Legacy?)

> > +  switch (SYMBOL_CLASS (sym))
> > +    {
> > +    case LOC_UNRESOLVED:
> > +      addr = ~(CORE_ADDR) 0;
> > +      break;
>
> Why do we need to fixup the section for an LOC_UNRESOLVED symbol?
>
> I understand that every time we want to use the address of a
> LOC_UNRESOLVED, the user needs to look up the msymbol anyway.
> They should then use the section from the msymbol too, right?
>

Ah, you're right, from read_var_value:

    case LOC_UNRESOLVED:
      {
	struct minimal_symbol *msym;

	msym = lookup_minimal_symbol (DEPRECATED_SYMBOL_NAME (var), NULL, NULL);
	if (msym == NULL)
	  return 0;
	if (overlay_debugging)
	  addr = symbol_overlayed_address (SYMBOL_VALUE_ADDRESS (msym),
					   SYMBOL_BFD_SECTION (msym));
	else
	  addr = SYMBOL_VALUE_ADDRESS (msym);
      }
      break;


> > +  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;
> > +    }
>
> Any reason for not supporting LOC_LABEL or LOC_INDIRECT for psymbols?
>
> (Well, except from the fact that apparently none of the symbol readers
> left in GDB will ever generate LOC_INDIRECT ...  But at least mdebugread.c
> will generate LOC_LABEL psymbols, it seems.)
>

But that comes from debug info.  I didn't think LOC_LABEL's would ever
be listed in the minsyms.  Can they?  There's certainly no harm in
adding it to the switch, though.
As for LOC_INDIRECT, I remember finding out no reader records it, and
meaning to garbage collect it instead, but defered that to
submission time.  :-/   If we want to keep it, it looks like, yes,
we should be fixing up its section too.

-- 
Pedro Alves

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

[Ulrich Weigand]

Pedro Alves wrote:

> Yes, while still reading the debug info, you can get here with
> a sym->symtab == NULL, but you'll have a valid objfile to pass
> down.  The symtab is then set at the end of end_symtab.  E.g.:

I see, the var_decode_location case is indeed special.

> (I actually have no idea why that output *symtab arg is needed
> in the lookup_symbol* functions, if a symbol has a symtab
> pointer.  Legacy?)

This seems quite broken, in particular because of this code in
lookup_symbol_in_language:

  /* Override the returned symtab with the symbol's specific one.  */
  if (returnval != NULL && symtab != NULL)
    *symtab = SYMBOL_SYMTAB (returnval);

All the dozen functions below lookup_symbol_in_language in the
call tree pass around a symtab output pointer and try to fill
in a value -- but everything is completely ignored anyway ...

This is certainly an opportunity for some cleanup.

> > Any reason for not supporting LOC_LABEL or LOC_INDIRECT for psymbols?
> >
> > (Well, except from the fact that apparently none of the symbol readers
> > left in GDB will ever generate LOC_INDIRECT ...  But at least mdebugread.c
> > will generate LOC_LABEL psymbols, it seems.)
> >
> 
> But that comes from debug info.  I didn't think LOC_LABEL's would ever
> be listed in the minsyms.  Can they?  There's certainly no harm in
> adding it to the switch, though.

I guess labels could still be in the symbol table, depending on how you
link -- in any case, it certainly cannot hurt to add the case to the
switch here (also to be consistent between fixup_psymbol_section and
fixup_symbol_section).

> As for LOC_INDIRECT, I remember finding out no reader records it, and
> meaning to garbage collect it instead, but defered that to
> submission time.  :-/   If we want to keep it, it looks like, yes,
> we should be fixing up its section too.

I've left it in for now; this can be cleaned up in another patch.


I've now merged your patch with mine and added the above changes.
Would it be OK with you if I commit that patch?

Tested on spu-elf (fixes all overlays.exp FAILs), powerpc-linux, and
powerpc64-linux with no regressions.

Bye,
Ulrich


gdb/

2008-05-15  Pedro Alves  <pedro@codesourcery.com>
	    Ulrich Weigand  <uweigand@de.ibm.com>

	* minsyms.c (lookup_minimal_symbol_by_pc_name): New function.
	* symtab.h (lookup_minimal_symbol_by_pc_name): Add prototype.

	* symtab.c (fixup_section): Remove prototype.  Add ADDR parameter;
	use it instead of ginfo->value.address.  Look up minimal symbol by
	address and name.  Assume OBJFILE is non-NULL.
	(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.

	(find_pc_sect_psymtab): Fall back to non-addrmap case when debugging
	overlays and the addrmap returned the wrong section.

	* dwarf2read.c (var_decode_location): Set SYMBOL_CLASS before
	calling fixup_symbol_section.


gdb/testsuite/

2008-05-15  Pedro Alves  <pedro@codesourcery.com>

	* gdb.base/fixsection.exp: New file.
	* gdb.base/fixsection0.c: New file.
	* gdb.base/fixsection1.c: New file.


diff -urNp gdb-orig/gdb/dwarf2read.c gdb-head/gdb/dwarf2read.c
--- gdb-orig/gdb/dwarf2read.c	2008-05-11 23:41:46.000000000 +0200
+++ gdb-head/gdb/dwarf2read.c	2008-05-15 17:13:27.000000000 +0200
@@ -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;
     }
 
diff -urNp gdb-orig/gdb/minsyms.c gdb-head/gdb/minsyms.c
--- gdb-orig/gdb/minsyms.c	2008-05-14 20:26:07.000000000 +0200
+++ gdb-head/gdb/minsyms.c	2008-05-15 18:05:10.448995498 +0200
@@ -331,6 +331,41 @@ lookup_minimal_symbol_text (const char *
 }
 
 /* Look through all the current minimal symbol tables and find the
+   first minimal symbol that matches NAME and PC.  If OBJF is non-NULL,
+   limit the search to that objfile.  Returns a pointer to the minimal
+   symbol that matches, or NULL if no match is found.  */
+
+struct minimal_symbol *
+lookup_minimal_symbol_by_pc_name (CORE_ADDR pc, const char *name,
+				  struct objfile *objf)
+{
+  struct objfile *objfile;
+  struct minimal_symbol *msymbol;
+
+  unsigned int hash = msymbol_hash (name) % MINIMAL_SYMBOL_HASH_SIZE;
+
+  for (objfile = object_files;
+       objfile != NULL;
+       objfile = objfile->next)
+    {
+      if (objf == NULL || objf == objfile
+	  || objf->separate_debug_objfile == objfile)
+	{
+	  for (msymbol = objfile->msymbol_hash[hash];
+	       msymbol != NULL;
+	       msymbol = msymbol->hash_next)
+	    {
+	      if (SYMBOL_VALUE_ADDRESS (msymbol) == pc
+		  && strcmp (SYMBOL_LINKAGE_NAME (msymbol), name) == 0)
+		return msymbol;
+	    }
+	}
+    }
+
+  return NULL;
+}
+
+/* Look through all the current minimal symbol tables and find the
    first minimal symbol that matches NAME and is a solib trampoline.
    If OBJF is non-NULL, limit the search to that objfile.  Returns a
    pointer to the minimal symbol that matches, or NULL if no match is
diff -urNp gdb-orig/gdb/symtab.c gdb-head/gdb/symtab.c
--- gdb-orig/gdb/symtab.c	2008-05-14 15:57:46.000000000 +0200
+++ gdb-head/gdb/symtab.c	2008-05-15 18:02:17.398508575 +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,23 +1025,23 @@ 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);
 
   /* First, check whether a minimal symbol with the same name exists
      and points to the same address.  The address check is required
      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)
+  msym = lookup_minimal_symbol_by_pc_name (addr, ginfo->name, objfile);
+  if (msym)
     {
       ginfo->bfd_section = SYMBOL_BFD_SECTION (msym);
       ginfo->section = SYMBOL_SECTION (msym);
     }
-  else if (objfile)
+  else
     {
       /* Static, function-local variables do appear in the linker
 	 (minimal) symbols, but are frequently given names that won't
@@ -1064,11 +1079,7 @@ 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;
@@ -1087,13 +1098,42 @@ 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 || SYMBOL_SYMTAB (sym));
+
+  if (objfile == NULL)
+    objfile = SYMBOL_SYMTAB (sym)->objfile;
+
+  /* We should have an objfile by now.  */
+  gdb_assert (objfile);
+
+  switch (SYMBOL_CLASS (sym))
+    {
+    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,31 @@ 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_LABEL:
+    case LOC_INDIRECT:
+    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;
 }
diff -urNp gdb-orig/gdb/symtab.h gdb-head/gdb/symtab.h
--- gdb-orig/gdb/symtab.h	2008-05-11 23:41:56.000000000 +0200
+++ gdb-head/gdb/symtab.h	2008-05-15 17:52:33.454647964 +0200
@@ -1183,6 +1183,9 @@ struct minimal_symbol *lookup_minimal_sy
 							       struct objfile
 							       *);
 
+extern struct minimal_symbol *lookup_minimal_symbol_by_pc_name
+				(CORE_ADDR, const char *, struct objfile *);
+
 extern struct minimal_symbol *lookup_minimal_symbol_by_pc (CORE_ADDR);
 
 extern struct minimal_symbol *lookup_minimal_symbol_by_pc_section (CORE_ADDR,
diff -urNp gdb-orig/gdb/testsuite/gdb.base/fixsection.c gdb-head/gdb/testsuite/gdb.base/fixsection.c
--- gdb-orig/gdb/testsuite/gdb.base/fixsection.c	1970-01-01 01:00:00.000000000 +0100
+++ gdb-head/gdb/testsuite/gdb.base/fixsection.c	2008-05-15 17:13:27.000000000 +0200
@@ -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;
+}
diff -urNp gdb-orig/gdb/testsuite/gdb.base/fixsection.exp gdb-head/gdb/testsuite/gdb.base/fixsection.exp
--- gdb-orig/gdb/testsuite/gdb.base/fixsection.exp	1970-01-01 01:00:00.000000000 +0100
+++ gdb-head/gdb/testsuite/gdb.base/fixsection.exp	2008-05-15 17:13:27.000000000 +0200
@@ -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
diff -urNp gdb-orig/gdb/testsuite/gdb.base/fixsectshr.c gdb-head/gdb/testsuite/gdb.base/fixsectshr.c
--- gdb-orig/gdb/testsuite/gdb.base/fixsectshr.c	1970-01-01 01:00:00.000000000 +0100
+++ gdb-head/gdb/testsuite/gdb.base/fixsectshr.c	2008-05-15 17:13:27.000000000 +0200
@@ -0,0 +1,10 @@
+#include <stdio.h>
+#include <stdlib.h>
+
+static FILE *static_fun = NULL;
+
+FILE *
+force_static_fun (void)
+{
+  return static_fun;
+}


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Pedro Alves]

A Thursday 15 May 2008 17:50:29, Ulrich Weigand wrote:

> I've now merged your patch with mine and added the above changes.
> Would it be OK with you if I commit that patch?

Certainly OK with me.  Thanks a lot!

-- 
Pedro Alves

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

[Ulrich Weigand]

Pedro Alves wrote:
> A Thursday 15 May 2008 17:50:29, Ulrich Weigand wrote:
> 
> > I've now merged your patch with mine and added the above changes.
> > Would it be OK with you if I commit that patch?
> 
> Certainly OK with me.  Thanks a lot!

OK, I've checked this in now.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> Yes, I don't like that part either.  I wonder if the memory usage
> would be too bad if we kept an addrmap for each section and one
> combined one for the non-overlay case?

Hmm, maybe this would even save memory; a per-section addrmap would
not need to store any result except from the boolean "address in
section or not" flag ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Michael Snyder]

On Thu, 2008-05-15 at 12:11 +0200, Jan Kratochvil wrote:
> On Tue, 13 May 2008 19:00:48 +0200, Ulrich Weigand wrote:
> ...
> > Which parts of GDB do you think do not support overlays?  I'd be interested
> > in fixing such problems ...
> 
> There is still a lot of stub functions X() just calling X_sect() using
> find_pc_mapped_section(), these should get removed as otherwise one may find
> a countercase where it fails for the overlayed sections.
> [blockvector_for_pc, block_for_pc, find_pc_psymtab, find_pc_psymbol,
>  find_pc_symtab, find_pc_section, find_pc_function]

?  It's been 10 years, but I think that's the way it is
supposed to work.

Old code calls X(), without supplying a section.
Then X forwards the call to X_sect() after attempting
to lookup the appropriate section.

In most cases this should not be critical for overlays, 
because code that depends on overlays should be calling
X_sect() directly.  It was just a shortcut so that we
wouldn't have to change all the calls of X into X_sect
(and do the section lookup in many places instead of one).

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

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> Yes, I don't like that part either.  I wonder if the memory usage
> would be too bad if we kept an addrmap for each section and one
> combined one for the non-overlay case?
> 
> I don't know what overlay debug info looks like.  Can we detect
> overlays in any practical way when reading in symbols?

Not really, I don't think there is any indication in the debug
info that we have an overlay.  The address ranges just happen
to overlap ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Daniel Jacobowitz]

On Thu, May 15, 2008 at 06:38:48PM +0200, Ulrich Weigand wrote:
> > > 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.
> > 
> > I started coding a similiar patch as IMO the overlayed sections have no use for
> > addrmap as they are not discontiguous, thanks for fixing it up this way.
> 
> Hmm, OK.  However, even with overlay debugging, there might be some other
> discontiguous sections, so I don't really like the
>   if (overlay_debugging ...)
> aspect of my patch.  But without that condition, one of your new test cases
> would fail again.

Yes, I don't like that part either.  I wonder if the memory usage
would be too bad if we kept an addrmap for each section and one
combined one for the non-overlay case?

I don't know what overlay debug info looks like.  Can we detect
overlays in any practical way when reading in symbols?

-- 
Daniel Jacobowitz
CodeSourcery

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

[Ulrich Weigand]

Jan Kratochvil wrote:
> On Tue, 13 May 2008 19:00:48 +0200, Ulrich Weigand wrote:
> ...
> > Which parts of GDB do you think do not support overlays?  I'd be interested
> > in fixing such problems ...
> 
> There is still a lot of stub functions X() just calling X_sect() using
> find_pc_mapped_section(), these should get removed as otherwise one may find
> a countercase where it fails for the overlayed sections.
> [blockvector_for_pc, block_for_pc, find_pc_psymtab, find_pc_psymbol,
>  find_pc_symtab, find_pc_section, find_pc_function]

I thought those remaining usages were generally OK -- but I guess you're
right that new uses may have crept in over time ...

> > 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.
> 
> I started coding a similiar patch as IMO the overlayed sections have no use for
> addrmap as they are not discontiguous, thanks for fixing it up this way.

Hmm, OK.  However, even with overlay debugging, there might be some other
discontiguous sections, so I don't really like the
  if (overlay_debugging ...)
aspect of my patch.  But without that condition, one of your new test cases
would fail again.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Jan Kratochvil]

On Tue, 13 May 2008 19:00:48 +0200, Ulrich Weigand wrote:
...
> Which parts of GDB do you think do not support overlays?  I'd be interested
> in fixing such problems ...

There is still a lot of stub functions X() just calling X_sect() using
find_pc_mapped_section(), these should get removed as otherwise one may find
a countercase where it fails for the overlayed sections.
[blockvector_for_pc, block_for_pc, find_pc_psymtab, find_pc_psymbol,
 find_pc_symtab, find_pc_section, find_pc_function]


> 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.

I started coding a similiar patch as IMO the overlayed sections have no use for
addrmap as they are not discontiguous, thanks for fixing it up this way.

...
> 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.

It is not worth it as it would mean to either
* support multiple results from a single call of addrmap_find()
or
* to move `addrmap_find' from `struct objfile' to each `struct partial_symtab'
which is IMO not worth the performance in a general case as your patch only
stays at the same performance as before addrmap for the overlayed case.



Regards,
Jan

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

[Michael Snyder]

On Tue, 2008-05-13 at 19:20 +0100, Pedro Alves wrote:

> 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.

Try building the m32r target and testing with the simulator.

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

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 7363 bytes --]

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

[-- Attachment #2: fixup_sections.diff --]
[-- Type: text/x-diff, Size: 8895 bytes --]

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

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

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 7363 bytes --]

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

[-- Attachment #2: fixup_sections.diff --]
[-- Type: text/x-diff, Size: 8895 bytes --]

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

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

[Ulrich Weigand]

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.


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.

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;
 }

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Michael Snyder]

On Mon, 2008-05-12 at 16:39 -0400, Daniel Jacobowitz wrote:
> On Mon, May 12, 2008 at 01:06:44PM -0700, Michael Snyder wrote:
> > I wrote that overlay code over 10 years ago.
> > I wonder whether anyone still uses it?
> 
> On Mon, May 12, 2008 at 10:32:32PM +0200, 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 you may be able to guess from context, the answer is SPU :-)
> 
> Each SPE has only a 256K local store which it uses for code, stack,
> and heap.  That's not very much space, so overlays are frequently
> required.

Oho, well, Stan Shebs should be gratified.  He assured me
at the time that overlays would get more use than I thought
they would.  ;-)

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

[Doug Evans]

On Mon, May 12, 2008 at 1:39 PM, Daniel Jacobowitz <drow@false.org> wrote:
> On Mon, May 12, 2008 at 01:06:44PM -0700, Michael Snyder wrote:
>  > I wrote that overlay code over 10 years ago.
>  > I wonder whether anyone still uses it?
>
>
> On Mon, May 12, 2008 at 10:32:32PM +0200, 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 you may be able to guess from context, the answer is SPU :-)

Ah hah! :-)

>  Each SPE has only a 256K local store which it uses for code, stack,
>  and heap.  That's not very much space, so overlays are frequently
>  required.

Yep.

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

[Doug Evans]

On Mon, May 12, 2008 at 1:06 PM, Michael Snyder <msnyder@specifix.com> wrote:
>  I wrote that overlay code over 10 years ago.
>  I wonder whether anyone still uses it?

I've been wondering if it'd be useful in the spu context.  Is what how
you discovered the breakage Ulrich?

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

[Jan Kratochvil]

On Mon, 12 May 2008 22:39:34 +0200, Daniel Jacobowitz wrote:
...
> As you may be able to guess from context, the answer is SPU :-)

OK.  Going to provide some patch to be somehow sections-aware.


Regards,
Jan

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

[Daniel Jacobowitz]

On Mon, May 12, 2008 at 01:06:44PM -0700, Michael Snyder wrote:
> I wrote that overlay code over 10 years ago.
> I wonder whether anyone still uses it?

On Mon, May 12, 2008 at 10:32:32PM +0200, 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 you may be able to guess from context, the answer is SPU :-)

Each SPE has only a 256K local store which it uses for code, stack,
and heap.  That's not very much space, so overlays are frequently
required.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Jan Kratochvil]

On Mon, 12 May 2008 21:45:39 +0200, Ulrich Weigand wrote:
...
> It appears this change broke overlay support.

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?


Sorry,
Jan

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

[Michael Snyder]

On Mon, 2008-05-12 at 21:45 +0200, Ulrich Weigand wrote:
> Jan Kratochvil wrote:
> 
> > 	* symtab.c: Include "addrmap.h"
> > 	(find_pc_sect_psymtab): Support reading the field PSYMTABS_ADDRMAP.
> > 	Move the psymtab locator into ...
> > 	(find_pc_sect_psymtab_closer): ... a new function.
> 
> It appears this change broke overlay support.  

Thanks for noticing.   ;-)

I wrote that overlay code over 10 years ago.
I wonder whether anyone still uses it?

Michael

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

[Ulrich Weigand]

Jan Kratochvil wrote:

> 	* symtab.c: Include "addrmap.h"
> 	(find_pc_sect_psymtab): Support reading the field PSYMTABS_ADDRMAP.
> 	Move the psymtab locator into ...
> 	(find_pc_sect_psymtab_closer): ... a new function.

It appears this change broke overlay support.  In fact, it looks like the
main idea of this patch is fundamentally incompatible with overlays:

> +    /* Map addresses to the entries of PSYMTABS.  It would be more efficient to
> +       have a map per the whole process but ADDRMAP cannot selectively remove
> +       its items during FREE_OBJFILE.  This mapping is already present even for
> +       PARTIAL_SYMTABs which still have no corresponding full SYMTABs read.  */
> +
> +    struct addrmap *psymtabs_addrmap;

In the presence of overlays, a single address can map to *multiple* PSYMTABS
corresponding to multiple overlay segments optionally loaded at that address.

The old code chose between them using the "section" argument to 
find_pc_sect_psymtab; it seems the new code just ignores this argument:

  /* Try just the PSYMTABS_ADDRMAP mapping first as it has better granularity
     than the later used TEXTLOW/TEXTHIGH one.  */

  ALL_OBJFILES (objfile)
    if (objfile->psymtabs_addrmap != NULL)
      {
        struct partial_symtab *pst;

        pst = addrmap_find (objfile->psymtabs_addrmap, pc);
        if (pst != NULL)
          {
            /* 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
               a worse chosen section due to the TEXTLOW/TEXTHIGH ranges
               overlap.  */

            return pst;
          }
      }


Any suggestions how to fix this?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com