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

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

[Joel Brobecker]

Hello,

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.

So, I think the function documentation should be changed to remove
the permission to pass NULL, and a gdb_assert should also be added
to verify that SYMBOL_OBJ_SECTION (msymbol) != NULL after the
BFD section to obj_section search.

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.

[RFA/commit 1/2] Unused local variables in xcoffread.c:read_xcoff_symtab

[Joel Brobecker]

Some local variables are being set, but never used.  This patch deletes
them.

gdb/ChangeLog:

        * xcoffread.c (read_xcoff_symtab): Delete variables
        last_csect_val and last_csect_sec and associated code.

This patch is actually independent of the series, but it was triggered
by the investigation of the internal-error when creating a breakpoint.
I will commit as soon as testing is complete.

---
 gdb/xcoffread.c |   15 +++------------
 1 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
index 86ae8fb..fcabddb 100644
--- a/gdb/xcoffread.c
+++ b/gdb/xcoffread.c
@@ -986,9 +986,7 @@ read_xcoff_symtab (struct partial_symtab *pst)
 
   char *filestring = " _start_ ";	/* Name of the current file.  */
 
-  const char *last_csect_name;	/* Last seen csect's name and value.  */
-  CORE_ADDR last_csect_val;
-  int last_csect_sec;
+  const char *last_csect_name;	/* Last seen csect's name.  */
 
   this_symtab_psymtab = pst;
 
@@ -998,7 +996,6 @@ read_xcoff_symtab (struct partial_symtab *pst)
 
   last_source_file = NULL;
   last_csect_name = 0;
-  last_csect_val = 0;
 
   start_stabs ();
   start_symtab (filestring, (char *) NULL, file_start_addr);
@@ -1171,14 +1168,8 @@ read_xcoff_symtab (struct partial_symtab *pst)
 						SECT_OFF_TEXT (objfile));
 		      file_end_addr = file_start_addr + CSECT_LEN (&main_aux);
 
-		      if (cs->c_name && (cs->c_name[0] == '.'
-					 || cs->c_name[0] == '@'))
-			{
-			  last_csect_name = cs->c_name;
-			  last_csect_val = cs->c_value;
-			  last_csect_sec = secnum_to_section (cs->c_secnum,
-							      objfile);
-			}
+		      if (cs->c_name && (cs->c_name[0] == '.' || cs->c_name[0] == '@'))
+			last_csect_name = cs->c_name;
 		    }
 		    continue;
 
-- 
1.6.5.rc2

[RFA/commit 2/2] pspace != NULL failed assertion on ppc-aix

[Joel Brobecker]

This patch fixes the following problem:

    % 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)

The problem happens when we try to get the program space from
a minimal symbol.  For that, we go through the minimal symbol's
obj_section->objfile->pspace.  But the minimal symbol's obj_section
is not set, and thus we somehow get a NULL program space. And
the reason why the obj_section is not set is because the XCOFF
reader did not pass the bfd_section when calling minsyms.c's
prim_record_minimal_symbol_and_info.

This patch fixes the problem by making sure that the obj_section
part of all minimal symbols is correctly set.

There are several layers to this patch:

  1. The RECORD_MINIMAL_SYMBOL macro is replaced by a function
     (record_minimal_symbol).  The macro's comment explains that
     it was designed as macro for performance, but we have to make
     two function calls performing two searches, together with
     a call to prim_record_minimal_symbol_and_info. I do not think
     that the extra overhead caused by an additional function call
     is going to have a noticeable impact.

     This allows this function to perform the searches for both
     SECT_OFF_* index and BFD section using one search instead
     of two.  We could have done that with a macro, but the size
     of the macro would have become unreasonable.

  2. The patch introduces a new function "xcoff_secnum_to_sections",
     which performs the search needed in (1) above.  It also
     reimplements secnum_to_section and secnum_to_bfd_section
     on top of that new function.

  3. Adjusts all the calls to RECORD_MINIMAL_SYMBOL and
     prim_record_minimal_symbol_and_info accordingly.

  4. Changes the "last_csect_sec" variable in scan_xcoff_symtab to
     contain the XCOFF section number rather than the SECT_OFF_*
     index.  This is necessary because we need to get both the
     associated SECT_OFF_* index as well its BFD section.  We cannot
     find the BFD section from the SECT_OFF_* index, so we keep
     the XCOFF index instead, and transform it into the SECT_OFF_*
     index on demand.

