[Crash-utility] [RFC PATCH 09/15] Support "sym -n" option

[lijiang]

On Thu, Jun 1, 2023 at 4:01 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab at nec.com>
wrote:

> >> -               if (CRASHDEBUG(1)) {
> >> +               if (CRASHDEBUG(1) && lm->mod_load_symtable) {
> >>                          for (sp = lm->mod_load_symtable;
> >> -                            sp < lm->mod_load_symend; sp++) {
> >> +                            sp <= lm->mod_load_symend; sp++) {
> >>
> >
> > The real problem might not be here? Some member variables are not
> properly
> > initialized?
>
> sorry, I don't understand this.
>
There is the read problem here.  lm->mod_load_symend is the last
> valid syment address.
>

Ok, Thank you for the explanation, Kazu. But I saw the following code in
other patches(06/15,etc):

+                               for (sp = lm->mod_load_symtable; sp <
lm->mod_load_symend; sp++) {

Are there any differences?


>
> >> +/* Only for 6.4 and later */
> >> +struct syment *
> >> +next_module_symbol(char *symbol, struct syment *sp_in, ulong val_in)
> >> +{
> >> +       int i, j, k;
> >> +       struct load_module *lm;
> >> +       struct syment *sp, *sp_end;
> >> +
> >> +       if (symbol)
> >> +               goto symbol_search;
> >> +       if (val_in)
> >> +               goto value_search;
> >> +
> >> +       /* for sp_in */
> >> +       for (i = 0; i < st->mods_installed; i++) {
> >> +               lm = &st->load_modules[i];
> >> +
> >> +               /* quick check: sp_in is not in the module range. */
> >> +               if (sp_in < lm->symtable[lm->address_order[0]] ||
> >> +                   sp_in >
> lm->symend[lm->address_order[lm->nr_mems-1]])
> >> +                       continue;
> >> +
> >> +               for (j = 0; j < lm->nr_mems; j++) {
> >> +                       k = lm->address_order[j];
> >> +                       if (sp_in < lm->symtable[k] || sp_in >
> >> lm->symend[k])
> >> +                               continue;
> >> +
> >> +                       if (sp_in == lm->symend[k])
> >> +                               return next_module_symbol(NULL, NULL,
> >> sp_in->value);
> >> +
> >>
> >
> > This means it has to be invoked recursively.
>
> It looks to be a recursive call, but actually it's just a composite
> of three functions, i.e. symbol, syment and value search, and the
> value search does not do a recursive call.  So I think it's not a
> real recursive call.
>
>
Ok, but it looks unusual and hard to understand.


> >
> > +                       sp = sp_in + 1;
> >> +                       if (MODULE_PSEUDO_SYMBOL(sp))
> >> +                               return next_module_symbol(NULL, NULL,
> >> sp->value);
> >> +
> >> +                       return sp;
> >> +               }
> >> +       }
> >> +       return NULL;
> >> +
> >> +value_search:
> >> +       sp = sp_end = NULL;
> >> +       for (i = 0; i < st->mods_installed; i++) {
> >> +               lm = &st->load_modules[i];
> >> +
> >> +               /* quick check: val_in is higher than the highest in the
> >> module. */
> >> +               if (val_in >
> >> lm->symend[lm->address_order[lm->nr_mems-1]]->value)
> >> +                       continue;
> >> +
> >> +               for (j = 0; j < lm->nr_mems; j++) {
> >> +                       k = lm->address_order[j];
> >> +                       if (val_in < lm->symtable[k]->value &&
> >> +                           (sp == NULL || lm->symtable[k]->value <
> >> sp->value)) {
> >> +                               sp = lm->symtable[k];
> >> +                               sp_end = lm->symend[k];
> >> +                               break;
> >> +                       }
> >> +               }
> >> +       }
> >> +       for ( ; sp < sp_end; sp++) {
> >> +               if (MODULE_PSEUDO_SYMBOL(sp))
> >> +                       continue;
> >> +               if (sp->value > val_in)
> >> +                       return sp;
> >> +       }
> >> +       return NULL;
> >> +
> >> +symbol_search:
> >> +       /*
> >> +        *  Deal with a few special cases...
> >> +        *
> >> +        * hmm, looks like crash now does not use these special cases.
> >> +        *
> >> +       if (strstr(symbol, " module)")) {
> >> +                sprintf(buf, "_MODULE_START_");
> >> +                strcat(buf, &symbol[1]);
> >> +                p1 = strstr(buf, " module)");
> >> +                *p1 = NULLCHAR;
> >> +                symbol = buf;
> >> +       }
> >> +
> >> +       if (STREQ(symbol, "_end")) {
> >> +               if (!st->mods_installed)
> >> +                       return NULL;
> >> +
> >> +                lm = &st->load_modules[0];
> >> +
> >> +               return lm->mod_symtable;
> >> +       }
> >> +       */
> >>
> >
> > The symbol_search code block can be moved to the beginning of this
> > function, the current code has become very simple, that can avoid the
> goto
> > statement.
>
> Isn't it pointless?
> If we do so, we will need something like "goto syment_search" instead.
>
>
It's true, I did not see that the next_module_symbol() is called again the
label symbol_search.

Thanks
Lianbo
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20230601/f9ed4be5/attachment-0001.htm>