On Thu, May 25, 2023 at 10:36 AM HAGIO KAZUHITO(萩尾 一仁) <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