Re: [rfa] ALL_OBJFILE_MSYMBOLS

[Jim Blandy <jimb at redhat dot com>]

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