gdb/ChangeLog:

        * xcoffread.c (xcoff_secnum_to_sections): New function.
        (secnum_to_section, secnum_to_bfd_section): Reimplement
        using xcoff_secnum_to_sections.  Rename "secnum" parameter
        into "n_scnum".
        (RECORD_MINIMAL_SYMBOL): Delete.
        (record_minimal_symbol): New function.
        (scan_xcoff_symtab): Replace uses of RECORD_MINIMAL_SYMBOL
        by call to record_minimal_symbol and set misc_func_recorded
        to 1.  Set last_csect_sec to the XCOFF section index instead
        of GDB's section_offset index.  Update calls to
        prim_record_minimal_symbol_and_info to pass the BFD section
        as well.

Tested on ppc-aix using AdaCore's testsuite.  I will also test using
the official testsuite, but I think I will have to go back to the
sources prior to Keith's linespec rewrite to be able to get one good
run.

---
 gdb/xcoffread.c |  143 ++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 88 insertions(+), 55 deletions(-)

diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
index fcabddb..1a1b5de 100644
--- a/gdb/xcoffread.c
+++ b/gdb/xcoffread.c
@@ -272,36 +272,53 @@ find_targ_sec (bfd *abfd, asection *sect, void *obj)
     }
 }
 
-/* Return the section number (SECT_OFF_*) that CS points to.  */
-static int
-secnum_to_section (int secnum, struct objfile *objfile)
-{
-  int off = SECT_OFF_TEXT (objfile);
+/* Search all BFD sections for the section whose target_index is
+   equal to N_SCNUM.  Set *BFD_SECT to that section.  The section's
+   associated index in the objfile's section_offset table is also
+   stored in *SECNUM.
 
-  asection *sect = NULL;
+   If no match is found, *BFD_SECT is set to NULL, and *SECNUM
+   is set to the text section's number.  */
+
+static void
+xcoff_secnum_to_sections (int n_scnum, struct objfile *objfile,
+			  asection **bfd_sect, int *secnum)
+{
   struct find_targ_sec_arg args;
-  args.targ_index = secnum;
-  args.resultp = &off;
-  args.bfd_sect = §
+
+  args.targ_index = n_scnum;
+  args.resultp = secnum;
+  args.bfd_sect = bfd_sect;
   args.objfile = objfile;
+
+  *bfd_sect = NULL;
+  *secnum = SECT_OFF_TEXT (objfile);
+
   bfd_map_over_sections (objfile->obfd, find_targ_sec, &args);
-  return off;
 }
 
-/* Return the BFD section that CS points to.  */
+/* Return the section number (SECT_OFF_*) that N_SCNUM points to.  */
+
+static int
+secnum_to_section (int n_scnum, struct objfile *objfile)
+{
+  int secnum;
+  asection *ignored;
+
+  xcoff_secnum_to_sections (n_scnum, objfile, &ignored, &secnum);
+  return secnum;
+}
+
+/* Return the BFD section that N_SCNUM points to.  */
+
 static asection *
-secnum_to_bfd_section (int secnum, struct objfile *objfile)
+secnum_to_bfd_section (int n_scnum, struct objfile *objfile)
 {
-  int off = SECT_OFF_TEXT (objfile);
+  int ignored;
+  asection *bfd_sect;
 
-  asection *sect = NULL;
-  struct find_targ_sec_arg args;
-  args.targ_index = secnum;
-  args.resultp = &off;
-  args.bfd_sect = §
-  args.objfile = objfile;
-  bfd_map_over_sections (objfile->obfd, find_targ_sec, &args);
-  return sect;
+  xcoff_secnum_to_sections (n_scnum, objfile, &bfd_sect, &ignored);
+  return bfd_sect;
 }
 \f
 /* add a given stab string into given stab vector.  */
@@ -877,21 +894,34 @@ enter_line_range (struct subfile *subfile, unsigned beginoffset,
    This function can read past the end of the symbol table
    (into the string table) but this does no harm.  */
 
-/* Reading symbol table has to be fast! Keep the followings as macros, rather
-   than functions.  */
-
-#define	RECORD_MINIMAL_SYMBOL(NAME, ADDR, TYPE, SECTION, OBJFILE) \
-{						\
-  const char *namestr;				\
-						\
-  namestr = (NAME);				\
-  if (namestr[0] == '.') ++namestr;		\
-  prim_record_minimal_symbol_and_info (namestr, (ADDR), (TYPE),     \
-				       (SECTION), (asection *)NULL, \
-				       (OBJFILE));		    \
-  misc_func_recorded = 1;			\
-}
+/* Create a new minimal symbol (using prim_record_minimal_symbol_and_info).
+
+   Arguments are:
+
+   NAME - the symbol's name (but if NAME starts with a period, that
+   leading period is discarded).
+   ADDRESS - the symbol's address.
+   MS_TYPE - the symbol's type.
+   N_SCNUM - the symbol's XCOFF section number.
+   OBJFILE - the objfile associated with the minimal symbol.  */
+
+static void
+record_minimal_symbol (const char *name, CORE_ADDR address,
+		       enum minimal_symbol_type ms_type,
+		       int n_scnum,
+		       struct objfile *objfile)
+{
+  struct find_targ_sec_arg args;
+  int secnum;
+  asection *bfd_sect;
 
+  if (name[0] == '.')
+    ++name;
+
+  xcoff_secnum_to_sections (n_scnum, objfile, &bfd_sect, &secnum);
+  prim_record_minimal_symbol_and_info (name, address, ms_type,
+				       secnum, bfd_sect, objfile);
+}
 
 /* xcoff has static blocks marked in `.bs', `.es' pairs.  They cannot be
    nested.  At any given time, a symbol can only be in one static block.
@@ -2277,10 +2307,10 @@ scan_xcoff_symtab (struct objfile *objfile)
 
 			if (!misc_func_recorded)
 			  {
-			    RECORD_MINIMAL_SYMBOL
+			    record_minimal_symbol
 			      (last_csect_name, last_csect_val,
-			       mst_text, last_csect_sec,
-			       objfile);
+			       mst_text, last_csect_sec, objfile);
+			    misc_func_recorded = 1;
 			  }
 
 			if (pst != NULL)
@@ -2312,8 +2342,7 @@ scan_xcoff_symtab (struct objfile *objfile)
 		      {
 			last_csect_name = namestring;
 			last_csect_val = symbol.n_value;
-			last_csect_sec =
-			  secnum_to_section (symbol.n_scnum, objfile);
+			last_csect_sec = symbol.n_scnum;
 		      }
 		    if (pst != NULL)
 		      {
@@ -2337,7 +2366,8 @@ scan_xcoff_symtab (struct objfile *objfile)
 			(namestring, symbol.n_value,
 			 sclass == C_HIDEXT ? mst_file_data : mst_data,
 			 secnum_to_section (symbol.n_scnum, objfile),
-			 NULL, objfile);
+			 secnum_to_bfd_section (symbol.n_scnum, objfile),
+			 objfile);
 		    break;
 
 		  case XMC_TC0:
@@ -2371,11 +2401,13 @@ scan_xcoff_symtab (struct objfile *objfile)
 		    if (first_fun_line_offset == 0 && symbol.n_numaux > 1)
 		      first_fun_line_offset =
 			main_aux[0].x_sym.x_fcnary.x_fcn.x_lnnoptr;
-		    RECORD_MINIMAL_SYMBOL
-		      (namestring, symbol.n_value,
-		       sclass == C_HIDEXT ? mst_file_text : mst_text,
-		       secnum_to_section (symbol.n_scnum, objfile),
-		       objfile);
+		      {
+			record_minimal_symbol
+			  (namestring, symbol.n_value,
+			   sclass == C_HIDEXT ? mst_file_text : mst_text,
+			   symbol.n_scnum, objfile);
+			misc_func_recorded = 1;
+		      }
 		    break;
 
 		  case XMC_GL:
@@ -2386,11 +2418,10 @@ scan_xcoff_symtab (struct objfile *objfile)
 		       mst_solib_trampoline symbol.  When we lookup mst
 		       symbols, we will choose mst_text over
 		       mst_solib_trampoline.  */
-		    RECORD_MINIMAL_SYMBOL
+		    record_minimal_symbol
 		      (namestring, symbol.n_value,
-		       mst_solib_trampoline,
-		       secnum_to_section (symbol.n_scnum, objfile),
-		       objfile);
+		       mst_solib_trampoline, symbol.n_scnum, objfile);
+		    misc_func_recorded = 1;
 		    break;
 
 		  case XMC_DS:
@@ -2413,7 +2444,8 @@ scan_xcoff_symtab (struct objfile *objfile)
 			(namestring, symbol.n_value,
 			 sclass == C_HIDEXT ? mst_file_data : mst_data,
 			 secnum_to_section (symbol.n_scnum, objfile),
-			 NULL, objfile);
+			 secnum_to_bfd_section (symbol.n_scnum, objfile),
+			 objfile);
 		    break;
 		  }
 		break;
