Simple but crucial bug fix to gdb

Simple but crucial bug fix to gdb

[Charlie Mills]

Dear gdb maintainer,

I would like to submit a very simple patch to gdb.
I would like if possible to avoid legal issues (avoid having to
submit a form to our legal department) by simply describing the fix,
which is a diff of only a few characters, rather than sending you a
source file.

Bug description:  gdb 4.xx and 5.0 crashes while reading our executable.
Our executable is the result of linking objects compiled by gcc with
other objects compiled using SPARCworks CC.  The stack trace is
appended at the end of this message.

Unfortunately the executable is large and proprietary.
Although I can't submit a test case, it is very easy to confirm by
inspecting the code that the patch is correct and the original code
is incorrect.  The patch is as follows:

File: gdb-5.0/gdb/partial-stab.h

OLD, lines 602-605:

        if (textlow_not_set
            || (CUR_SYMBOL_VALUE < pst->textlow
                && CUR_SYMBOL_VALUE
                != ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT)))
          {
            pst->textlow = CUR_SYMBOL_VALUE;

NEW, lines 602-605:

        if (pst && (textlow_not_set
            || (CUR_SYMBOL_VALUE < pst->textlow
                && CUR_SYMBOL_VALUE
                != ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT))))
          {
            pst->textlow = CUR_SYMBOL_VALUE;

OLD crashes because pst is 0 (and is intended to be 0 I think).

I hope this is enough for you to get this included in the next release.
I hate maintaining patches.  Please let me know if there is anything
else I can do for you about this.  Thank you!

-- Charlie Mills
   cmills@synopsys.com  (work)
   mills@q7.com         (personal)
   (503) 748-2665       (work)


#0  0xc4810 in read_dbx_symtab (objfile=0x1f7120) at partial-stab.h:602
#1  0xc2a94 in dbx_symfile_read (objfile=0x1f7120, mainline=0) at
dbxread.c:631
#2  0xc6974 in elfstab_build_psymtabs (objfile=0x1f7120, mainline=0, 
    staboffset=59705972, stabsize=2080088, stabstroffset=72560872, 
    stabstrsize=59705971) at dbxread.c:2652
#3  0xca24c in elf_symfile_read (objfile=0x1f7120, mainline=0) at
elfread.c:663
#4  0x5384c in syms_from_objfile (objfile=0x1f7120, addrs=0xffbedde8, 
    mainline=1, verbo=0) at symfile.c:807
#5  0x53a74 in symbol_file_add (
    name=0x1edee0 "/u/formal/nightly/synopsys/sparcOS5/fm/bin/fm_gui-g", 
    from_tty=0, addrs=0x0, mainline=1, flags=32) at symfile.c:943
#6  0x53d70 in symbol_file_command (args=0x1d7140 "", from_tty=0)
    at symfile.c:1087
#7  0x91a58 in do_captured_command (data=0xffbee468) at top.c:679
#8  0x919c8 in catch_errors (func=0x91a44 <do_captured_command>, 
    args=0xffbee468, errstring=0x16d438 "", mask=6) at top.c:615
#9  0x91a90 in catch_command_errors (command=0x53b84 <symbol_file_command>, 
    arg=0xffbeeaa7 "/u/formal/nightly/synopsys/sparcOS5/fm/bin/fm_gui-g", 
    from_tty=0, mask=6) at top.c:699
#10 0x34634 in captured_main (data=0xffbee864) at main.c:603
#11 0x919c8 in catch_errors (func=0x33d4c <captured_main>, args=0xffbee7e8, 
    errstring=0x14e880 "", mask=6) at top.c:615
#12 0x348e8 in main (argc=2, argv=0xffbee864) at main.c:761

Re: Simple but crucial bug fix to gdb

[Christopher Faylor]

On Wed, May 30, 2001 at 02:27:45PM -0700, Charlie Mills wrote:
>Dear gdb maintainer,
>
>I would like to submit a very simple patch to gdb.
>I would like if possible to avoid legal issues (avoid having to
>submit a form to our legal department) by simply describing the fix,
>which is a diff of only a few characters, rather than sending you a
>source file.
>
>Bug description:  gdb 4.xx and 5.0 crashes while reading our executable.
>Our executable is the result of linking objects compiled by gcc with
>other objects compiled using SPARCworks CC.  The stack trace is
>appended at the end of this message.
>
>Unfortunately the executable is large and proprietary.
>Although I can't submit a test case, it is very easy to confirm by
>inspecting the code that the patch is correct and the original code
>is incorrect.  The patch is as follows:
>
>File: gdb-5.0/gdb/partial-stab.h
>
>OLD, lines 602-605:
>
>        if (textlow_not_set
>            || (CUR_SYMBOL_VALUE < pst->textlow
>                && CUR_SYMBOL_VALUE
>                != ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT)))
>          {
>            pst->textlow = CUR_SYMBOL_VALUE;
>
>NEW, lines 602-605:
>
>        if (pst && (textlow_not_set
>            || (CUR_SYMBOL_VALUE < pst->textlow
>                && CUR_SYMBOL_VALUE
>                != ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT))))
>          {
>            pst->textlow = CUR_SYMBOL_VALUE;
>
>OLD crashes because pst is 0 (and is intended to be 0 I think).
>
>I hope this is enough for you to get this included in the next release.
>I hate maintaining patches.  Please let me know if there is anything
>else I can do for you about this.  Thank you!

Assuming that I have properly tracked where this is in the current sources,
I think that this change looks reasonable.  There is a similar check for
pst being non-null a few lines up from this point and pst does not get
set in the intervening space.  So, if the previous check is correct, then
this one is obviously needed.

I've included a diff below with some extended context.  If there are no
objections, I'll be happy to check this in.

cgf

Index: partial-stab.h
===================================================================
RCS file: /cvs/uberbaum/gdb/partial-stab.h,v
retrieving revision 1.7
diff -c -2 -0 -p -r1.7 partial-stab.h
*** partial-stab.h	2001/03/06 08:21:11	1.7
--- partial-stab.h	2001/05/30 21:34:59
*************** switch (CUR_SYMBOL_TYPE)
*** 583,626 ****
  #ifdef SOFUN_ADDRESS_MAYBE_MISSING
  	/* Do not fix textlow==0 for .o or NLM files, as 0 is a legit
  	   value for the bottom of the text seg in those cases. */
  	if (pst && textlow_not_set)
  	  {
  	    pst->textlow =
  	      find_stab_function_addr (namestring, pst->filename, objfile);
  	    textlow_not_set = 0;
  	  }
  #endif
  	/* End kludge.  */
  
  	/* Keep track of the start of the last function so we
  	   can handle end of function symbols.  */
  	last_function_start = CUR_SYMBOL_VALUE;
  
  	/* In reordered executables this function may lie outside
  	   the bounds created by N_SO symbols.  If that's the case
  	   use the address of this function as the low bound for
  	   the partial symbol table.  */
! 	if (textlow_not_set
! 	    || (pst && CUR_SYMBOL_VALUE < pst->textlow
! 		&& CUR_SYMBOL_VALUE
! 		!= ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile))))
  	  {
  	    pst->textlow = CUR_SYMBOL_VALUE;
  	    textlow_not_set = 0;
  	  }
  #endif /* DBXREAD_ONLY */
  	add_psymbol_to_list (namestring, p - namestring,
  			     VAR_NAMESPACE, LOC_BLOCK,
  			     &objfile->static_psymbols,
  			     0, CUR_SYMBOL_VALUE,
  			     psymtab_language, objfile);
  	continue;
  
  	/* Global functions were ignored here, but now they
  	   are put into the global psymtab like one would expect.
  	   They're also in the minimal symbol table.  */
        case 'F':
  	CUR_SYMBOL_VALUE += ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
  #ifdef DBXREAD_ONLY
  	/* Kludges for ELF/STABS with Sun ACC */
  	last_function_name = namestring;
