[rfa] ALL_OBJFILE_MSYMBOLS

[rfa] ALL_OBJFILE_MSYMBOLS

[David Carlton]

When playing around with anonymous objfiles on a branch, I noticed
another problem with them: the macro ALL_OBJFILE_MSYMBOLS assumes that
objfile->msymbols is always non-NULL, which isn't the case if you have
an artificial objfile floating around.

Looking further, I noticed that the macro ALL_MSYMBOLS already has an
explicit test that objfile->msymbols is non-NULL; it seems to me that
the easiest thing to do is is to require users of ALL_OBJFILE_MSYMBOLS
to first check that objfile->msymbols is non-NULL.

Here's a patch that does that, along with adding some comments to
objfiles.h explaining the situation.  It turns out that
ALL_OBJFILE_MSYMBOLS is only used in two places; in two of those
places, it's immediately within ALL_OBJFILES, so the intended effect
is to look at all the msymbols; I thus replaced those two occurrences
by uses of ALL_MSYMBOLS.  The third occurrence isn't surrounded by a
use of ALL_OBJFILES, so I just enclosed it in a test that
objfile->msymbols is non-NULL.

Like my earlier allocate_objfile(NULL, 0) patch, I don't know of any
concrete bugs that this fixes on mainline GDB, though I could imagine
that Java code could run into this problem since Java does allocate an
anonymous objfile.  But the patch is so straightforward that I don't
see how applying it could hurt.  (It looks a little more complicated
than it is because it changes the indentation levels.)

Tested on i686-pc-linux-gnu/GCC3.1/DWARF-2; I also verified that 'make
arm-linux-tdep.o' compiles without errors, though I haven't run that
code directly.  (The code in question in arm-linux-tdep.c is
essentially identical to the code in i386-linux-tdep.c.)

David Carlton
carlton@math.stanford.edu

2003-01-27  David Carlton  <carlton@math.stanford.edu>

	* objfiles.h: Add comments about objfile->msymbols being NULL.
	* objfiles.c (objfile_relocate): Enclose ALL_OBJFILE_MSYMBOLS in
	guard.
	* i386-linux-tdep.c (find_minsym_and_objfile): Call ALL_MSYMBOLS
	instead of ALL_OBJFILES and ALL_OBJFILE_MSYMBOLS.
	* arm-linux-tdep.c (find_minsym_and_objfile): Ditto.

Index: objfiles.h
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.h,v
retrieving revision 1.17
diff -u -p -r1.17 objfiles.h
--- objfiles.h	23 Jan 2003 23:03:31 -0000	1.17
+++ objfiles.h	28 Jan 2003 22:09:15 -0000
@@ -300,6 +300,12 @@ struct objfile
        null symbol.  The array itself, as well as all the data that it points
        to, should be allocated on the symbol_obstack for this file. */
 
+    /* NOTE: carlton/2003-01-27: For a newly-created objfile, msymbols
+       is set to NULL, rather than a one-element array ending in a
+       null symbol.  ALL_MSYMBOLS already guarded against that case,
+       so that seems to be a valid possibility; it can be useful if
+       you like to create artificial objfiles.  */
+
     struct minimal_symbol *msymbols;
     int minimal_symbol_count;
 
@@ -574,6 +580,10 @@ extern int is_in_import_list (char *, st
     for ((p) = (objfile) -> psymtabs; (p) != NULL; (p) = (p) -> next)
 
 /* Traverse all minimal symbols in one objfile.  */
+
+/* NOTE: carlton/2003-01-27: Don't call this macro unless
+   objfile->msymbols is non-NULL.  See NOTE above in the declaration
+   of 'struct objfile'.  */
 
 #define	ALL_OBJFILE_MSYMBOLS(objfile, m) \
     for ((m) = (objfile) -> msymbols; SYMBOL_NAME(m) != NULL; (m)++)
Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.24
diff -u -p -r1.24 objfiles.c
--- objfiles.c	23 Jan 2003 23:03:31 -0000	1.24
+++ objfiles.c	28 Jan 2003 22:08:14 -0000
@@ -674,12 +674,15 @@ objfile_relocate (struct objfile *objfil
       }
   }
 
