This is the mail archive of the
insight@sources.redhat.com
mailing list for the Insight project.
Re: [rfa] Remove a silly looking symtab sort.
- To: Elena Zannoni <ezannoni at cygnus dot com>
- Subject: Re: [rfa] Remove a silly looking symtab sort.
- From: Daniel Jacobowitz <drow at mvista dot com>
- Date: Mon, 5 Nov 2001 18:30:16 -0500
- Cc: pes at regent dot e-technik dot tu-muenchen dot de, gdb-patches at sources dot redhat dot com,insight at sources dot redhat dot com
- References: <3BE6DCE4.6550BBBD@cygnus.com> <3BE7105A.195AF167@cygnus.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