--- 583,627 ----
  #ifdef SOFUN_ADDRESS_MAYBE_MISSING
  	/* Do not fix textlow==0 for .o or NLM files, as 0 is a legit
  	   value for the bottom of the text seg in those cases. */
  	if (pst && textlow_not_set)
  	  {
  	    pst->textlow =
  	      find_stab_function_addr (namestring, pst->filename, objfile);
  	    textlow_not_set = 0;
  	  }
  #endif
  	/* End kludge.  */
  
  	/* Keep track of the start of the last function so we
  	   can handle end of function symbols.  */
  	last_function_start = CUR_SYMBOL_VALUE;
  
  	/* In reordered executables this function may lie outside
  	   the bounds created by N_SO symbols.  If that's the case
  	   use the address of this function as the low bound for
  	   the partial symbol table.  */
! 	if (pst
! 	    && (textlow_not_set
! 		|| (pst && CUR_SYMBOL_VALUE < pst->textlow
! 		  && CUR_SYMBOL_VALUE
! 		  != ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile)))))
  	  {
  	    pst->textlow = CUR_SYMBOL_VALUE;
  	    textlow_not_set = 0;
  	  }
  #endif /* DBXREAD_ONLY */
  	add_psymbol_to_list (namestring, p - namestring,
  			     VAR_NAMESPACE, LOC_BLOCK,
  			     &objfile->static_psymbols,
  			     0, CUR_SYMBOL_VALUE,
  			     psymtab_language, objfile);
  	continue;
  
  	/* Global functions were ignored here, but now they
  	   are put into the global psymtab like one would expect.
  	   They're also in the minimal symbol table.  */
        case 'F':
  	CUR_SYMBOL_VALUE += ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
  #ifdef DBXREAD_ONLY
  	/* Kludges for ELF/STABS with Sun ACC */
  	last_function_name = namestring;

Re: Simple but crucial bug fix to gdb

[Fernando Nasser]

Christopher Faylor wrote:
> 
> Assuming that I have properly tracked where this is in the current sources,
> I think that this change looks reasonable.  There is a similar check for
> pst being non-null a few lines up from this point and pst does not get
> set in the intervening space.  So, if the previous check is correct, then
> this one is obviously needed.
> 
> I've included a diff below with some extended context.  If there are no
> objections, I'll be happy to check this in.
> 

I cannot approve the patch because I am not the maintainer of the stabs reader, but (for the reasons you've explained above) this seems to fall clearly into the obvious fix rule.

Fernando



> 
> Index: partial-stab.h
> ===================================================================
> RCS file: /cvs/uberbaum/gdb/partial-stab.h,v
> retrieving revision 1.7
> diff -c -2 -0 -p -r1.7 partial-stab.h
> *** partial-stab.h      2001/03/06 08:21:11     1.7
> --- partial-stab.h      2001/05/30 21:34:59
> *************** switch (CUR_SYMBOL_TYPE)
> *** 583,626 ****
>   #ifdef SOFUN_ADDRESS_MAYBE_MISSING
>         /* Do not fix textlow==0 for .o or NLM files, as 0 is a legit
>            value for the bottom of the text seg in those cases. */
>         if (pst && textlow_not_set)
>           {
>             pst->textlow =
>               find_stab_function_addr (namestring, pst->filename, objfile);
>             textlow_not_set = 0;
>           }
>   #endif
>         /* End kludge.  */
> 
>         /* Keep track of the start of the last function so we
>            can handle end of function symbols.  */
>         last_function_start = CUR_SYMBOL_VALUE;
> 
>         /* In reordered executables this function may lie outside
>            the bounds created by N_SO symbols.  If that's the case
>            use the address of this function as the low bound for
>            the partial symbol table.  */
> !       if (textlow_not_set
> !           || (pst && CUR_SYMBOL_VALUE < pst->textlow
> !               && CUR_SYMBOL_VALUE
> !               != ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile))))
>           {
>             pst->textlow = CUR_SYMBOL_VALUE;
>             textlow_not_set = 0;
>           }
>   #endif /* DBXREAD_ONLY */
>         add_psymbol_to_list (namestring, p - namestring,
>                              VAR_NAMESPACE, LOC_BLOCK,
>                              &objfile->static_psymbols,
>                              0, CUR_SYMBOL_VALUE,
>                              psymtab_language, objfile);
>         continue;
> 
>         /* Global functions were ignored here, but now they
>            are put into the global psymtab like one would expect.
>            They're also in the minimal symbol table.  */
>         case 'F':
>         CUR_SYMBOL_VALUE += ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
>   #ifdef DBXREAD_ONLY
>         /* Kludges for ELF/STABS with Sun ACC */
>         last_function_name = namestring;
> --- 583,627 ----
>   #ifdef SOFUN_ADDRESS_MAYBE_MISSING
>         /* Do not fix textlow==0 for .o or NLM files, as 0 is a legit
>            value for the bottom of the text seg in those cases. */
>         if (pst && textlow_not_set)
>           {
>             pst->textlow =
>               find_stab_function_addr (namestring, pst->filename, objfile);
>             textlow_not_set = 0;
>           }
>   #endif
>         /* End kludge.  */
> 
>         /* Keep track of the start of the last function so we
>            can handle end of function symbols.  */
>         last_function_start = CUR_SYMBOL_VALUE;
> 
>         /* In reordered executables this function may lie outside
>            the bounds created by N_SO symbols.  If that's the case
>            use the address of this function as the low bound for
>            the partial symbol table.  */
> !       if (pst
> !           && (textlow_not_set
> !               || (pst && CUR_SYMBOL_VALUE < pst->textlow
> !                 && CUR_SYMBOL_VALUE
> !                 != ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile)))))
>           {
>             pst->textlow = CUR_SYMBOL_VALUE;
>             textlow_not_set = 0;
>           }
>   #endif /* DBXREAD_ONLY */
>         add_psymbol_to_list (namestring, p - namestring,
>                              VAR_NAMESPACE, LOC_BLOCK,
>                              &objfile->static_psymbols,
>                              0, CUR_SYMBOL_VALUE,
>                              psymtab_language, objfile);
>         continue;
> 
>         /* Global functions were ignored here, but now they
>            are put into the global psymtab like one would expect.
>            They're also in the minimal symbol table.  */
>         case 'F':
>         CUR_SYMBOL_VALUE += ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
>   #ifdef DBXREAD_ONLY
>         /* Kludges for ELF/STABS with Sun ACC */
>         last_function_name = namestring;

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