-  {
-    struct minimal_symbol *msym;
-    ALL_OBJFILE_MSYMBOLS (objfile, msym)
-      if (SYMBOL_SECTION (msym) >= 0)
-      SYMBOL_VALUE_ADDRESS (msym) += ANOFFSET (delta, SYMBOL_SECTION (msym));
-  }
+  if (objfile->msymbols != NULL)
+    {
+      struct minimal_symbol *msym;
+      ALL_OBJFILE_MSYMBOLS (objfile, msym)
+	if (SYMBOL_SECTION (msym) >= 0)
+	  SYMBOL_VALUE_ADDRESS (msym)
+	    += ANOFFSET (delta, SYMBOL_SECTION (msym));
+    }
+
   /* Relocating different sections by different amounts may cause the symbols
      to be out of order.  */
   msymbols_sort (objfile);
Index: i386-linux-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-linux-tdep.c,v
retrieving revision 1.22
diff -u -p -r1.22 i386-linux-tdep.c
--- i386-linux-tdep.c	8 Jan 2003 22:47:46 -0000	1.22
+++ i386-linux-tdep.c	28 Jan 2003 22:07:08 -0000
@@ -325,19 +325,15 @@ static struct minimal_symbol *
 find_minsym_and_objfile (char *name, struct objfile **objfile_p)
 {
   struct objfile *objfile;
+  struct minimal_symbol *msym;
 
-  ALL_OBJFILES (objfile)
+  ALL_MSYMBOLS (objfile, msym)
     {
-      struct minimal_symbol *msym;
-
-      ALL_OBJFILE_MSYMBOLS (objfile, msym)
+      if (SYMBOL_NAME (msym)
+	  && STREQ (SYMBOL_NAME (msym), name))
 	{
-	  if (SYMBOL_NAME (msym)
-	      && STREQ (SYMBOL_NAME (msym), name))
-	    {
-	      *objfile_p = objfile;
-	      return msym;
-	    }
+	  *objfile_p = objfile;
+	  return msym;
 	}
     }
 
Index: arm-linux-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-linux-tdep.c,v
retrieving revision 1.25
diff -u -p -r1.25 arm-linux-tdep.c
--- arm-linux-tdep.c	4 Jan 2003 23:38:44 -0000	1.25
+++ arm-linux-tdep.c	28 Jan 2003 22:06:25 -0000
@@ -356,19 +356,15 @@ static struct minimal_symbol *
 find_minsym_and_objfile (char *name, struct objfile **objfile_p)
 {
   struct objfile *objfile;
+  struct minimal_symbol *msym;
 
-  ALL_OBJFILES (objfile)
+  ALL_MSYMBOLS (objfile, msym)
     {
-      struct minimal_symbol *msym;
-
-      ALL_OBJFILE_MSYMBOLS (objfile, msym)
+      if (SYMBOL_NAME (msym)
+	  && strcmp (SYMBOL_NAME (msym), name) == 0)
 	{
-	  if (SYMBOL_NAME (msym)
-	      && strcmp (SYMBOL_NAME (msym), name) == 0)
-	    {
-	      *objfile_p = objfile;
-	      return msym;
-	    }
+	  *objfile_p = objfile;
+	  return msym;
 	}
     }
 

Re: [rfa] ALL_OBJFILE_MSYMBOLS

[Tom Tromey]

>>>>> "David" == David Carlton <carlton@math.stanford.edu> writes:

David> Like my earlier allocate_objfile(NULL, 0) patch, I don't know
David> of any concrete bugs that this fixes on mainline GDB, though I
David> could imagine that Java code could run into this problem since
David> Java does allocate an anonymous objfile.

I just wanted to say: I tried your earlier patch locally.  With it,
gdb crashes a lot less when debugging gcj-compiled code.  In
particular, I can re-run the inferior more or less reliably (whereas
before this would crash gdb pretty reliably).  I'm keeping it in my
gdb tree to make my life easier.

I'll save this new patch and hopefully someday find the time to see if
it helps.

Tom

Re: [rfa] ALL_OBJFILE_MSYMBOLS

[Jim Blandy]

David Carlton <carlton@math.stanford.edu> writes:
> When playing around with anonymous objfiles on a branch, I noticed
> another problem with them: the macro ALL_OBJFILE_MSYMBOLS assumes that
> objfile->msymbols is always non-NULL, which isn't the case if you have
> an artificial objfile floating around.
> 
> Looking further, I noticed that the macro ALL_MSYMBOLS already has an
> explicit test that objfile->msymbols is non-NULL; it seems to me that
> the easiest thing to do is is to require users of ALL_OBJFILE_MSYMBOLS
> to first check that objfile->msymbols is non-NULL.
> 
> Here's a patch that does that, along with adding some comments to
> objfiles.h explaining the situation.  It turns out that
> ALL_OBJFILE_MSYMBOLS is only used in two places; in two of those
> places, it's immediately within ALL_OBJFILES, so the intended effect
> is to look at all the msymbols; I thus replaced those two occurrences
> by uses of ALL_MSYMBOLS.  The third occurrence isn't surrounded by a
> use of ALL_OBJFILES, so I just enclosed it in a test that
> objfile->msymbols is non-NULL.
> 
> Like my earlier allocate_objfile(NULL, 0) patch, I don't know of any
> concrete bugs that this fixes on mainline GDB, though I could imagine
> that Java code could run into this problem since Java does allocate an
> anonymous objfile.  But the patch is so straightforward that I don't
> see how applying it could hurt.  (It looks a little more complicated
> than it is because it changes the indentation levels.)
> 
> Tested on i686-pc-linux-gnu/GCC3.1/DWARF-2; I also verified that 'make
> arm-linux-tdep.o' compiles without errors, though I haven't run that
> code directly.  (The code in question in arm-linux-tdep.c is
> essentially identical to the code in i386-linux-tdep.c.)
> 
> David Carlton
> carlton@math.stanford.edu
> 
> 2003-01-27  David Carlton  <carlton@math.stanford.edu>
> 
> 	* objfiles.h: Add comments about objfile->msymbols being NULL.
> 	* objfiles.c (objfile_relocate): Enclose ALL_OBJFILE_MSYMBOLS in
> 	guard.
> 	* i386-linux-tdep.c (find_minsym_and_objfile): Call ALL_MSYMBOLS
> 	instead of ALL_OBJFILES and ALL_OBJFILE_MSYMBOLS.
> 	* arm-linux-tdep.c (find_minsym_and_objfile): Ditto.

I think I'd prefer the patch below.  Could you try it out and see if
it works as well?

This was messier than I had expected.  My first impulse was to make
ALL_OBJFILE_MSYMBOLS just do the 'if' itself.  But it's a macro, and
there's no way to prevent the macro from grabbing any 'else' that
happens to follow the loop body.  (Which is a great example of one
reason C macros suck and Lisp macros don't --- in case that's an
emotionally charged issue for anyone else out there like it is for me.
:) )

But it would be better anyway for an empty minsym table to have a
single, consistent representation.  So I thought, "Hey, why don't we
just forget about the terminating entry, and always use the count?"
But there's no way I could see to reliably find all the code that
expects it.  Not every loop uses ALL_OBJFILE_MSYMBOLS.  And since not
every loop need necessarily refer to objfile->msymbols, you can't grep
for that either.  And grepping for uses of 'struct msymbol' yields
hundreds of hits.

So here's a patch which simply ensures that every objfile's minsym
table has a terminating entry, and makes some appropriate accompanying
changes.  The tests are still running, but I haven't noticed any
regressions yet.

2003-01-31  Jim Blandy  <jimb@redhat.com>

	Use a single, consistent representation for an empty minimal
	symbol table in an objfile.
	* objfiles.c (terminate_minimal_symbol_table): New function.
	(allocate_objfile): Call it.
	* objfiles.h (terminate_minimal_symbol_table): New declaration.
	(ALL_MSYMBOLS): No need to test whether (objfile)->msymbols is
	non-NULL. 
	* minsyms.c (lookup_minimal_symbol_by_pc_section): To see whether
	objfile has minimal symbols, compare minimal_symbol_count to zero,
	instead of comparing msymbols with NULL.
	* objfiles.c (have_minimal_symbols): Same.
	* solib-sunos.c (solib_add_common_symbols): Call
	terminate_minimal_symbol_table.
	* symfile.c (reread_symbols): Same.
	
Index: gdb/minsyms.c
===================================================================
RCS file: /cvs/src/src/gdb/minsyms.c,v
retrieving revision 1.23
diff -c -r1.23 minsyms.c
*** gdb/minsyms.c	8 Jan 2003 18:38:47 -0000	1.23
--- gdb/minsyms.c	31 Jan 2003 19:52:29 -0000
***************
*** 411,418 ****
           "null symbol".  If there are no real symbols, then there is no
           minimal symbol table at all. */
  
!       if ((msymbol = objfile->msymbols) != NULL)
  	{
  	  lo = 0;
  	  hi = objfile->minimal_symbol_count - 1;
  
--- 411,419 ----
           "null symbol".  If there are no real symbols, then there is no
           minimal symbol table at all. */
  
!       if (objfile->minimal_symbol_count > 0)
  	{
+           msymbol = objfile->msymbols;
  	  lo = 0;
  	  hi = objfile->minimal_symbol_count - 1;
  
Index: gdb/objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.24
diff -c -r1.24 objfiles.c
*** gdb/objfiles.c	23 Jan 2003 23:03:31 -0000	1.24
--- gdb/objfiles.c	31 Jan 2003 19:52:29 -0000
***************
*** 281,286 ****
--- 281,288 ----
        obstack_specify_allocation (&objfile->type_obstack, 0, 0, xmalloc,
  				  xfree);
        flags &= ~OBJF_MAPPED;
+ 
+       terminate_minimal_symbol_table (objfile);
      }
  
    /* Update the per-objfile information that comes from the bfd, ensuring
***************
*** 333,338 ****
--- 335,367 ----
    return (objfile);
  }
  
+ 
+ /* Create the terminating entry of OBJFILE's minimal symbol table.
+    If OBJFILE->msymbols is zero, allocate a single entry from
+    OBJFILE->symbol_obstack; otherwise, just initialize
+    OBJFILE->msymbols[OBJFILE->minimal_symbol_count].  */
+ void
+ terminate_minimal_symbol_table (struct objfile *objfile)
+ {
+   if (! objfile->msymbols)
+     objfile->msymbols = ((struct minimal_symbol *)
+                          obstack_alloc (&objfile->symbol_obstack,
+                                         sizeof (objfile->msymbols[0])));
+ 
+   {
+     struct minimal_symbol *m
+       = &objfile->msymbols[objfile->minimal_symbol_count];
+ 
+     memset (m, 0, sizeof (*m));
+     SYMBOL_NAME (m) = NULL;
+     SYMBOL_VALUE_ADDRESS (m) = 0;
+     MSYMBOL_INFO (m) = NULL;
+     MSYMBOL_TYPE (m) = mst_unknown;
+     SYMBOL_INIT_LANGUAGE_SPECIFIC (m, language_unknown);
+   }
+ }
+ 
+ 
  /* Put one object file before a specified on in the global list.
     This can be used to make sure an object file is destroyed before
     another when using ALL_OBJFILES_SAFE to free all objfiles. */
***************
*** 810,816 ****
  
    ALL_OBJFILES (ofp)
    {
!     if (ofp->msymbols != NULL)
        {
  	return 1;
        }
--- 839,845 ----
  
    ALL_OBJFILES (ofp)
    {
!     if (ofp->minimal_symbol_count > 0)
        {
  	return 1;
        }
Index: gdb/objfiles.h
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.h,v
retrieving revision 1.18
diff -c -r1.18 objfiles.h
*** gdb/objfiles.h	29 Jan 2003 23:46:39 -0000	1.18
--- gdb/objfiles.h	31 Jan 2003 19:52:30 -0000
***************
*** 515,520 ****
--- 515,522 ----
  
  extern int build_objfile_section_table (struct objfile *);
  
+ extern void terminate_minimal_symbol_table (struct objfile *objfile);
+ 
  extern void put_objfile_before (struct objfile *, struct objfile *);
  
  extern void objfile_to_front (struct objfile *);
***************
*** 595,602 ****
  
  #define	ALL_MSYMBOLS(objfile, m) \
    ALL_OBJFILES (objfile)	 \
!     if ((objfile)->msymbols)	 \
!       ALL_OBJFILE_MSYMBOLS (objfile, m)
  
  #define ALL_OBJFILE_OSECTIONS(objfile, osect)	\
    for (osect = objfile->sections; osect < objfile->sections_end; osect++)
--- 597,603 ----
  
  #define	ALL_MSYMBOLS(objfile, m) \
    ALL_OBJFILES (objfile)	 \
!     ALL_OBJFILE_MSYMBOLS (objfile, m)
  
  #define ALL_OBJFILE_OSECTIONS(objfile, osect)	\
    for (osect = objfile->sections; osect < objfile->sections_end; osect++)
Index: gdb/solib-sunos.c
===================================================================
RCS file: /cvs/src/src/gdb/solib-sunos.c,v
retrieving revision 1.6
diff -c -r1.6 solib-sunos.c
*** gdb/solib-sunos.c	20 Oct 2002 14:38:26 -0000	1.6
--- gdb/solib-sunos.c	31 Jan 2003 19:52:30 -0000
***************
*** 184,189 ****
--- 184,190 ----
  				  xmalloc, xfree);
        rt_common_objfile->minimal_symbol_count = 0;
        rt_common_objfile->msymbols = NULL;
+       terminate_minimal_symbol_table (rt_common_objfile);
      }
  
    init_minimal_symbol_collection ();
Index: gdb/symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.83
diff -c -r1.83 symfile.c
*** gdb/symfile.c	30 Jan 2003 21:45:07 -0000	1.83
--- gdb/symfile.c	31 Jan 2003 19:52:33 -0000
***************
*** 2018,2023 ****
--- 2018,2024 ----
  		  error ("Can't find the file sections in `%s': %s",
  			 objfile->name, bfd_errmsg (bfd_get_error ()));
  		}
+               terminate_minimal_symbol_table (objfile);
  
  	      /* We use the same section offsets as from last time.  I'm not
  	         sure whether that is always correct for shared libraries.  */

Re: [rfa] ALL_OBJFILE_MSYMBOLS

[David Carlton]

On 31 Jan 2003 15:01:24 -0500, Jim Blandy <jimb@redhat.com> said:

> I think I'd prefer the patch below.  Could you try it out and see if
> it works as well?

Yes, it also fixes the bug I'm seeing.

> This was messier than I had expected.

Yah.

> (Which is a great example of one reason C macros suck and Lisp
> macros don't --- in case that's an emotionally charged issue for
> anyone else out there like it is for me.  :) )

