On 2023/05/29 20:11, lijiang wrote:
> On Thu, May 11, 2023 at 12:35 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx>
> wrote:
>
>> and fix off-by-one bug in "help -s" output with CRASHDEBUG(1)
>>
>> Signed-off-by: Kazuhito Hagio <k-hagio-ab@xxxxxxx>
>> ---
>> symbols.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 109 insertions(+), 2 deletions(-)
>>
>> diff --git a/symbols.c b/symbols.c
>> index 5399c7494cb1..a432909ff28e 100644
>> --- a/symbols.c
>> +++ b/symbols.c
>> @@ -110,6 +110,7 @@ static int module_mem_type(ulong, struct load_module
>> *);
>> static ulong module_mem_end(ulong, struct load_module *);
>> static int _in_module_range(ulong, struct load_module *, int, int);
>> struct syment *value_search_module_v2(ulong, ulong *);
>> +struct syment *next_module_symbol(char *, struct syment *, ulong);
>>
>> static const char *module_start_tags[];
>> static const char *module_start_strs[];
>> @@ -4116,9 +4117,9 @@ dump_symbol_table(void)
>>
>> fprintf(fp, " loaded_objfile: %lx\n",
>> (ulong)lm->loaded_objfile);
>>
>> - 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.
crash> help -s
...
ffffffffc0146838 shared_memory_lock
ffffffffc014683c dm_stat_need_rcu_barrier
ffffffffc014e000 _MODULE_END_dm_mod
mod_base: ffffffffc014f000
Without the patch, _MODULE_END_* are not displayed:
ffffffffc0146838 shared_memory_lock
ffffffffc014683c dm_stat_need_rcu_barrier
mod_base: ffffffffc014f000
>
> fprintf(fp, " %lx %s\n",
>> sp->value, sp->name);
>> }
>> @@ -5857,6 +5858,106 @@ closest_symbol_value(ulong value)
>> return(0);
>> }
>>
>> +/* 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.
>
> + 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;
>> + }
>> + */
>>
>
> I tried to find the old code, but still did not get the changed history,
> and did not see the similar symbols " module)" in the latest kernel.
Yes.
> 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.
>
> + if ((sp = symbol_search(symbol))) {
>> + sp++;
>> + if (MODULE_PSEUDO_SYMBOL(sp) && strstr(sp->name, "_END"))
>> + return next_module_symbol(NULL, NULL, sp->value);
>> +
>> + return sp;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> /*
>> * For a given symbol, return a pointer to the next (higher) symbol's
>> syment.
>> * Either a symbol name or syment pointer may be passed as an argument.
>> @@ -5886,6 +5987,9 @@ next_symbol(char *symbol, struct syment *sp_in)
>>
>> search_init = FALSE;
>>
>> + if (MODULE_MEMORY())
>> + return next_module_symbol(NULL, sp_in, 0);
>> +
>>
>
> The above if-code block should be moved before the "search_init = FALSE".
True, will fix.
Thanks,
Kazu
>
> Thanks.
> Lianbo
>
> for (i = 0; i < st->mods_installed; i++) {
>> lm = &st->load_modules[i];
>> if (lm->mod_flags & MOD_INIT)
>> @@ -5928,6 +6032,9 @@ next_symbol(char *symbol, struct syment *sp_in)
>> }
>>
>>
>> + if (MODULE_MEMORY())
>> + return next_module_symbol(symbol, NULL, 0);
>> +
>> /*
>> * Deal with a few special cases...
>> */
>> --
>> 2.31.1
>>
>>
>>
>> On Thu, May 11, 2023 at 12:35 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx <mailto:k-hagio-ab@xxxxxxx>> wrote:
>>
>> and fix off-by-one bug in "help -s" output with CRASHDEBUG(1)
>>
>> Signed-off-by: Kazuhito Hagio <k-hagio-ab@xxxxxxx <mailto:k-hagio-ab@xxxxxxx>>
>> ---
>> symbols.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 109 insertions(+), 2 deletions(-)
>>
>> diff --git a/symbols.c b/symbols.c
>> index 5399c7494cb1..a432909ff28e 100644
>> --- a/symbols.c
>> +++ b/symbols.c
>> @@ -110,6 +110,7 @@ static int module_mem_type(ulong, struct load_module *);
>> static ulong module_mem_end(ulong, struct load_module *);
>> static int _in_module_range(ulong, struct load_module *, int, int);
>> struct syment *value_search_module_v2(ulong, ulong *);
>> +struct syment *next_module_symbol(char *, struct syment *, ulong);
>>
>> static const char *module_start_tags[];
>> static const char *module_start_strs[];
>> @@ -4116,9 +4117,9 @@ dump_symbol_table(void)
>>
>> fprintf(fp, " loaded_objfile: %lx\n", (ulong)lm->loaded_objfile);
>>
>> - 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?
>>
>> fprintf(fp, " %lx %s\n",
>> sp->value, sp->name);
>> }
>> @@ -5857,6 +5858,106 @@ closest_symbol_value(ulong value)
>> return(0);
>> }
>>
>> +/* 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.
>>
>> + 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;
>> + }
>> + */
>>
>>
>> I tried to find the old code, but still did not get the changed history, and did not see the similar symbols " module)" in the latest kernel.
>>
>> 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.
>>
>> + if ((sp = symbol_search(symbol))) {
>> + sp++;
>> + if (MODULE_PSEUDO_SYMBOL(sp) && strstr(sp->name, "_END"))
>> + return next_module_symbol(NULL, NULL, sp->value);
>> +
>> + return sp;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> /*
>> * For a given symbol, return a pointer to the next (higher) symbol's syment.
>> * Either a symbol name or syment pointer may be passed as an argument.
>> @@ -5886,6 +5987,9 @@ next_symbol(char *symbol, struct syment *sp_in)
>>
>> search_init = FALSE;
>>
>> + if (MODULE_MEMORY())
>> + return next_module_symbol(NULL, sp_in, 0);
>> +
>>
>>
>> The above if-code block should be moved before the "search_init = FALSE".
>>
>> Thanks.
>> Lianbo
>>
>> for (i = 0; i < st->mods_installed; i++) {
>> lm = &st->load_modules[i];
>> if (lm->mod_flags & MOD_INIT)
>> @@ -5928,6 +6032,9 @@ next_symbol(char *symbol, struct syment *sp_in)
>> }
>>
>>
>> + if (MODULE_MEMORY())
>> + return next_module_symbol(symbol, NULL, 0);
>> +
>> /*
>> * Deal with a few special cases...
>> */
>> --
>> 2.31.1
>>
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
