On 2023/05/30 15:37, lijiang wrote: > On Thu, May 25, 2023 at 10:36 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx> > wrote: > >>> >>> The variable "j" should be defined as enum mod_mem_type. >> >> hmm, so can we change the enums to macros for now? >> >> > Here, the enums are more flexible than macros, once the kernel changes the > enums, crash-utility is easy to follow up kernel changes. Sorry, I couldn't imagine this. How can we use enum flexibly to follow up kernel changes, for example, when MOD_FOO=1 is inserted? It feels like to me macros can be used a bit flexibly, e.g. like [1]. [1] https://listman.redhat.com/archives/crash-utility/2023-May/010683.html > > #define MOD_TEXT 0 >> #define MOD_DATA 1 >> ... >> >> I don't see enum's good point here in crash. >> >> > If the value is out of the enums ranges, it may have a warning from the > compiler, just a guess, not confirmed. I tried this, but couldn't get a warning and find such an option. Maybe this isn't what you mean? $ cat enum.c #include <stdio.h> enum test_enum { a, b = 5, c, }; int main(int argc, char *argv[]) { enum test_enum x = 10; printf("x = %d\n", x); printf("a = %d b = %d c= %d\n", a, b, c); return 0; } $ cc -Wall -o enum enum.c $ ./enum x = 10 a = 0 b = 5 c= 6 Thanks, Kazu > > >>> @@ -1808,6 +1911,24 @@ static const char *module_end_tags[] = { >>>> "_MODULE_INIT_DATA_END_", >>>> "_MODULE_INIT_RODATA_END_" >>>> }; >>>> +static const char *module_start_strs[] = { >>>> + "MODULE TEXT START", >>>> + "MODULE DATA START", >>>> + "MODULE RODATA START", >>>> + "MODULE RO_AFTER_INIT START", >>>> + "MODULE INIT_TEXT START", >>>> + "MODULE INIT_DATA START", >>>> + "MODULE INIT_RODATA START" >>>> +}; >>>> +static const char *module_end_strs[] = { >>>> + "MODULE TEXT END", >>>> + "MODULE DATA END", >>>> + "MODULE RODATA END", >>>> + "MODULE RO_AFTER_INIT END", >>>> + "MODULE INIT_TEXT END", >>>> + "MODULE INIT_DATA END", >>>> + "MODULE INIT_RODATA END" >>>> +}; >>>> >>>> >>> As I mentioned in the [PATCH 01/15], similarly the above strings are in >>> pairs, so they can be defined as one array or macro substitution. >> >> Yes, how about this? >> >> > Also looks good to me. Thanks. > > >> struct module_tag { >> char *start; >> char *end; >> char *start_str; >> char *end_str; >> }; >> >> #define MODULE_TAG(type, suffix) ("_MODULE_" #type "_" #suffix) >> #define MODULE_STR(type, suffix) ( "MODULE " #type " " #suffix) >> >> #define MODULE_TAGS(type) { \ >> .start = MODULE_TAG(type, START), \ >> .end = MODULE_TAG(type, END), \ >> .start_str = MODULE_STR(type, START), \ >> .end_str = MODULE_STR(type, END) \ >> } >> >> static const struct module_tag module_tag[] = { >> MODULE_TAGS(TEXT), >> MODULE_TAGS(DATA), >> MODULE_TAGS(RODATA), >> MODULE_TAGS(RO_AFTER_INIT), >> MODULE_TAGS(INIT_TEXT), >> MODULE_TAGS(INIT_DATA), >> MODULE_TAGS(INIT_RODATA), >> }; >> >> ... > >> >>> >>> >>>> + if (is_insmod_builtin(lm, sp)) { >>>> + spnext = sp+1; >>>> + if ((spnext < sp_end) && >>>> + (value == >>>> spnext->value)) >>>> + sp = spnext; >>>> + } >>>> + if (sp->name[0] == '.') { >>>> + spnext = sp+1; >>>> + if (spnext->value == >> value) >>>> + sp = spnext; >>>> + } >>>> + if (offset) >>>> + *offset = 0; >>>> + return sp; >>>> + } >>>> + >>>> + if (sp->value > value) { >>>> + sp = splast ? splast : sp - 1; >>>> + if (offset) >>>> + *offset = value - >>>> sp->value; >>>> + return sp; >>>> + } >>>> + >>>> >>> >>> Why is it needed? The splast will also store the "__insmod_"-type >> symbols? >> >> To return the previous sp and offset, when value is lower than the >> current sp. >> >> > Thank you for the explanation, Kazu. > > Lianbo > > > On Thu, May 25, 2023 at 10:36 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx <mailto:k-hagio-ab@xxxxxxx>> wrote: > > > > > The variable "j" should be defined as enum mod_mem_type. > > hmm, so can we change the enums to macros for now? > > Here, the enums are more flexible than macros, once the kernel changes the enums, crash-utility is easy to follow up kernel changes. > > #define MOD_TEXT 0 > #define MOD_DATA 1 > ... > > I don't see enum's good point here in crash. > > > If the value is out of the enums ranges, it may have a warning from the compiler, just a guess, not confirmed. > > > >> @@ -1808,6 +1911,24 @@ static const char *module_end_tags[] = { > >> "_MODULE_INIT_DATA_END_", > >> "_MODULE_INIT_RODATA_END_" > >> }; > >> +static const char *module_start_strs[] = { > >> + "MODULE TEXT START", > >> + "MODULE DATA START", > >> + "MODULE RODATA START", > >> + "MODULE RO_AFTER_INIT START", > >> + "MODULE INIT_TEXT START", > >> + "MODULE INIT_DATA START", > >> + "MODULE INIT_RODATA START" > >> +}; > >> +static const char *module_end_strs[] = { > >> + "MODULE TEXT END", > >> + "MODULE DATA END", > >> + "MODULE RODATA END", > >> + "MODULE RO_AFTER_INIT END", > >> + "MODULE INIT_TEXT END", > >> + "MODULE INIT_DATA END", > >> + "MODULE INIT_RODATA END" > >> +}; > >> > >> > > As I mentioned in the [PATCH 01/15], similarly the above strings are in > > pairs, so they can be defined as one array or macro substitution. > > Yes, how about this? > > > Also looks good to me. Thanks. > > struct module_tag { > char *start; > char *end; > char *start_str; > char *end_str; > }; > > #define MODULE_TAG(type, suffix) ("_MODULE_" #type "_" #suffix) > #define MODULE_STR(type, suffix) ( "MODULE " #type " " #suffix) > > #define MODULE_TAGS(type) { \ > .start = MODULE_TAG(type, START), \ > .end = MODULE_TAG(type, END), \ > .start_str = MODULE_STR(type, START), \ > .end_str = MODULE_STR(type, END) \ > } > > static const struct module_tag module_tag[] = { > MODULE_TAGS(TEXT), > MODULE_TAGS(DATA), > MODULE_TAGS(RODATA), > MODULE_TAGS(RO_AFTER_INIT), > MODULE_TAGS(INIT_TEXT), > MODULE_TAGS(INIT_DATA), > MODULE_TAGS(INIT_RODATA), > }; > > ... > > > > > > > >> + if (is_insmod_builtin(lm, sp)) { > >> + spnext = sp+1; > >> + if ((spnext < sp_end) && > >> + (value == > >> spnext->value)) > >> + sp = spnext; > >> + } > >> + if (sp->name[0] == '.') { > >> + spnext = sp+1; > >> + if (spnext->value == value) > >> + sp = spnext; > >> + } > >> + if (offset) > >> + *offset = 0; > >> + return sp; > >> + } > >> + > >> + if (sp->value > value) { > >> + sp = splast ? splast : sp - 1; > >> + if (offset) > >> + *offset = value - > >> sp->value; > >> + return sp; > >> + } > >> + > >> > > > > Why is it needed? The splast will also store the "__insmod_"-type symbols? > > To return the previous sp and offset, when value is lower than the > current sp. > > Thank you for the explanation, Kazu. > > 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