I thought for a while about good responses to this, but I think I'll
just leave it alone for now. :-)

> But it would be better anyway for an empty minsym table to have a
> single, consistent representation.

That was my first reaction, too.  But while I was thinking about how
to do that, I noticed that ALL_MSYMBOLS already dealt with the
possibility of a NULL entry, so I just went with that out of laziness.

Like you, trusting the count sounds like a good idea.  And, even if
you go with a terminating entry, having it be a fake symbol instead
of, say, a NULL pointer is pretty weird, too.  But making either of
those changes sounds like too much (fallible) work for too little
benefit.

> So here's a patch which simply ensures that every objfile's minsym
> table has a terminating entry, and makes some appropriate accompanying
> changes.  The tests are still running, but I haven't noticed any
> regressions yet.

Seems sensible to me.  I didn't do a full testsuite run, but I did
apply it to my branch and try to trigger the bug I'd been seeing
there, and your patch does protect against the bug.

David Carlton
carlton@math.stanford.edu

Re: [rfa] ALL_OBJFILE_MSYMBOLS

[Jim Blandy]

Okay, I've committed this change.

(My test run completed with no new failures.)

David Carlton <carlton@math.stanford.edu> writes:

> On 31 Jan 2003 15:01:24 -0500, Jim Blandy <jimb@redhat.com> said:
> 
> > I think I'd prefer the patch below.  Could you try it out and see if
> > it works as well?
> 
> Yes, it also fixes the bug I'm seeing.
> 
> > This was messier than I had expected.
> 
> Yah.
> 
> > (Which is a great example of one reason C macros suck and Lisp
> > macros don't --- in case that's an emotionally charged issue for
> > anyone else out there like it is for me.  :) )
> 
> I thought for a while about good responses to this, but I think I'll
> just leave it alone for now. :-)
> 
> > But it would be better anyway for an empty minsym table to have a
> > single, consistent representation.
> 
> That was my first reaction, too.  But while I was thinking about how
> to do that, I noticed that ALL_MSYMBOLS already dealt with the
> possibility of a NULL entry, so I just went with that out of laziness.
> 
> Like you, trusting the count sounds like a good idea.  And, even if
> you go with a terminating entry, having it be a fake symbol instead
> of, say, a NULL pointer is pretty weird, too.  But making either of
> those changes sounds like too much (fallible) work for too little
> benefit.
> 
> > So here's a patch which simply ensures that every objfile's minsym
> > table has a terminating entry, and makes some appropriate accompanying
> > changes.  The tests are still running, but I haven't noticed any
> > regressions yet.
> 
> Seems sensible to me.  I didn't do a full testsuite run, but I did
> apply it to my branch and try to trigger the bug I'd been seeing
> there, and your patch does protect against the bug.
> 
> David Carlton
> carlton@math.stanford.edu