Re: Simple but crucial bug fix to gdb

[Christopher Faylor]

I have checked in this patch, along with the ChangeLog below.

cgf

2001-05-29  Christopher Faylor  <cgf@redhat.com>

        * partial-stab.h: Consistently guard against pst being NULL.

Re: Simple but crucial bug fix to gdb

[Jim Blandy]

> I would like to submit a very simple patch to gdb.
> I would like if possible to avoid legal issues (avoid having to
> submit a form to our legal department) by simply describing the fix,
> which is a diff of only a few characters, rather than sending you a
> source file.

The size of the text you send doesn't make any difference --- it's the
size of the change that matters.  You could have posted an ordinary
patch without legal troubles.

I find it odd that the stabs reader is seeing a function definition
symbol (a descriptor of 'f') before it's seen anything to set up the
psymtab.  Does your object file really have an N_FUN stab (or
whatever) before the first N_SO stab?

If we just ignore the N_FUN, I'm worried that the pst's textlow will
not be set correctly --- that is, I'm not sure it's safe to just
ignore the N_FUN when pst is zero.

Could you post the result of running `objdump --stabs' on your
executable, or object file, if it's not too large or revealing?

Re: Simple but crucial bug fix to gdb

[Jim Blandy]

Fernando Nasser <fnasser@cygnus.com> writes:
> I cannot approve the patch because I am not the maintainer of the
> stabs reader, but (for the reasons you've explained above) this
> seems to fall clearly into the obvious fix rule.

This is *not* an obvious fix.

The stabs reader has seen a symbol definition, before it has any idea
what compilation unit that symbol belongs to.  Are you *sure* it's
okay to just ignore that symbol definition?  Won't the psymtab's start
and end addresses get set wrong?

Re: Simple but crucial bug fix to gdb

[Christopher Faylor]

On Wed, May 30, 2001 at 10:19:02PM -0500, Jim Blandy wrote:
>
>Fernando Nasser <fnasser@cygnus.com> writes:
>> I cannot approve the patch because I am not the maintainer of the
>> stabs reader, but (for the reasons you've explained above) this
>> seems to fall clearly into the obvious fix rule.
>
>This is *not* an obvious fix.
>
>The stabs reader has seen a symbol definition, before it has any idea
>what compilation unit that symbol belongs to.  Are you *sure* it's
>okay to just ignore that symbol definition?  Won't the psymtab's start
>and end addresses get set wrong?

Oops.  I just checked it in.  I thought it was obvious since the previous
code was checking for pst != NULL, although the code that previously
checked is under a SOFUN_ADDRESS_MAYBE_MISSING conditional.

Is the SOFUN... an extenuating case or is this code:

#ifdef SOFUN_ADDRESS_MAYBE_MISSING
        /* Do not fix textlow==0 for .o or NLM files, as 0 is a legit
           value for the bottom of the text seg in those cases. */
        if (pst && textlow_not_set)
          {
            pst->textlow =
              find_stab_function_addr (namestring, pst->filename, objfile);
            textlow_not_set = 0;
          }
#endif

wrong?

cgf

Re: Simple but crucial bug fix to gdb

[Christopher Faylor]

On Wed, May 30, 2001 at 11:00:55PM -0400, Christopher Faylor wrote:
>I have checked in this patch, along with the ChangeLog below.
>
>cgf
>
>2001-05-29  Christopher Faylor  <cgf@redhat.com>
>
>        * partial-stab.h: Consistently guard against pst being NULL.

Since JimB has reservations about this patch and since it falls far
outside of my very limited realm of gdb expertise, I've reverted this
patch to partial-stab.h.

cgf

Re: Simple but crucial bug fix to gdb

[Daniel Berlin]

Jim Blandy <jimb@zwingli.cygnus.com> writes:

> > I would like to submit a very simple patch to gdb.
> > I would like if possible to avoid legal issues (avoid having to
> > submit a form to our legal department) by simply describing the fix,
> > which is a diff of only a few characters, rather than sending you a
> > source file.
> 
> The size of the text you send doesn't make any difference --- it's the
> size of the change that matters.  You could have posted an ordinary
> patch without legal troubles.
> 
> I find it odd that the stabs reader is seeing a function definition
> symbol (a descriptor of 'f') before it's seen anything to set up the
> psymtab.  Does your object file really have an N_FUN stab (or
> whatever) before the first N_SO stab?

Ha.
You are so funny.
Your joking, right?
This is the same STABS reader keeps psymtabs around so that we can't free them
after conversion to symtabs, at the expense of every other debug info
reader. And requires hacks in buildsym that only the other 8 year old
symbol reader (mdebugread, split off from mipsread) requires?
You are surprised it does something else odd?
Anyway, ranting aside, you are completely correct in this case.
The first symbol is *either* the GCC_COMPILED_FLAG_SYMBOL (followed
directly by an N_SO), or the N_SO.
It says so in read_ofile_symtab, in very explicit terms ("The N_SO
starting this symtab is the first symbol, so we better not check the
symbol before it")

So either all the comments are wrong, or gcc is broken.
> 
> If we just ignore the N_FUN, I'm worried that the pst's textlow will
> not be set correctly --- that is, I'm not sure it's safe to just
> ignore the N_FUN when pst is zero.
> 
> Could you post the result of running `objdump --stabs' on your
> executable, or object file, if it's not too large or revealing?