@@ -2430,7 +2462,8 @@ scan_xcoff_symtab (struct objfile *objfile)
 			(namestring, symbol.n_value,
 			 sclass == C_HIDEXT ? mst_file_bss : mst_bss,
 			 secnum_to_section (symbol.n_scnum, objfile),
-			 NULL, objfile);
+			 secnum_to_bfd_section (symbol.n_scnum, objfile),
+			 objfile);
 		    break;
 		  }
 		break;
@@ -2456,9 +2489,9 @@ scan_xcoff_symtab (struct objfile *objfile)
 		   it as a function.  This will take care of functions like
 		   strcmp() compiled by xlc.  */
 
-		RECORD_MINIMAL_SYMBOL
-		  (last_csect_name, last_csect_val,
-		   mst_text, last_csect_sec, objfile);
+		record_minimal_symbol (last_csect_name, last_csect_val,
+				       mst_text, last_csect_sec, objfile);
+		misc_func_recorded = 1;
 	      }
 
 	    if (pst)
-- 
1.6.5.rc2

Re: [RFA/commit 2/2] pspace != NULL failed assertion on ppc-aix

[Joel Brobecker]

> gdb/ChangeLog:
> 
>         * xcoffread.c (xcoff_secnum_to_sections): New function.
>         (secnum_to_section, secnum_to_bfd_section): Reimplement
>         using xcoff_secnum_to_sections.  Rename "secnum" parameter
>         into "n_scnum".
>         (RECORD_MINIMAL_SYMBOL): Delete.
>         (record_minimal_symbol): New function.
>         (scan_xcoff_symtab): Replace uses of RECORD_MINIMAL_SYMBOL
>         by call to record_minimal_symbol and set misc_func_recorded
>         to 1.  Set last_csect_sec to the XCOFF section index instead
>         of GDB's section_offset index.  Update calls to
>         prim_record_minimal_symbol_and_info to pass the BFD section
>         as well.

