This is the mail archive of the insight@sources.redhat.com mailing list for the Insight project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

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



Daniel Jacobowitz wrote:
> 
> 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.
> 

Looking at the output of 'info function' they are sorted
alphabetically per compilation unit. Well, that maybe depends on the
particular executable I have used. But yes they are locally sorted.


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

Ah, yes. GLOBAL_BLOCK and STATIC_BLOCK only, no function blocks.

I am not sure about the reason for the 40 limit. Wonder what would
happen if we got rid of that check. It predates our internal cvs repo 
(i.e. it has been there from before 1991).

I think that the comments from Peter Schauer indicated that the REGISTER 
should come first. 

OK, I took out the stabs manual...  It says that if a parameter is
passed on the stack, then there is only one stab symbol for it, but if
it is passed in a register there are 2 symbols, like in your
example. One is the 'p' stab to indicate that it is a parameter, the
second is an 'r' stab to indicate it is in a register.  Quoting:
"Debuggers use the second one to find the value, and the first one to
know that it is an argument".

Arguments stored as local variables by the compiler are treated
similarly, with a 'p' symbol and an 'r' symbol.

The manual also says that the stabs for the parameters are in the
order in which the debugger should print them. Hence the 'don't sort'
requirement.

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


Yes.

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

I don't want to step on anybody's toes. (CC-ing Keith...)

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


Yep!

Elena

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]