-- 
"We were in Salino, Utah when we were arrested for not going
through a green light.  We pleaded "maybe".  I asked the judge
if he knew what time it is, he did, and I said, "No further
questions."
"-Steven Wright

Re: Simple but crucial bug fix to gdb

[Fernando Nasser]

Jim Blandy wrote:
> 
> Fernando Nasser <fnasser@cygnus.com> writes:
> > I cannot approve the patch because I am not the maintainer of the
> > stabs reader, but (for the reasons you've explained above) this
> > seems to fall clearly into the obvious fix rule.
> 
> This is *not* an obvious fix.
> 
> The stabs reader has seen a symbol definition, before it has any idea
> what compilation unit that symbol belongs to.  Are you *sure* it's
> okay to just ignore that symbol definition?  Won't the psymtab's start
> and end addresses get set wrong?

Irrelevant to the fact that his compiler may not be doing the right
thing, GDB should not be dumping core.

Furthermore, as Chris said, the previous test already test for 'pst'
being set, so he would not be making it any worse than it was.

Furthermore, it would not affect any one with a more sane compiler.

Furthermore, the condition of _this_ if already test for 'pst' after
'textlow_not_set' is reset (i.e., on every subsequent pass of the loop
after this test succeeds the first time).

So,

original:
        if (textlow_not_set
            || (pst && CUR_SYMBOL_VALUE < pst->textlow
                && CUR_SYMBOL_VALUE
                != ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT
(objfile))))

and

changed:
        if (pst && textlow_not_set
            || (pst && CUR_SYMBOL_VALUE < pst->textlow
                && CUR_SYMBOL_VALUE
                != ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT
(objfile))))

are either as right or as wrong as each other, except that the second
one is more defensive and IMHO more right than the first one, where we
guard from one condition (pst not set) on the second part of the if and
not on the first).


This is an obvious fix!   If the original code there is wrong, it has
been so for a while.  You, as the maintainer, should go there and see
what is wrong (or understand why it does that ;-)  ).  But letting GDB
dump core on someone is just not the right thing to do.

I sincerely expect that you provide us with the "more correct" fix asap.

Regards,
Fernando




------------------------code from the file-----------------------
      case 'f':
        CUR_SYMBOL_VALUE += ANOFFSET (objfile->section_offsets,
SECT_OFF_TEXT (objfile));
#ifdef DBXREAD_ONLY
        /* Kludges for ELF/STABS with Sun ACC */
        last_function_name = namestring;
#ifdef SOFUN_ADDRESS_MAYBE_MISSING
        /* Do not fix textlow==0 for .o or NLM files, as 0 is a legit
           value for the bottom of the text seg in those cases. */
        if (pst && textlow_not_set)
          {
            pst->textlow =
              find_stab_function_addr (namestring, pst->filename,
objfile);
            textlow_not_set = 0;
          }
#endif
        /* End kludge.  */

        /* Keep track of the start of the last function so we
           can handle end of function symbols.  */
        last_function_start = CUR_SYMBOL_VALUE;

        /* In reordered executables this function may lie outside
           the bounds created by N_SO symbols.  If that's the case
           use the address of this function as the low bound for
           the partial symbol table.  */
        if (textlow_not_set
            || (pst && CUR_SYMBOL_VALUE < pst->textlow
                && CUR_SYMBOL_VALUE
                != ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT
(objfile))))
          {
            pst->textlow = CUR_SYMBOL_VALUE;
            textlow_not_set = 0;
          }
#endif /* DBXREAD_ONLY */
        add_psymbol_to_list (namestring, p - namestring,
                             VAR_NAMESPACE, LOC_BLOCK,
                             &objfile->static_psymbols,
                             0, CUR_SYMBOL_VALUE,
                             psymtab_language, objfile);
        continue;

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

Re: Simple but crucial bug fix to gdb

[Jim Blandy]

Fernando Nasser <fnasser@redhat.com> writes:
> Irrelevant to the fact that his compiler may not be doing the right
> thing, GDB should not be dumping core.

That's absolutely true.

> I sincerely expect that you provide us with the "more correct" fix asap.

Please, don't be upset.  I can't provide a more correct fix without
understanding the user's situation more.  I asked him a question in a
message before the one I sent you, and I'm waiting to see what he
says.

Each partial symbol table object has an address range, textlow and
texthigh, which is supposed to enclose all the functions it covers.
If that address range is not set correctly, then GDB may not be able
to find the full symbols for a given text address.

The user has a stabs file which manages to get textlow_not_set cleared
(indicating that pst->textlow has been set), while pst is zero.  This
is very curious --- if the pst is zero, which textlow was it that got
set?

There's something very odd (or, as Daniel would have me say, even
odder than usual) going on here.  Rather than slap a test for null
over the problem and have it disappear, I want to try to understand
what's going on.

If you've got scurvy, you want limes, not spare teeth.

Re: Simple but crucial bug fix to gdb

[Fernando Nasser]

Jim Blandy wrote:
> 
> There's something very odd (or, as Daniel would have me say, even
> odder than usual) going on here.  Rather than slap a test for null
> over the problem and have it disappear, I want to try to understand
> what's going on.
> 

Sure.  I agree with that.

How about adding a warning() (with code to print it only once per object file that has this problem) so it does not go unnoticed?   The test for pst could still be there as defensive programming about crazy compilers (if this is what is causing the problem).

This way we would do both: prevent the core dump without hiding the problem.


P.S.: Unfortunately this code is "included" inside a loop, which makes the warning-once part awkward.  

P.S.2: Being "included" also prevents me to follow things with Source Navigator :-)   It could be made a function with 10 arguments (END_PSYMTAB and START_PSYMTAB would have to be turned into callbacks) instead.  I can do this in a rainy day if you like the idea.




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

Re: Simple but crucial bug fix to gdb

[Christopher Faylor]

On Thu, May 31, 2001 at 03:56:32PM -0500, Jim Blandy wrote:
>
>Fernando Nasser <fnasser@redhat.com> writes:
>> Irrelevant to the fact that his compiler may not be doing the right
>> thing, GDB should not be dumping core.
>
>That's absolutely true.
>
>> I sincerely expect that you provide us with the "more correct" fix asap.
>
>Please, don't be upset.  I can't provide a more correct fix without
>understanding the user's situation more.  I asked him a question in a
>message before the one I sent you, and I'm waiting to see what he
>says.
>
>Each partial symbol table object has an address range, textlow and
>texthigh, which is supposed to enclose all the functions it covers.
>If that address range is not set correctly, then GDB may not be able
>to find the full symbols for a given text address.
>
>The user has a stabs file which manages to get textlow_not_set cleared
>(indicating that pst->textlow has been set), while pst is zero.  This
>is very curious --- if the pst is zero, which textlow was it that got
>set?
>
>There's something very odd (or, as Daniel would have me say, even
>odder than usual) going on here.  Rather than slap a test for null
>over the problem and have it disappear, I want to try to understand
>what's going on.
>
>If you've got scurvy, you want limes, not spare teeth.