Unfortunately, as explained at
http://www.sourceware.org/ml/gdb-patches/2012-04/msg00523.html,  I was
not able to test this patch against the official testsuite. I think
it's an expect problem on AIX, and I did take the time a long time
to look into the expect sources, and fix some of the problems I faced
at the time.  But I don't want to do that again, not for AIX.

As far as I am concerned, I am satisfied with just running the
AdaCore testsuite, where it shows no regression, and I think it
is an overall improvement as well. I don't know if anyone else really
cares about that port? In the meantime, I have checked the patch in.
It's easy to revert if there are objections.

-- 
Joel

Re: [RFA/commit 2/2] pspace != NULL failed assertion on ppc-aix

[Pedro Alves]

On 04/18/2012 01:31 AM, Joel Brobecker wrote:

> Unfortunately, as explained at
> http://www.sourceware.org/ml/gdb-patches/2012-04/msg00523.html,  I was
> not able to test this patch against the official testsuite. I think
> it's an expect problem on AIX, and I did take the time a long time
> to look into the expect sources, and fix some of the problems I faced
> at the time.  But I don't want to do that again, not for AIX.


You really really need to look into remote host testing.  :-) In that
mode, dejagnu runs on e.g., a GNU/Linux machine, which ssh's into the AIX
host just to run the tests against GDB.  No except runs on the remote host
that way.

