Re: RFC: bfd_section should not be NULL in call to prim_record_minimal_*

[Pedro Alves <palves@redhat.com>]

On 04/16/2012 10:13 PM, Joel Brobecker wrote:

> It seems that creating breakpoints no longer works on ppc-aix:
> 
>     % gdb foo
>     (gdb) b main
>     /[...]/progspace.c:216: internal-error: set_current_program_space: Assertion `pspace != NULL' failed.
>     A problem internal to GDB has been detected,
>     further debugging may prove unreliable.
>     Quit this debugging session? (y or n)
> 
> Patch #2 explains what is going on, but basically, xcoffread.c
> creates minimal symbols where the obj_section is not set. At first
> sight, minsyms.h seems to indicate that it is OK, if you look at
> prim_record_minimal_symbol_full's documentation:
> 
>    BFD_SECTION - the symbol's BFD section; used to find the
>    appropriate obj_section for the minimal symbol.  This can be NULL.
>                                                     ^^^^^^^^^^^^^^^^
> 
> But I think it is wrong, because I think a lot of places in the GDB code
> make the assumption that a minimal symbol's obj_section is not NULL, and
> the only way to set it, I think, is to have the BFD section.


When I added the pspace stuff, I remember trying to be careful to not follow
SYMBOL_OBJ_SECTION (msymbol)->objfile to get at objfile->pspace.

[In my mind, symbols ideally wouldn't need back pointers to thei
"containers" (symtabs, object files, etc), because that wastes
space; the lookup routines instead should bubble up the "container"
the symbol was found in if necessary.  But that may be very
hard to do.]

At first sight, I can't find any other place that does that; this was
added recently with the linespec rewrite only, it seems.

The "section" and "obj_section" are one of those sources of difference
and confusion that it'd be nice to get rid of:

  /* Which section is this symbol in?  This is an index into
     section_offsets for this objfile.  Negative means that the symbol
     does not get relocated relative to a section.
     Disclaimer: currently this is just used for xcoff, so don't
     expect all symbol-reading code to set it correctly (the ELF code
     also tries to set it correctly).  */

  short section;

  /* The section associated with this symbol.  It can be NULL.  */

  struct obj_section *obj_section;

OTOH, this stuff is space sensitive, and in the long term,
it could prove better to only hold a "short" instead of a
pointer (8 bytes on 64-bit hosts).

Anyway, all that to say that I think the new linespec code should be using
the proper API to get at a msymbol's objfile -- msymbol_objfile.  Like
in the patch below.  When testing it, I ran linespec.exp first, and
that revealed that msymbol_objfile had a bug in that it didn't look
at all the pspaces, only the current (the test adds a second inferior,
therefore a second pspace).  Not good when you want to look up the
objfile because you want to know the msyms' pspace to begin with.

I think we should put this change in anyway, and, if we really
want to assume msymbol->obj_section->objfile will always be possible,
then msymbol_objfile could/should be changed to do that directly link too
instead of the hash based lookup.

> In the meantime, patch #2 fixes the problem by making sure that we
> always pass a BFD section.  I haven't tested it against the official
> testsuite, I will do so now, but I also wanted to start this discussion
> before I forget.


I looked over that patch, and nothing jumped out as wrong to me.

The patch below was tested on x86_64 Fedora 16.  I'm guessing it fixes
AIX too.

2012-04-17  Pedro Alves  <palves@redhat.com>

	Avoid SYMBOL_OBJ_SECTION (elem->minsym)->objfile.

	* linespec.c (convert_linespec_to_sals, compare_msymbols): Use
	msymbol_objfile to get at the msymbol's objfile.
	* minsyms.c (msymbol_objfile): Iterate over program spaces.
---

 gdb/linespec.c |    6 +++---
 gdb/minsyms.c  |   10 ++++++----
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 228214b..cf073ae 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -1899,7 +1899,7 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
 	       VEC_iterate (minsym_and_objfile_d, ls->minimal_symbols, i, elem);
 	       ++i)
 	    {
-	      pspace = SYMBOL_OBJ_SECTION (elem->minsym)->objfile->pspace;
+	      pspace = msymbol_objfile (elem->minsym)->pspace;
 	      set_current_program_space (pspace);
 	      minsym_found (state, elem->objfile, elem->minsym, &sals);
 	    }
@@ -2588,8 +2588,8 @@ compare_msymbols (const void *a, const void *b)
   struct minimal_symbol * const *sb = b;
   uintptr_t uia, uib;

-  uia = (uintptr_t) SYMBOL_OBJ_SECTION (*sa)->objfile->pspace;
-  uib = (uintptr_t) SYMBOL_OBJ_SECTION (*sb)->objfile->pspace;
+  uia = (uintptr_t) msymbol_objfile (*sa)->pspace;
+  uib = (uintptr_t) msymbol_objfile (*sb)->pspace;

   if (uia < uib)
     return -1;
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index d762b2d..dfc2a55 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -144,16 +144,18 @@ add_minsym_to_demangled_hash_table (struct minimal_symbol *sym,
 struct objfile *
 msymbol_objfile (struct minimal_symbol *sym)
 {
+  struct program_space *pspace;
   struct objfile *objf;
   struct minimal_symbol *tsym;

   unsigned int hash
     = msymbol_hash (SYMBOL_LINKAGE_NAME (sym)) % MINIMAL_SYMBOL_HASH_SIZE;

-  for (objf = object_files; objf; objf = objf->next)
-    for (tsym = objf->msymbol_hash[hash]; tsym; tsym = tsym->hash_next)
-      if (tsym == sym)
-	return objf;
+  ALL_PSPACES (pspace)
+    for (objf = pspace->objfiles; objf; objf = objf->next)
+      for (tsym = objf->msymbol_hash[hash]; tsym; tsym = tsym->hash_next)
+	if (tsym == sym)
+	  return objf;

   /* We should always be able to find the objfile ...  */
   internal_error (__FILE__, __LINE__, _("failed internal consistency check"));