Re: [rfa] eliminate some annoying mdebug-related symtab crashes

[Elena Zannoni <ezannoni at cygnus dot com>]

Daniel Jacobowitz writes:
 > On Fri, Jul 20, 2001 at 07:27:12PM -0400, Elena Zannoni wrote:
 > > 
 > > Daniel Jacobowitz writes:
 > >  > I'm really looking forward to getting away from mdebug and back to straight
 > >  > ELF stabs, but I need mdebug for one last project.  This patch addresses two
 > >  > of the crashes I've been having - properly, this time.
 > >  > 
 > >  > The init_header_files fix is almost trivial, although it might be preferable
 > >  > to rename the functions now that I've had to make them non-static.  The list
 > >  > was NULL, mdebugread's psymtab_to_symtab_1 was calling dbxread's
 > >  > process_one_symbol which called add_new_header_file, and we crashed.  I'm
 > >  > not sure if the extra:
 > >  > 
 > >  > +  stabsread_new_init ();
 > >  > +  buildsym_new_init ();
 > >  > 
 > >  > is really necessary, since elfread_new_init() calls them, but analogy with
 > >  > every other existing symbol reader suggests that it is correct, or at least
 > >  > customary.
 > >  > 
 > > 
 > > Is the above another fix for the same problem you submitted in the
 > > previous patch?  If you rewrite a patch could you please withdraw the
 > > first one, so that no time goes into reviewing it?  Anyway, I consider
 > > this addressed. It occurred to me to do it this way, but I would be
 > > curious to see if the other way I suggested works as well.
 > 
 > The include files fix here is indeed for the same problem as in the
 > other patch.  I didn't withdraw the first patch because it seemed best
 > to me to fix it in both places - initialize properly, and don't die if
 > we didn't.  Defensive programming; I'm sick of GDB crashing on the
 > (admittedly somewhat bizarre) binaries I've been giving it.
 > 


Ah, that wasn't clear from the two messages.  I was more propense
towards finding a proper place for initialization. That's why I
suggested the other patch instead.


 > > The below is for another problem. They should really be two separate
 > > patches... Anyway.  For this I would be really hesitant to commit
 > 
 > Yes, you're right.  Sorry.
 > 
 > > changes to the other readers.  Unless you can show that there are no
 > > regressions. I think overall is better to leave those alone.  Even
 > > though the change seems to make sense, and the comments seem to imply
 > > that both list should be empty. But the code has been like that for ages, I
 > > just looked at the cvs repository, and it was like that already in
 > > 1995. I know this is not a real argument, but....
 > > 
 > > Can you elaborate some more on what's happening here?  Do you have 2
 > > different debug info sections, mdebug + stabs in the same object file?
 > > Do you end up with two psymtabs chains (one created by
 > > elfmdebug_build_psymtabs, the other by elfstabs_build_psymtabs)
 > > pointing to the same object file, therefore sharing the
 > > static_psymbols and global_psymbol lists? And this is why at the point
 > > of the call to init_psymbol_list from dbx_symfile_read (ultimately
 > > deriving from the call to elfstab_build_psymtabs) there is information
 > > in the psymbol lists already?  
 > > 
 > > According to elf_symfile_read there can be even more than two
 > > different debugging sections per object file. I wonder why this hasn't
 > > created problems for others before. What incorrect behavior do you
 > > see, i.e. what's the symptom, where do you get the crash? Can I see a
 > > stack trace?
 > > 
 > > Is there any way for me to reproduce this?  What are the platform and
 > > the target? Do you have an example program that exhibits these
 > > problems?
 > 
 > Reproducing it would be somewhat difficult.  It happened when the user
 > program was built with GCC 3.0 and had stabs debug info in normal
 > .stabs sections, and the C library (and specifically crt*.o) were built
 > by GCC 2.95 and an early binutils, and so had .mdedug sections.  I can
 > easily reproduce the other two crashes that I've submitted patches for,
 > but not this one.  There's also no useful backtrace; global_psymbol
 > gets corrupted when init_psymbol_list is called, and then a later
 > attempt to access a global symbol from crtbegin.o causes the crash.

I can see why, yes, there are only one static_psymbol list and one
global_psymbol list and they are all shared and manipulated by the psymtabs
builders that get called for each different debug format.
Whenever you call a psymtab builder you wipe those lists out.

 > 
 > The reason it doesn't seem to have caused problems before is that most
 > other symbol readers have a rough idea of how many symbols there will
 > be, and call init_psymbol_list with some non-zero value.  This
 > initializes both global and static sizes, and so the old check prevents
 > us from re-calling init_psymbol_lists, at a cost of some waste of space
 > (there seem to be substantially fewer static than global psymbols in
 > most of the tests I tried).
 > 

Oh dear, I feel ill. Where is the paper bag, quick!  So, the || check
before calling init_psymbol_list works if the two lists are kind of in
synch. They are grown/allocated together.  They are both zero at
start, and they get initialized both to the same size in
init_psymbol_lists.

So, in theory they are both of zero length at the same time.  Except
that in our call path to building the mdebug psymtabs, we never call
init_psymbol_lists with a real, non zero, size.  Then the two lists
get out of synch, because there is another way to grow the lists by
calling extend_psymbol_list, that just changes one list (this function
is called by add_psymbol_to_list).


 > mdebugread fills in only a few (11, I believe) global symbols from
 > crtbegin.o and no static symbols.  Then when we go into
 > dbx_symfile_read, we lose.
 > 
 > Does that make sense?

Yes, it is this line in elfread.c that sets the lists to null and their
sizes to 0.

elfread.c:599:      init_psymbol_list (objfile, 0);


Dig  dig  dig,  in  our  internal repository,  I  found  the  original
changelog entry (just missed the 10 years annyversary):

Mon Apr  8 23:57:43 1991  John Gilmore  (gnu at cygint.cygnus.com)

        * dbxread.c (dbx_symfile_read):  Initialize psymbol list if this
        is the first symbol read, even if not mainline.

I would think that extend_psymbol_list was introduced afterwards, and
this broke the logic of Gilmore's change.

So what to do?  I am kind of convinced that your patch is
correct. But..., is there any chance you can run the gdb testsuite
before and after your changes on a couple of platforms that have at
least dwarf2 and stabs debug info (I mean separately, not in the same
objfile)?

Another alternative would be to insert a call to
init_psymbol_lists(objfile, NUM_OF_SYMBOLS) somewhere in mdebugread.c.

Elena

 > -- 
 > Daniel Jacobowitz                           Carnegie Mellon University
 > MontaVista Software                         Debian GNU/Linux Developer