-- 
Pedro Alves

Re: [RFA/commit 2/2] pspace != NULL failed assertion on ppc-aix

[Joel Brobecker]

> You really really need to look into remote host testing.  :-) In that
> mode, dejagnu runs on e.g., a GNU/Linux machine, which ssh's into the AIX
> host just to run the tests against GDB.  No except runs on the remote host
> that way.

That would be nice. Is there a pointer somewhere on how to do that?

Thanks,
-- 
Joel

Re: [RFA/commit 2/2] pspace != NULL failed assertion on ppc-aix

[Pedro Alves]

On 04/18/2012 03:54 PM, Joel Brobecker wrote:

>> You really really need to look into remote host testing.  :-) In that
>> mode, dejagnu runs on e.g., a GNU/Linux machine, which ssh's into the AIX
>> host just to run the tests against GDB.  No except runs on the remote host
>> that way.
> 
> That would be nice. Is there a pointer somewhere on how to do that?


There's a "Remote Host Testing" section in the DejaGNU manual:

https://www.gnu.org/software/dejagnu/manual/x844.html#releng

Basically, you'll need a host board for --host_board=.  Unfortunately, I no
longer have access to examples, but I know it works.  :-)

-- 
Pedro Alves

checked in: [RFA/commit 1/2] Unused local variables in xcoffread.c:read_xcoff_symtab

[Joel Brobecker]

> gdb/ChangeLog:
> 
>         * xcoffread.c (read_xcoff_symtab): Delete variables
>         last_csect_val and last_csect_sec and associated code.

Checked in.

-- 
Joel

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

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> But I think it is wrong, because I think a lot of places in the GDB code
Joel> make the assumption that a minimal symbol's obj_section is not NULL, and
Joel> the only way to set it, I think, is to have the BFD section.

Joel> So, I think the function documentation should be changed to remove
Joel> the permission to pass NULL, and a gdb_assert should also be added
Joel> to verify that SYMBOL_OBJ_SECTION (msymbol) != NULL after the
Joel> BFD section to obj_section search.

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

This change is fine with me -- even more than fine, I think removing
special cases is generally better when possible.

However, if this is just a regression caused by linespec changes, maybe
it can also be fixed in another way.  That is, you can find a minsym's
objfile using msymbol_objfile; I think this would fix the possibly
problematic uses I see in linespec.c (the one in minsym_found is maybe
ok).

I wanted to mention this in case the change causes other regressions in
your testing.

Did you audit the other symbol readers?

Tom

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

[Joel Brobecker]

> This change is fine with me -- even more than fine, I think removing
> special cases is generally better when possible.

Thanks. I will commit this change today.

> However, if this is just a regression caused by linespec changes, maybe
> it can also be fixed in another way.  That is, you can find a minsym's
> objfile using msymbol_objfile; I think this would fix the possibly
> problematic uses I see in linespec.c (the one in minsym_found is maybe
> ok).

