[Crash-utility] [RFC PATCH 02/15] Support "sym -l|-M|-m" options
HAGIO KAZUHITO(萩尾 一仁)
k-hagio-ab at nec.com
Fri Jun 2 00:59:12 UTC 2023
On 2023/05/30 15:37, lijiang wrote:
> On Thu, May 25, 2023 at 10:36 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab at nec.com>
> 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 at nec.com <mailto:k-hagio-ab at nec.com>> 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
More information about the Crash-utility
mailing list