Is there some reason why you are not responding to the part of the
analysis which points to pst being guarded against NULL earlier in the
file?  Why is it ok to do this at one point and not ok a few lines
later?  Does SOFUN_ADDRESS_MAYBE_MISSING have some bearing on this
or is the existing code actually broken?

cgf

Re: Simple but crucial bug fix to gdb

[Daniel Berlin]

Christopher Faylor <cgf@redhat.com> writes:

> On Thu, May 31, 2001 at 03:56:32PM -0500, Jim Blandy wrote:
> >
> >Fernando Nasser <fnasser@redhat.com> writes:
> >> Irrelevant to the fact that his compiler may not be doing the right
> >> thing, GDB should not be dumping core.
> >
> >That's absolutely true.
> >
> >> I sincerely expect that you provide us with the "more correct" fix asap.
> >
> >Please, don't be upset.  I can't provide a more correct fix without
> >understanding the user's situation more.  I asked him a question in a
> >message before the one I sent you, and I'm waiting to see what he
> >says.
> >
> >Each partial symbol table object has an address range, textlow and
> >texthigh, which is supposed to enclose all the functions it covers.
> >If that address range is not set correctly, then GDB may not be able
> >to find the full symbols for a given text address.
> >
> >The user has a stabs file which manages to get textlow_not_set cleared
> >(indicating that pst->textlow has been set), while pst is zero.  This
> >is very curious --- if the pst is zero, which textlow was it that got
> >set?
> >
> >There's something very odd (or, as Daniel would have me say, even
> >odder than usual) going on here.  Rather than slap a test for null
> >over the problem and have it disappear, I want to try to understand
> >what's going on.
> >
> >If you've got scurvy, you want limes, not spare teeth.
> 
> Is there some reason why you are not responding to the part of the
> analysis which points to pst being guarded against NULL earlier in the
> file?
>   Why is it ok to do this at one point and not ok a few lines
> later?  Does SOFUN_ADDRESS_MAYBE_MISSING have some bearing on this
> or is the existing code actually broken?

The reason it's guarded against earlier is for the case where you have
a textlow of 0. Like in an object file.

We don't want to change the textlow address if that's really what it
claims the starting address to be.

So if we've started a psymtab, and we think textlow_not_set, and we
see a function, we assume everything is copacetic.

However, we should *never* see a case where pst is NULL, and
textlow_not_set is 1, at the point we see a function.
This would imply either some other code is broken, or the compiler is
producing symbols that appear outside of all the source files, which is
specifically not allowed.  We also won't get the function address
right if this is the case, unless we patch it up later, so we actually
need to completely skip this symbol. The current patch will still add
the psymbol to the list, which is a bad idea, since we *know* the
address is wrong.

Guarding against NULL is also the wrong thing to do here anyway, unless the
compiler really is that broken.  IN which case, you probably actually
need to kludge somewhere *else*, so that we don't run into this case.
Guarding against null is equivalent of ignoring the problem until it
goes away.  It's certainly something that shouldn't be done without
making it clear that something has gone very awry.
> 
> cgf

-- 
"It's a good apartment because they allow pets.  I have a
Shetland pony named Nikkie.  Last summer Nikkie was involved in
a bizarre electrolysis accident.  All her hair was removed
except for her tail.  Now I rent her out to Hare Krishna family
picnics.
"-Steven Wright

Re: Simple but crucial bug fix to gdb

[Frank Ch. Eigler]

Daniel Berlin <dan@cgsoftware.com> writes:

: [...]
: However, we should *never* see a case where pst is NULL, and
: textlow_not_set is 1, at the point we see a function.
: [...]

Would a gdb_assert() to this effect satisfy all sides?

- FChE

Re: Simple but crucial bug fix to gdb

[Fernando Nasser]

"Frank Ch. Eigler" wrote:
> 
> Daniel Berlin <dan@cgsoftware.com> writes:
> 
> : [...]
> : However, we should *never* see a case where pst is NULL, and
> : textlow_not_set is 1, at the point we see a function.
> : [...]
> 
> Would a gdb_assert() to this effect satisfy all sides?
> 

I proposed to Jim a warning(), which is the same principle: do not crash and still do not ignore it. 

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

Re: Simple but crucial bug fix to gdb

[Michael Snyder]

"Frank Ch. Eigler" wrote:
> 
> Daniel Berlin <dan@cgsoftware.com> writes:
> 
> : [...]
> : However, we should *never* see a case where pst is NULL, and
> : textlow_not_set is 1, at the point we see a function.
> : [...]
> 
> Would a gdb_assert() to this effect satisfy all sides?