Yes, I agree. I read the entire thread, and it seems so simple to
fix the problem that way as well.  I think both approaches should
be applied.

> Did you audit the other symbol readers?

Hmmm, no. Wildo.

-- 
Joel

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

[Tom Tromey]

Joel> Yes, I agree. I read the entire thread, and it seems so simple to
Joel> fix the problem that way as well.  I think both approaches should
Joel> be applied.

Could you test my patch, applied to an unpatched gdb, on AIX?
If it works for you I will write a ChangeLog entry and check it in.
I don't have a good way to test it here AFAIK.

Tom

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

[Joel Brobecker]

> Could you test my patch, applied to an unpatched gdb, on AIX?
> If it works for you I will write a ChangeLog entry and check it in.
> I don't have a good way to test it here AFAIK.

Of course! I will explain this again next to the other AIX patches,
but I wasn't able to run the entire testsuite on AIX. After printing
the results of the first group of testcases, expect just hangs, and
the terminal also appears to be unrecoverably frozen. I've tried
killing the expect processes without luck, and I can't see any
runtest script running either, so my guess is that we're screwed.

So I tested your patch using AdaCore's testsuite, which shows that
it fixes the problem as well, with no unexpected failures. And since
this patch affects all platforms, I also tested it against the
official testsuite on x86_64-linux. No regression either.

So I think your patch is good to go.

-- 
Joel

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

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> So I tested your patch using AdaCore's testsuite, which shows that
Joel> it fixes the problem as well, with no unexpected failures. And since
Joel> this patch affects all platforms, I also tested it against the
Joel> official testsuite on x86_64-linux. No regression either.

Joel> So I think your patch is good to go.

Thanks Joel.
I'm checking it in now, with this ChangeLog entry:

2012-04-18  Tom Tromey  <tromey@redhat.com>

	* linespec.c (convert_linespec_to_sals): Don't use
	SYMBOL_OBJ_SECTION.
	(compare_msymbols): Arguments are minsym_and_objfile, not
	minimal_symbol*.  Don't use SYMBOL_OBJ_SECTION.

Tom

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

[Joel Brobecker]

> Did you audit the other symbol readers?

Here is what I found - not exactly completely pretty, and definitely
shows that it would have been a bad idea to add an assertion ;-).

What it also tells me is that, if some of the code out there
explicitly makes the assumption that the minsym's obj_section
is set, it is wrong. I remember seeing this, but in fact, that
was in elfread.c, where the assumption is true.

| * coff-pe-read.c:
|   Says: Used as a last resort if no debugging symbols recognized
|
|     - Calls prim_record_minimal_symbol from add_pe_exported_sym
|         => No bfd_section => No obj_section.
|       Fixing might not be completely straightforward, depends
|       (needs deeper investigation)...
|
| * dbxread.c:
|     - Calls prim_record_minimal_symbol from read_dbx_dynamic_symtab.
|         => No bfd_section.
|       Should be possible to fix? Need to re-learn about format first.
|
| * mdebugread.c:
|     - Calls prim_record_minimal_symbol_and_info, but sometimes with
|       NULL bfd_section. May not always be fixable:
|
|       What does the following mean? When can this happen?
|       > default:
|       >   /* This kind of symbol is not associated to a section.  */
|       >   section = -1;
|       >   bfd_section = NULL;
|
|     - The other calls to prim_record_minimal_symbol_and_info (with a NULL
|       bfd_class) should be replaceable by a call to record_minimal_symbol,
|       which provides the bfd_section (apart from the exception above).
|
| * mips-read.c:  Do we still support this format??? (bfd_arch_alpha)
|     - read_alphacoff_dynamic_symtab calls prim_record_minimal_symbol
|       => No bfd_section.
|
| * somread.c: (pa-hpux format - time to propose deprecation?)
|    - som_symtab_read calls prim_record_minimal_symbol => No bfd_section.
|      Don't remember SOM format anymore, but maybe fixable.
|
| * coffread.c, machoread.c:
|   OK: Calls prim_record_minimal_symbol_and_info with a bfd_section.
|
| * elfread.c:
|   OK: Calls prim_record_minimal_symbol_full with a bfd_section.

