[rfa] eliminate some annoying mdebug-related symtab crashes

[rfa] eliminate some annoying mdebug-related symtab crashes

[Daniel Jacobowitz]

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.


The init_psymbol_list is a little trickier.  Normally, both the global and
static symbol lists for an objfile are pre-allocated based on the expected
number of symbols.  mdebugread does not do that, which is, I think, fine. 
If global symbols are read but no static symbols are read, which has
happened to me in the startfiles several times, we should not be
re-initializing the list of symbols - there are psymtabs pointing in to what
we're freeing.  We only want to do that if neither global nor static symbols
have been read.


Are these OK to commit?

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer
From fnasser@cygnus.com Mon Jul 09 15:13:00 2001
From: Fernando Nasser <fnasser@cygnus.com>
To: Daniel Jacobowitz <dmj+@andrew.cmu.edu>
Cc: gdb-patches@sourceware.cygnus.com
Subject: Re: [RFA] Testsuite addition for x86 linux GDB and SIGALRM fix
Date: Mon, 09 Jul 2001 15:13:00 -0000
Message-id: <3B4A2C7C.85C688C4@cygnus.com>
References: <200005192321.e4JNLEv13368@delius.kettenis.local> <3B3ABD6E.1040304@cygnus.com> <3B4A2056.4D58E307@cygnus.com> <20010709143406.A17003@nevyn.them.org>
X-SW-Source: 2001-07/msg00222.html
Content-length: 1657

Daniel Jacobowitz wrote:
> 
> On Mon, Jul 09, 2001 at 05:21:26PM -0400, Fernando Nasser wrote:
> > W.r.t. the tests for HP and IA64 I sincerely regret that we do not
> > have two commands: "finishi" and "finish".  The current behavior of
> > "finish" (stop at the assembler instruction after the call) is very
> > unsettling for someone who is doing source level debugging -- in this
> > case it should, after returning, single step until the end of the
> > sourceline where the call is ("if it is not at the beginning of a
> > source line after the return, single step to the end of it" would
> > do).
> 
> I think that the current behavior of finish, while awkward, is better
> than what you're suggesting here.  Suppose we have:
>   foo (bar (x));
> and we want to step in to foo.  There's two ways to do it; a breakpoint
> on foo, or step - finish - step. 

The breakpoint is the correct way.  The latter is an artifact.

> Stepping in to bar, typing finish,
> and ending up after the call to foo would be exceedingly non-intuitive.
> 

This is true. But a finish would not stop after the call to foo() in this case.  The stepping would be aborted as we entered foo() itself (note that I said "step", not "next").  The result is quite intuitive in this case and you just provided one good example of how we could use it -- one could go "finish"-ing until the desired function was entered (without the need to step again and without the weird thing of appearing to stop at the same line you were before).



-- 
Fernando Nasser
Red Hat - Toronto                       E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

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

[Daniel Jacobowitz]

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.

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

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

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?

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

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

[Elena Zannoni]

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

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

[Daniel Jacobowitz]

On Fri, Jul 20, 2001 at 10:24:25PM -0400, Elena Zannoni wrote:
> 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.

Exactly.

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

I'll try to do this and get back to you about it next week.

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

I didn't do that because I couldn't find a reasonable NUM_OF_SYMBOLS,
but I could just use a hardcoded guess like some of the other symbol
readers do, I suppose.  I'd rather fix it as I proposed; I'll run those
testsuites and see what I can turn up.

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

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

[Daniel Jacobowitz]

On Fri, Jul 20, 2001 at 10:24:25PM -0400, Elena Zannoni wrote:
> 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)?

I ran mipsel-linux stabs with and without, and i386-linux stabs/dwarf2
with and without.  The numbers all look in line, and no unexpected
crashes.

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