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); > } >