gdb_assert causes an abort if the conditional fails.
I generally think it's better if the debugger doesn't abort
(unles it's believed to be in an unrecoverable state).

Re: Simple but crucial bug fix to gdb

[Daniel Berlin]

Michael Snyder <msnyder@cygnus.com> writes:

> "Frank Ch. Eigler" wrote:
> > 
> > Daniel Berlin <dan@cgsoftware.com> writes:
> > 
> > : [...]
> > : However, we should *never* see a case where pst is NULL, and
> > : textlow_not_set is 1, at the point we see a function.
> > : [...]
> > 
> > Would a gdb_assert() to this effect satisfy all sides?
> 
> gdb_assert causes an abort if the conditional fails.
> I generally think it's better if the debugger doesn't abort
> (unles it's believed to be in an unrecoverable state).

To be honest, i'd consider it an unrecoverable state.
This is because if the compiler is producing such broken debug info
that we see functions outside of where we should, it's likely your
debug info is so screwed up as to be worthless, and just cause you to
think GDB is broken.

-- 
"If the pen is mightier than the sword, in a duel I'll let you
have the pen!
"-Steven Wright

Re: Simple but crucial bug fix to gdb

[Fernando Nasser]

Daniel Berlin wrote:
> 
> Michael Snyder <msnyder@cygnus.com> writes:
> 
> > "Frank Ch. Eigler" wrote:
> > >
> > > Daniel Berlin <dan@cgsoftware.com> writes:
> > >
> > > : [...]
> > > : However, we should *never* see a case where pst is NULL, and
> > > : textlow_not_set is 1, at the point we see a function.
> > > : [...]
> > >
> > > Would a gdb_assert() to this effect satisfy all sides?
> >
> > gdb_assert causes an abort if the conditional fails.
> > I generally think it's better if the debugger doesn't abort
> > (unles it's believed to be in an unrecoverable state).
> 
> To be honest, i'd consider it an unrecoverable state.
> This is because if the compiler is producing such broken debug info
> that we see functions outside of where we should, it's likely your
> debug info is so screwed up as to be worthless, and just cause you to
> think GDB is broken.
> 

As I understand from Charles Mills message, he can use the debugger if it is allowed to proceed.  So things are not that bad.

Charles, can you confirm that?  Thanks.


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

Re: Simple but crucial bug fix to gdb

[Jim Blandy]

Charlie Mills <cmills@synopsys.com> writes:
> Bug description:  gdb 4.xx and 5.0 crashes while reading our executable.
> Our executable is the result of linking objects compiled by gcc with
> other objects compiled using SPARCworks CC.  The stack trace is
> appended at the end of this message.

I managed to construct an executable that would crash GDB in the same
way yours did (by editing a binary to get stabs of the sort Sun's
compiler produces).  I'm committing the patch below, which prevents
the crashes.  It's essentially the same fix as yours, except that
there are two cases (static and global functions) that need attention,
not one.

I'm concerned that your builds are producing debugging entries for
functions that appear to be outside of any compilation unit.  GDB
doesn't really know what to do with this; at the moment, it mostly
ignores the function's entry.  I wish I could play around with the
situation and figure out what really needs to be done; with the
current fix, I strongly suspect that GDB will be unable to find
debugging information for some functions in some circumstances.  So
that the problem doesn't disappear from view entirely, my patch makes
GDB complain (that's a technical term) when it sees debugging info
like that present in your executable.  Perhaps we'll find another
case, and we'll be able to really fix the bug.

In any case, thanks for the bug report.

2001-06-01  Jim Blandy  <jimb@redhat.com>

	* partial-stab.h: New complaint: function_outside_compilation_unit.
	(case N_FUN: case 'f':, case N_FUN: case 'F':): If pst is zero,
	complain, and don't try to set pst's start address.

Index: gdb/partial-stab.h
===================================================================
RCS file: /cvs/src/src/gdb/partial-stab.h,v
retrieving revision 1.9
diff -c -r1.9 partial-stab.h
*** gdb/partial-stab.h	2001/05/31 03:41:31	1.9
--- gdb/partial-stab.h	2001/06/01 20:24:26
***************
*** 40,45 ****
--- 40,48 ----
  
  switch (CUR_SYMBOL_TYPE)
    {
+     static struct complaint function_outside_compilation_unit = {
+       "function `%s' appears to be defined outside of all compilation units", 0, 0
+     };
      char *p;
      /*
       * Standard, external, non-debugger, symbols
***************
*** 576,581 ****
--- 579,592 ----
  	continue;
  
        case 'f':
+         if (! pst)
+           {
+             int name_len = p - namestring;
+             char *name = xmalloc (name_len + 1);
+             memcpy (name, namestring, name_len);
+             name[name_len] = '\0';
+             complain (&function_outside_compilation_unit, name);
+           }
  	CUR_SYMBOL_VALUE += ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
  #ifdef DBXREAD_ONLY
  	/* Kludges for ELF/STABS with Sun ACC */
***************
*** 600,609 ****
  	   the bounds created by N_SO symbols.  If that's the case
  	   use the address of this function as the low bound for
  	   the partial symbol table.  */
! 	if (textlow_not_set
! 	    || (pst && CUR_SYMBOL_VALUE < pst->textlow
! 		&& CUR_SYMBOL_VALUE
! 		!= ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile))))
  	  {
  	    pst->textlow = CUR_SYMBOL_VALUE;
  	    textlow_not_set = 0;
--- 611,622 ----
  	   the bounds created by N_SO symbols.  If that's the case
  	   use the address of this function as the low bound for
  	   the partial symbol table.  */
! 	if (pst
!             && (textlow_not_set
!                 || (CUR_SYMBOL_VALUE < pst->textlow
!                     && (CUR_SYMBOL_VALUE
!                         != ANOFFSET (objfile->section_offsets,
!                                      SECT_OFF_TEXT (objfile))))))
  	  {
  	    pst->textlow = CUR_SYMBOL_VALUE;
  	    textlow_not_set = 0;
***************
*** 620,625 ****
--- 633,646 ----
  	   are put into the global psymtab like one would expect.
  	   They're also in the minimal symbol table.  */
        case 'F':
+         if (! pst)
+           {
+             int name_len = p - namestring;
+             char *name = xmalloc (name_len + 1);
+             memcpy (name, namestring, name_len);
+             name[name_len] = '\0';
+             complain (&function_outside_compilation_unit, name);
+           }
  	CUR_SYMBOL_VALUE += ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
  #ifdef DBXREAD_ONLY
  	/* Kludges for ELF/STABS with Sun ACC */
***************
*** 647,656 ****
  	   the bounds created by N_SO symbols.  If that's the case
  	   use the address of this function as the low bound for
  	   the partial symbol table.  */
! 	if (textlow_not_set
! 	    || (pst && CUR_SYMBOL_VALUE < pst->textlow
! 		&& CUR_SYMBOL_VALUE
! 		!= ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile))))
  	  {
  	    pst->textlow = CUR_SYMBOL_VALUE;
  	    textlow_not_set = 0;
--- 668,679 ----
  	   the bounds created by N_SO symbols.  If that's the case
  	   use the address of this function as the low bound for
  	   the partial symbol table.  */
! 	if (pst
!             && (textlow_not_set
!                 || (CUR_SYMBOL_VALUE < pst->textlow
!                     && (CUR_SYMBOL_VALUE
!                         != ANOFFSET (objfile->section_offsets,
!                                      SECT_OFF_TEXT (objfile))))))
  	  {
  	    pst->textlow = CUR_SYMBOL_VALUE;
  	    textlow_not_set = 0;

Re: Simple but crucial bug fix to gdb

[Fernando Nasser]

Thank you Jim.

Fernando


Jim Blandy wrote:
> 
> Charlie Mills <cmills@synopsys.com> writes:
> > Bug description:  gdb 4.xx and 5.0 crashes while reading our executable.
> > Our executable is the result of linking objects compiled by gcc with
> > other objects compiled using SPARCworks CC.  The stack trace is
> > appended at the end of this message.
> 
> I managed to construct an executable that would crash GDB in the same
> way yours did (by editing a binary to get stabs of the sort Sun's
> compiler produces).  I'm committing the patch below, which prevents
> the crashes.  It's essentially the same fix as yours, except that
> there are two cases (static and global functions) that need attention,
> not one.
> 
> I'm concerned that your builds are producing debugging entries for
> functions that appear to be outside of any compilation unit.  GDB
> doesn't really know what to do with this; at the moment, it mostly
> ignores the function's entry.  I wish I could play around with the
> situation and figure out what really needs to be done; with the
> current fix, I strongly suspect that GDB will be unable to find
> debugging information for some functions in some circumstances.  So
> that the problem doesn't disappear from view entirely, my patch makes
> GDB complain (that's a technical term) when it sees debugging info
> like that present in your executable.  Perhaps we'll find another
> case, and we'll be able to really fix the bug.
> 
> In any case, thanks for the bug report.
> 
> 2001-06-01  Jim Blandy  <jimb@redhat.com>
> 
>         * partial-stab.h: New complaint: function_outside_compilation_unit.
>         (case N_FUN: case 'f':, case N_FUN: case 'F':): If pst is zero,
>         complain, and don't try to set pst's start address.
> 
> Index: gdb/partial-stab.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/partial-stab.h,v
> retrieving revision 1.9
> diff -c -r1.9 partial-stab.h
> *** gdb/partial-stab.h  2001/05/31 03:41:31     1.9
> --- gdb/partial-stab.h  2001/06/01 20:24:26
> ***************
> *** 40,45 ****
> --- 40,48 ----
> 
>   switch (CUR_SYMBOL_TYPE)
>     {
> +     static struct complaint function_outside_compilation_unit = {
> +       "function `%s' appears to be defined outside of all compilation units", 0, 0
> +     };
>       char *p;
>       /*
>        * Standard, external, non-debugger, symbols
> ***************
> *** 576,581 ****
> --- 579,592 ----
>         continue;
> 
>         case 'f':
> +         if (! pst)
> +           {
> +             int name_len = p - namestring;
> +             char *name = xmalloc (name_len + 1);
> +             memcpy (name, namestring, name_len);
> +             name[name_len] = '\0';
> +             complain (&function_outside_compilation_unit, name);
> +           }
>         CUR_SYMBOL_VALUE += ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
>   #ifdef DBXREAD_ONLY
>         /* Kludges for ELF/STABS with Sun ACC */
> ***************
> *** 600,609 ****
>            the bounds created by N_SO symbols.  If that's the case
>            use the address of this function as the low bound for
>            the partial symbol table.  */
> !       if (textlow_not_set
> !           || (pst && CUR_SYMBOL_VALUE < pst->textlow
> !               && CUR_SYMBOL_VALUE
> !               != ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile))))
>           {
>             pst->textlow = CUR_SYMBOL_VALUE;
>             textlow_not_set = 0;
> --- 611,622 ----
>            the bounds created by N_SO symbols.  If that's the case
>            use the address of this function as the low bound for
>            the partial symbol table.  */
> !       if (pst
> !             && (textlow_not_set
> !                 || (CUR_SYMBOL_VALUE < pst->textlow
> !                     && (CUR_SYMBOL_VALUE
> !                         != ANOFFSET (objfile->section_offsets,
> !                                      SECT_OFF_TEXT (objfile))))))
>           {
>             pst->textlow = CUR_SYMBOL_VALUE;
>             textlow_not_set = 0;
> ***************
> *** 620,625 ****
> --- 633,646 ----
>            are put into the global psymtab like one would expect.
>            They're also in the minimal symbol table.  */
>         case 'F':
> +         if (! pst)
> +           {
> +             int name_len = p - namestring;
> +             char *name = xmalloc (name_len + 1);
> +             memcpy (name, namestring, name_len);
> +             name[name_len] = '\0';
> +             complain (&function_outside_compilation_unit, name);
> +           }
>         CUR_SYMBOL_VALUE += ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
>   #ifdef DBXREAD_ONLY
>         /* Kludges for ELF/STABS with Sun ACC */
> ***************
> *** 647,656 ****
>            the bounds created by N_SO symbols.  If that's the case
>            use the address of this function as the low bound for
>            the partial symbol table.  */
> !       if (textlow_not_set
> !           || (pst && CUR_SYMBOL_VALUE < pst->textlow
> !               && CUR_SYMBOL_VALUE
> !               != ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile))))
>           {
>             pst->textlow = CUR_SYMBOL_VALUE;
>             textlow_not_set = 0;
> --- 668,679 ----
>            the bounds created by N_SO symbols.  If that's the case
>            use the address of this function as the low bound for
>            the partial symbol table.  */
> !       if (pst
> !             && (textlow_not_set
> !                 || (CUR_SYMBOL_VALUE < pst->textlow
> !                     && (CUR_SYMBOL_VALUE
> !                         != ANOFFSET (objfile->section_offsets,
> !                                      SECT_OFF_TEXT (objfile))))))
>           {
>             pst->textlow = CUR_SYMBOL_VALUE;
>             textlow_not_set = 0;

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

Re: Simple but crucial bug fix to gdb

[Charlie Mills]

At 02:25 PM 6/1/01 -0400, Fernando Nasser wrote:
>Daniel Berlin wrote:
>> 
>> Michael Snyder <msnyder@cygnus.com> writes:
>> 
>> > "Frank Ch. Eigler" wrote:
>> > >
>> > > Daniel Berlin <dan@cgsoftware.com> writes:
>> > >
>> > > : [...]
>> > > : However, we should *never* see a case where pst is NULL, and
>> > > : textlow_not_set is 1, at the point we see a function.
>> > > : [...]
>> > >
>> > > Would a gdb_assert() to this effect satisfy all sides?
>> >
>> > gdb_assert causes an abort if the conditional fails.
>> > I generally think it's better if the debugger doesn't abort
>> > (unles it's believed to be in an unrecoverable state).
>> 
>> To be honest, i'd consider it an unrecoverable state.
>> This is because if the compiler is producing such broken debug info
>> that we see functions outside of where we should, it's likely your
>> debug info is so screwed up as to be worthless, and just cause you to
>> think GDB is broken.
>> 
>
>As I understand from Charles Mills message, he can use the debugger if it
is allowed to proceed.  So things are not that bad.
>
>Charles, can you confirm that?  Thanks.
>
>
>-- 
>Fernando Nasser
>Red Hat - Toronto                       E-Mail:  fnasser@redhat.com
>2323 Yonge Street, Suite #300
>Toronto, Ontario   M4P 2C9
>
>

Well, yes and no!  I can and do use gdb with my simple
patch, it's certainly a lot better than a crash,
but it is true that gdb then cannot find line numbers in
*some* of my functions, just as Jim Blandy surmises, so I am
still going to have to figure out how to fix my executable to
be more friendly to gdb.

You guys are excellent!  Thanks.

-- Charlie Mills
   cmills@synopsys.com  ..work
   mills@q7.com         ..personal
   (503)748-2665        ..at work today

Re: Simple but crucial bug fix to gdb

[Daniel Berlin]

Jim Blandy <jimb@zwingli.cygnus.com> writes:

> Charlie Mills <cmills@synopsys.com> writes:
> > Bug description:  gdb 4.xx and 5.0 crashes while reading our executable.
> > Our executable is the result of linking objects compiled by gcc with
> > other objects compiled using SPARCworks CC.  The stack trace is
> > appended at the end of this message.
> 
> I managed to construct an executable that would crash GDB in the same
> way yours did (by editing a binary to get stabs of the sort Sun's
> compiler produces).  I'm committing the patch below, which prevents
> the crashes.  It's essentially the same fix as yours, except that
> there are two cases (static and global functions) that need attention,
> not one.
> 
> I'm concerned that your builds are producing debugging entries for
> functions that appear to be outside of any compilation unit.  GDB
> doesn't really know what to do with this; at the moment, it mostly
> ignores the function's entry.  I wish I could play around with the
> situation and figure out what really needs to be done; with the
> current fix, I strongly suspect that GDB will be unable to find
> debugging information for some functions in some circumstances.  So
> that the problem doesn't disappear from view entirely, my patch makes
> GDB complain (that's a technical term) when it sees debugging info
> like that present in your executable.  Perhaps we'll find another
> case, and we'll be able to really fix the bug.

One nit in the patch below:

> 
> In any case, thanks for the bug report.
> 
> 2001-06-01  Jim Blandy  <jimb@redhat.com>
> 
> 	* partial-stab.h: New complaint: function_outside_compilation_unit.
> 	(case N_FUN: case 'f':, case N_FUN: case 'F':): If pst is zero,
> 	complain, and don't try to set pst's start address.
> 
> Index: gdb/partial-stab.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/partial-stab.h,v
> retrieving revision 1.9
> diff -c -r1.9 partial-stab.h
> *** gdb/partial-stab.h	2001/05/31 03:41:31	1.9
> --- gdb/partial-stab.h	2001/06/01 20:24:26
> ***************
> *** 40,45 ****
> --- 40,48 ----
>   
>   switch (CUR_SYMBOL_TYPE)
>     {
> +     static struct complaint function_outside_compilation_unit = {
> +       "function `%s' appears to be defined outside of all compilation units", 0, 0
> +     };
>       char *p;
>       /*
>        * Standard, external, non-debugger, symbols
> ***************
> *** 576,581 ****
> --- 579,592 ----
>   	continue;
>   
>         case 'f':
> +         if (! pst)
> +           {
> +             int name_len = p - namestring;
> +             char *name = xmalloc (name_len + 1);
> +             memcpy (name, namestring, name_len);
> +             name[name_len] = '\0';
> +             complain (&function_outside_compilation_unit, name);

Err, isn't this a memory leak?

You never free the name after complaining.

Same below

-- 
"Why is the alphabet in that order?  Is it because of that song?
The guy who wrote that song wrote everything.
"-Steven Wright

Re: Simple but crucial bug fix to gdb

[Jim Blandy]

Daniel Berlin <dan@cgsoftware.com> writes:
> >         case 'f':
> > +         if (! pst)
> > +           {
> > +             int name_len = p - namestring;
> > +             char *name = xmalloc (name_len + 1);
> > +             memcpy (name, namestring, name_len);
> > +             name[name_len] = '\0';
> > +             complain (&function_outside_compilation_unit, name);
> 
> Err, isn't this a memory leak?
> 
> You never free the name after complaining.
> 
> Same below

Arf!  Thanks!

Re: Simple but crucial bug fix to gdb

[Jim Blandy]

Daniel Berlin <dan@cgsoftware.com> writes:
> Err, isn't this a memory leak?
> 
> You never free the name after complaining.
> 
> Same below

2001-06-01  Jim Blandy  <jimb@redhat.com>

	* partial-stab.h (case N_FUN: case 'f':, case N_FUN: case 'F':)
	Fix memory leak.

Index: partial-stab.h
===================================================================
RCS file: /cvs/src/src/gdb/partial-stab.h,v
retrieving revision 1.10
diff -c -r1.10 partial-stab.h
*** partial-stab.h	2001/06/01 20:37:11	1.10
--- partial-stab.h	2001/06/01 21:16:36
***************
*** 586,591 ****
--- 586,592 ----
              memcpy (name, namestring, name_len);
              name[name_len] = '\0';
              complain (&function_outside_compilation_unit, name);
+             xfree (name);
            }
  	CUR_SYMBOL_VALUE += ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
  #ifdef DBXREAD_ONLY
***************
*** 640,645 ****
--- 641,647 ----
              memcpy (name, namestring, name_len);
              name[name_len] = '\0';
              complain (&function_outside_compilation_unit, name);
+             xfree (name);
            }
  	CUR_SYMBOL_VALUE += ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
  #ifdef DBXREAD_ONLY

Re: Simple but crucial bug fix to gdb

[Andrew Cagney]

> Daniel Berlin <dan@cgsoftware.com> writes:
> 
> : [...]
> : However, we should *never* see a case where pst is NULL, and
> : textlow_not_set is 1, at the point we see a function.
> : [...]
> 
> Would a gdb_assert() to this effect satisfy all sides?
> 
> - FChE


Yes.

gdb_assert() and internal_error() are for reporting problems internal to 
gdb.  EG: a check for a NULL reference caused by GDB failing to 
detect/handle bad input.  Both of these functions force GDB to abandon 
the current command.

warning() and error() should only be used when the problem is caused by 
an external interface/input _and_ GDB correctly recovered.  EG: a 
warning() to notify the user that a file is corrupt but GDB has managed 
to recover from the problem.  You migt even end up with:

	if (recovery case 1)
	  warning (...); recover;
	else if (recovery case 2)
	  warning (...); recover;
	else
	  internal_error (oops, stuffed);

The difference is subtle but important.  I'd only think about replacing 
gdb_assert() / internal_error() with warning() / error() when it is 
clear that the problem isn't internal to GDB and that GDB is correctly 
recovering from it.

	Andrew