7:59 p.m.
On 2023/05/30 15:37, lijiang wrote:
On Thu, May 25, 2023 at 10:36 AM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab(a)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(a)nec.com
<mailto:k-hagio-ab@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