Re: [rfa] Remove a silly looking symtab sort.

[Daniel Jacobowitz <drow@mvista.com>]

On Mon, Nov 05, 2001 at 05:19:06PM -0500, Elena Zannoni wrote:
> 
> 
> Daniel Jacobowitz wrote:
> > 
> > The function search_symbols () does not seem to be used anywhere where
> > the
> > order of the returned functions matters, and the algorithm it searches
> > with
> > does not rely on any sorting.  The same thing seems to be true for
> > gdb_listfuncs, but I haven't investigated as thoroughly.
> > 
> > Both of these functions have a blurb like:
> >   if (!BLOCK_SHOULD_SORT (b))
> >     sort_block_syms (b);
> > 
> 
> Daniel, I would prefer for the output of these functions to remain
> sorted.
> >From the gdb side, this produces sorted 'info function', 'info var', 
> 'info types' output, and from the gdbtk side, it builds the contents 
> of the function combobox at the bottom of the source window.

This makes sense.  It's not terribly well sorted to begin with, though,
which is why I didn't sort them in the first version of this patch:

ALL_SYMTABS (objfile, s)
 ... 
   for (i = GLOBAL_BLOCK; i <= STATIC_BLOCK; i++)
     ...
          if (!BLOCK_SHOULD_SORT (b))
            sort_block_syms (b);

I.E.  they're only locally sorted.  I could be misinterpreting what is
done with them though.

>     /* The symbols.  If some of them are arguments, then they must be
>        in the order in which we would like to print them.  */
> 
> and the comment for BLOCK_SHOULD_SORT:
> 
> /* Nonzero if symbols of block BL should be sorted alphabetically.
>    Don't sort a block which corresponds to a function.  If we did the
>    sorting would have to preserve the order of the symbols for the
>    arguments.  */
> 
> 
> If this is something necessary, wouldn't the hashing break it?
> I am not sure what would be screwed up based on the order of the
> arguments, though.

> I agree that sorting something that was hashed makes no sense, but we
> are
> one step before that still. I would like to figure out why that change
> by
> Peter was necessary. I think it has to do with the fact that stabs emits
> two symbols for each parameter (I don't have the stabs manual handy 
> at the moment).
> 

This is what confuses me.  I couldn't find where the symtab for a
function block would be used in any order-sensitive way.  Reading the
code in the attached patch reminds me, though.  Here's an example of
what I think this is supposed to address:

int
foo (int a, int b)
{
  return a / b;
}

becomes:

        .align 4
.stabs "foo:F(0,1)",36,0,0,foo
.stabs "a:p(0,1)",160,0,0,8
.stabs "b:p(0,1)",160,0,0,12
.globl foo
        .type    foo,@function
foo:
        pushl %ebp
        movl %esp,%ebp
.stabn 68,0,3,.LM1-foo
.LM1:   
        movl 8(%ebp),%eax
.stabn 68,0,4,.LM2-foo
.LM2:   
        cltd
        idivl 12(%ebp)
        leave
        ret
.Lfe1:  
        .size    foo,.Lfe1-foo
.stabs "a:r(0,1)",64,0,0,0


We read in the symtab and see this (s->blockvector->block[3] is the
block for function foo):

(top-gdb) p *s->blockvector->block[3].sym[0]
$16 = {ginfo = {name = 0x82328c8 "a", value = {ivalue = 8, block = 0x8,
      bytes = 0x8 <Address 0x8 out of bounds>, address = 8, chain = 0x8}, language_specific = {
      cplus_specific = {demangled_name = 0x0}, chill_specific = {demangled_name = 0x0}},
    language = language_c, section = 0, bfd_section = 0x0}, type = 0x8232ae8,
  namespace = VAR_NAMESPACE, aclass = LOC_ARG, line = 0, aux_value = {basereg = 0}, aliases = 0x0,
  ranges = 0x0}
(top-gdb) p *s->blockvector->block[3].sym[1]
$17 = {ginfo = {name = 0x82328fc "b", value = {ivalue = 12, block = 0xc,
      bytes = 0xc <Address 0xc out of bounds>, address = 12, chain = 0xc}, language_specific = {
      cplus_specific = {demangled_name = 0x0}, chill_specific = {demangled_name = 0x0}},
    language = language_c, section = 0, bfd_section = 0x0}, type = 0x8232ae8,
  namespace = VAR_NAMESPACE, aclass = LOC_ARG, line = 0, aux_value = {basereg = 0}, aliases = 0x0,
  ranges = 0x0}
(top-gdb) p *s->blockvector->block[3].sym[2]
$18 = {ginfo = {name = 0x8232930 "a", value = {ivalue = 0, block = 0x0, bytes = 0x0, address = 0,
      chain = 0x0}, language_specific = {cplus_specific = {demangled_name = 0x0}, chill_specific = {
        demangled_name = 0x0}}, language = language_c, section = 0, bfd_section = 0x0},
  type = 0x8232ae8, namespace = VAR_NAMESPACE, aclass = LOC_REGISTER, line = 0, aux_value = {
    basereg = 0}, aliases = 0x0, ranges = 0x0}

Note LOC_ARG and LOC_REGISTER in those.  We want to look it up in the
context of the function and find that it is an argument, not a local.



... aha.  I see why the sort is OK now.  This makes a lot more sense. 
The for loop is on the range [GLOBAL_BLOCK, STATIC_BLOCK] - i.e. it
will never hit a function block.  So it's only interested in the <= 40
check, which seems to be some sort of optimization.

So I feel safe changing this code to not sort the blocks, and then sort
the results afterwards, using compare_symbol.

> > Is this patch OK to commit?  If someone disagrees with my assessment on
> > the
> > need for the results of these functions to be sorted, I can add code to
> > sort
> > them after searching.
> > 
> 
> In theory it would be ok because the symbols shouldn't be sorted at 
> this stage. So we should sort the results instead.
> If you can write something to sort the results we can get rid of these
> sorts. Well, the one in symtab.c, the gdbtk one is not for me to
> approve.

I'd be willing to commit the one to gdbtk as obvious, esp. since no one
commented on it; the code is essentially a copy of the one in symtab.c.

> But before we go ahead with the hashing I want to understand the need
> for
> that condition on sorting. Have you tried the hashing on stabs?

Yup.

So, OK if I sort the results afterwards?

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