On 2023/06/01 18:46, lijiang wrote:
> On Thu, Jun 1, 2023 at 4:01 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx>
> 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?
Yes, because those are before this subtraction in store_load_module_symbols():
lm->mod_load_symend--;
if (!MODULE_MEMORY() && !MODULE_END(lm->mod_load_symend) &&
>
>
>>
>>>> +/* 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.
ok, I will try to split the function into e.g.
next_module_symbol_by_syment()
next_module_symbol_by_symname()
next_module_symbol_by_value()
Thanks,
Kazu
>
>
>>>
>>> + 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
>
>
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
