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

Pedro Alves
2012-04-17T12:16:21Z

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