Re: Simple but crucial bug fix to gdb

[Fernando Nasser <fnasser@redhat.com>]

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