On Thu, Feb 25, 2016 at 03:05:21PM +0100, Martin Pieuchot wrote:
> I'd like to be able to iterate over all the kernel symbols. I could
> do like some of the functions below and abuse ``db_last_symtab'' but
> we since only have a single symbol table there's no point in keeping
> a complex interface.
>
> Ok?
comment inline.
-ml
>
> Index: ddb/db_hangman.c
> ===================================================================
> RCS file: /cvs/src/sys/ddb/db_hangman.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 db_hangman.c
> --- ddb/db_hangman.c 27 Jan 2016 10:37:12 -0000 1.33
> +++ ddb/db_hangman.c 25 Feb 2016 14:01:39 -0000
> @@ -88,20 +88,15 @@ db_hang_forall(db_symtab_t *stab, db_sym
> static __inline char *
> db_randomsym(size_t *lenp)
> {
> - extern db_symtab_t db_symtabs[];
> - db_symtab_t *stab;
> - int nsymtabs, nsyms;
> + extern db_symtab_t db_symtab;
> + db_symtab_t *stab = &db_symtab;
> + int nsyms;
> char *p, *q;
> struct db_hang_forall_arg dfa;
>
> - for (nsymtabs = 0; db_symtabs[nsymtabs].name != NULL; nsymtabs++)
> - ;
> -
> - if (nsymtabs == 0)
> + if (stab->start == 0)
> return (NULL);
>
> - stab = &db_symtabs[arc4random_uniform(nsymtabs)];
> -
> dfa.cnt = 0;
> db_elf_sym_forall(stab, db_hang_forall, &dfa);
> nsyms = -dfa.cnt;
> @@ -114,9 +109,9 @@ db_randomsym(size_t *lenp)
>
> q = db_qualify(dfa.sym, stab->name);
>
> - /* don't show symtab name if there are less than 3 of 'em */
> - if (nsymtabs < 3)
> - while (*q++ != ':');
> + /* don't show symtab name */
> + while (*q++ != ':')
> + ;
>
> /* strlen(q) && ignoring underscores and colons */
> for ((*lenp) = 0, p = q; *p; p++)
> Index: ddb/db_sym.c
> ===================================================================
> RCS file: /cvs/src/sys/ddb/db_sym.c,v
> retrieving revision 1.44
> diff -u -p -r1.44 db_sym.c
> --- ddb/db_sym.c 12 Feb 2016 10:58:41 -0000 1.44
> +++ ddb/db_sym.c 25 Feb 2016 14:01:39 -0000
> @@ -48,9 +48,7 @@
> #define MAXNOSYMTABS MAXLKMS+1 /* Room for kernel + LKM's */
> #endif
>
> -db_symtab_t db_symtabs[MAXNOSYMTABS] = {{0,},};
> -
> -db_symtab_t *db_last_symtab;
> +db_symtab_t db_symtab;
>
> extern char end[];
>
> @@ -102,23 +100,12 @@ ddb_init(void)
> int
> db_add_symbol_table(char *start, char *end, const char *name, char *ref)
> {
> - int slot;
> -
> - for (slot = 0; slot < MAXNOSYMTABS; slot++) {
> - if (db_symtabs[slot].name == NULL)
> - break;
> - }
> - if (slot >= MAXNOSYMTABS) {
> - db_printf("No slots left for %s symbol table", name);
> - return(-1);
> - }
> -
> - db_symtabs[slot].start = start;
> - db_symtabs[slot].end = end;
> - db_symtabs[slot].name = name;
> - db_symtabs[slot].private = ref;
> + db_symtab.start = start;
> + db_symtab.end = end;
> + db_symtab.name = name;
> + db_symtab.private = ref;
>
> - return(slot);
> + return 0;
> }
>
> /*
> @@ -127,22 +114,10 @@ db_add_symbol_table(char *start, char *e
> void
> db_del_symbol_table(char *name)
> {
> - int slot;
> -
> - for (slot = 0; slot < MAXNOSYMTABS; slot++) {
> - if (db_symtabs[slot].name &&
> - ! strcmp(db_symtabs[slot].name, name))
> - break;
> - }
> - if (slot >= MAXNOSYMTABS) {
> - db_printf("Unable to find symbol table slot for %s.", name);
> - return;
> - }
> -
> - db_symtabs[slot].start = 0;
> - db_symtabs[slot].end = 0;
> - db_symtabs[slot].name = 0;
> - db_symtabs[slot].private = 0;
> + db_symtab.start = 0;
> + db_symtab.end = 0;
> + db_symtab.name = 0;
> + db_symtab.private = 0;
> }
>
> /*
> @@ -194,54 +169,11 @@ db_value_of_name(char *name, db_expr_t *
>
> /*
> * Lookup a symbol.
> - * If the symbol has a qualifier (e.g., ux:vm_map),
> - * then only the specified symbol table will be searched;
> - * otherwise, all symbol tables will be searched.
> */
> db_sym_t
> db_lookup(char *symstr)
> {
> - db_sym_t sp;
> - int i;
> - int symtab_start = 0;
> - int symtab_end = MAXNOSYMTABS;
> - char *cp;
> -
> - /*
> - * Look for, remove, and remember any symbol table specifier.
> - */
> - for (cp = symstr; *cp; cp++) {
> - if (*cp == ':') {
> - *cp = '\0';
> - for (i = 0; i < MAXNOSYMTABS; i++) {
> - if (db_symtabs[i].name &&
> - ! strcmp(symstr, db_symtabs[i].name)) {
> - symtab_start = i;
> - symtab_end = i + 1;
> - break;
> - }
> - }
> - *cp = ':';
> - if (i == MAXNOSYMTABS) {
> - db_error("invalid symbol table name");
> - /*NOTREACHED*/
> - }
> - symstr = cp+1;
> - }
> - }
> -
> - /*
> - * Look in the specified set of symbol tables.
> - * Return on first match.
> - */
> - for (i = symtab_start; i < symtab_end; i++) {
> - if (db_symtabs[i].name &&
> - (sp = db_elf_sym_lookup(&db_symtabs[i], symstr))) {
> - db_last_symtab = &db_symtabs[i];
> - return sp;
> - }
> - }
> - return 0;
> + return db_elf_sym_lookup(&db_symtab, symstr);
> }
>From what I can tell, db_lookup is only called one time in the tree.
Rather than replace db_lookup with a 1-line "return" that just calls another
function, should we replace the single call to db_lookup with a call to
db_elf_sym_lookup and just remove db_lookup entirely? If you go this
route, please remove the reference in db_sym.h as well.
Other than that, this diff reads ok.
-ml
>
> /*
> @@ -253,20 +185,13 @@ db_search_symbol(db_addr_t val, db_strat
> {
> unsigned int diff;
> db_expr_t newdiff;
> - int i;
> db_sym_t ret = DB_SYM_NULL, sym;
>
> newdiff = diff = ~0;
> - db_last_symtab = NULL;
> - for (i = 0; i < MAXNOSYMTABS; i++) {
> - if (!db_symtabs[i].name)
> - continue;
> - sym = db_elf_sym_search(&db_symtabs[i], val, strategy, &newdiff);
> - if (newdiff < diff) {
> - db_last_symtab = &db_symtabs[i];
> + sym = db_elf_sym_search(&db_symtab, val, strategy, &newdiff);
> + if (newdiff < diff) {
> diff = newdiff;
> ret = sym;
> - }
> }
> *offp = diff;
> return ret;
> @@ -285,7 +210,7 @@ db_symbol_values(db_sym_t sym, char **na
> return;
> }
>
> - db_elf_sym_values(db_last_symtab, sym, namep, &value);
> + db_elf_sym_values(&db_symtab, sym, namep, &value);
>
> if (valuep)
> *valuep = value;
> @@ -336,7 +261,7 @@ db_printsym(db_expr_t off, db_strategy_t
> d, DB_FORMAT_R, 1, 0));
> }
> if (strategy != DB_STGY_PROC) {
> - if (db_elf_line_at_pc(db_last_symtab, cursym,
> + if (db_elf_line_at_pc(&db_symtab, cursym,
> &filename, &linenum, off))
> (*pr)(" [%s:%d]", filename, linenum);
> }
>