-- 
Joel

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

[Pedro Alves]

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

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

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

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

I agree, both that this would be preferable and that it might be hard.

In fact, on my current "split objfile" branch
(archer-tromey-multi-inferior-3), I had to break this link --
on the branch, the symbols are not relocated and may be shared
by multiple objfiles.  (I sent a WIP version of this branch to
gdb-patches a while ago.)

I broke the linkage in a somewhat hacky way, though.  The branch is big
enough that I didn't want to also try to fix this problem at the same
time.

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

No kidding.  I have often wondered about this myself.

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

It seems like we could perhaps use the BFD index everywhere.
This gets into a fair amount of ugly business.

Pedro> Like in the patch below.  When testing it, I ran linespec.exp
Pedro> first, and that revealed that msymbol_objfile had a bug in that
Pedro> it didn't look at all the pspaces, only the current (the test
Pedro> adds a second inferior, therefore a second pspace).  Not good
Pedro> when you want to look up the objfile because you want to know the
Pedro> msyms' pspace to begin with.

This seems odd to me.  And, with the split objfile stuff, it would be
actively wrong, since a given msymbol could appear in multiple pspaces.
Now, normally I wouldn't mention a patch that isn't checked in yet,
but...

I think it would be better to pass the pspace around with the minimal
symbol in linespec.c.  We certainly know it at the time we actually find
the minsym, so it is just a matter of not losing the information.

Ok, I just looked at linespec.c again, and this is why there is an
objfile in minsym_and_objfile.

Here's a totally untested patch based on this idea.

Note that compare_msymbols, while not exactly wrong right now, is
definitely weird in that it makes an assumption about the layout of
minsym_and_objfile.

Tom

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 228214b..7cbf1bf 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 = elem->objfile->pspace;
 	      set_current_program_space (pspace);
 	      minsym_found (state, elem->objfile, elem->minsym, &sals);
 	    }
@@ -2584,20 +2584,20 @@ compare_symbols (const void *a, const void *b)
 static int
 compare_msymbols (const void *a, const void *b)
 {
-  struct minimal_symbol * const *sa = a;
-  struct minimal_symbol * const *sb = b;
+  const struct minsym_and_objfile *sa = a;
+  const struct minsym_and_objfile *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) sa->objfile->pspace;
+  uib = (uintptr_t) sa->objfile->pspace;
 
   if (uia < uib)
     return -1;
   if (uia > uib)
     return 1;
 
-  uia = (uintptr_t) *sa;
-  uib = (uintptr_t) *sb;
+  uia = (uintptr_t) sa->minsym;
+  uib = (uintptr_t) sb->minsym;
 
   if (uia < uib)
     return -1;

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

[Pedro Alves]

On 04/17/2012 03:15 PM, Tom Tromey wrote:

> Pedro> Like in the patch below.  When testing it, I ran linespec.exp
> Pedro> first, and that revealed that msymbol_objfile had a bug in that
> Pedro> it didn't look at all the pspaces, only the current (the test
> Pedro> adds a second inferior, therefore a second pspace).  Not good
> Pedro> when you want to look up the objfile because you want to know the
> Pedro> msyms' pspace to begin with.
> 
> This seems odd to me.  And, with the split objfile stuff, it would be
> actively wrong, since a given msymbol could appear in multiple pspaces.
> Now, normally I wouldn't mention a patch that isn't checked in yet,
> but...


Indeed.

> 
> I think it would be better to pass the pspace around with the minimal
> symbol in linespec.c.  We certainly know it at the time we actually find
> the minsym, so it is just a matter of not losing the information.
> 
> Ok, I just looked at linespec.c again, and this is why there is an
> objfile in minsym_and_objfile.
> 
> Here's a totally untested patch based on this idea.


I definitely like this even more.

-- 
Pedro Alves