Re: [rfa] Symbol hashing (for the last time?)

Daniel Jacobowitz
2002-07-11T20:47:52Z

On Thu, Jul 11, 2002 at 04:00:02PM -0400, Elena Zannoni wrote:
> Daniel Jacobowitz writes:
> > Here's a patch from last October, dusted off and merged to the current
> > sources. The only substantial changes were some fixes for ada-lang.c,
> > merged after I wrote the original patch. I've verified no regressions
> > on i386-linux for GCC (2.95,3.0.4,3.1)/(stabs,dwarf2).
> >
> > This converts the normal symbol table lookups into hash tables. A few
> > sorts of symbol tables aren't hashed: those produced by mdebugread.c
> > and dstread.c, because they build symbol tables in lots of ad-hoc code,
> > and symbol tables which are actually the arguments to a function
> > (because order matters, or at least comments suggest so).
>
> Would you mind adding the paragraph above, to a comment in the block
> structure, maybe above the hashtable flag?

Sure.

> > A next step
> > will be to convert mdebugread.c, delete dstread.c (it's marked for an
> > upcoming obsoletion, isn't it?), and then delete all the complicated
> > binary search code since the only remaining unhashed symtabs will be
> > argument lists, which are small.
> >
>
> how about os9kread? Anything needs to be done there?

Nope (though that one's scheduled to go away too, I think?). Any
reader which uses finish_block is fine.

> > (ALL_BLOCK_SYMBOLS): New macro.
>
> This is not really true. The macro was there already.

Yup, just noticed that. Thanks. I added it in an earlier patch and
never updated my changelog.

> > (BLOCK_SHOULD_SORT): Do not sort hashed blocks.
> > (struct symbol): Add `hash_next' pointer.
> > * symtab.c (lookup_block_symbol): Search using the hash table when
> > possible.
> > (find_pc_sect_symtab): Use ALL_BLOCK_SYMBOLS.
> > (search_symbols, find_addr_symbol): Likewise.
> >
> > * dstread.c (process_dst_block): Clear hashtable bit for new block.
> > (read_dst_symtab): Likewise.
> > * jv-lang.c (get_java_class_symtab): Likewise.
> > * mdebugread.c: Include "gdb_assert.h".
> > (shrink_block): Assert that the block being modified is not hashed.
> > * coffread.c (patch_opaque_types): Use ALL_BLOCK_SYMBOLS.
> > * symmisc.c (free_symtab_block): Walk the hash table when freeing
> > symbols.
> > (dump_symtab): Recognize hashed blocks.
> > * printcmd.c (print_frame_args): Assert that function blocks do not
> > have hashed symbol tables.
> > * ada-lang.c (symtab_for_sym): Use ALL_BLOCK_SYMBOLS.
> > (fill_in_ada_prototype, debug_print_block): Likewise.
> > (ada_add_block_symbols): Use ALL_BLOCK_SYMBOLS. Handle hash tables.
> >
>
> I now wonder if it would be better to define another 'for' macro for this:
> for (sym = BLOCK_BUCKET (block, hash_index); sym; sym = sym->hash_next)
> it seems to reoccur a few times. Even though it's not that important.

I think I'll avoid introducing any more of those if I possibly can....

> I don't like too much the use of the hardcoded '5' maybe define a variable?

I thought about this for a little bit and settled on:

/* Macro used to set the size of a hashtable for N symbols. */
#define BLOCK_HASHTABLE_SIZE(n) ((N)/5 + 1)

I don't really see the point in making it user tunable; if we ever find
a case that this one is bad for, I'll gladly change my mind, though.

> Otherwise it's ok.
> (modulus the problem you found).

Thanks! Committed.