3:01 a.m.
On 2023/05/29 20:11, lijiang wrote:
On Thu, May 11, 2023 at 12:35 PM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab(a)nec.com>
wrote:
> and fix off-by-one bug in "help -s" output with CRASHDEBUG(1)
>
> Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com>
> ---
> 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(a)nec.com
<mailto:k-hagio-ab@nec.com>> wrote:
>>
>> and fix off-by-one bug in "help -s" output with CRASHDEBUG(1)
>>
>> Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com
<mailto:k-hagio-ab@nec.com>>
>> ---
>> 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
>>