On 2023/06/02 17:02, lijiang wrote:
> On Fri, Jun 2, 2023 at 12:33 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx>
> wrote:
>
>> On 2023/06/02 12:46, lijiang wrote:
>>> On Fri, Jun 2, 2023 at 8:59 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx
>>>
>>> wrote:
>>>
>>>> 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?
>>>>
>>>>
>>> Good question. Let me give an example:
>>>
>>> long mod_foo;
>>> long mod_data;
>>>
>>> enumerator_value("MOD_FOO", &mod_foo);
>>> or
>>> enumerator_value("MOD_DATA", &mod_data);
>> Doesn't this use enum in crash?
>>
>>
> Yes, that is my thoughts, just an idea for the enum types. Do you think it
> is doable in crash-utility?
Yes, it's possible. but I think it's an overkill for now, please see below.
>
>>
>>> The enumerator_value() can always get the correct result from the current
>>> vmcore/kcore, if I understand correctly.
>>
>> Yes, I also think about using enumerator_value(), when we need it.
>> But I'm not sure how much they change in future, so I would like
>>
>
> We can initialize the enum member variables one time in crash-utility, and
> store them in memory, when the enum member variables are needed, the crash
> tool can use them at any time. Unless crash needs to read the enum
> variables before initializing them.
>
>
>> to implement it simply for now. Macros can follow up small changes.
>>
>> And when the day comes, we can change the macros like
>>
>> #define MOD_DATA (st->enum_mod_data)
>>
>> or something.
>>
>
> Indeed, that is also one option.
>
>
>>>
>>> But it seems that the macro doesn't have the similar function to get the
>>> value from gdb. And the weakness is that the value may have conflict with
>>> the existing macros. For example:
>>>
>>> -#define ORC_TYPE_CALL 0
>>> +#define ORC_TYPE_CALL ((machdep->flags & ORC_6_4) ? 2
>> : 0)
>>> #define ORC_TYPE_REGS 1
>>> #define ORC_TYPE_REGS_IRET 2
>>
>> Yes, but we use only ORC_TYPE_CALL so far, so I just kept the others
>> as they are. If needed, they can be changed with macros.
>> (they should be removed, but it's discussed on another thread :)
>>
>>
> Ok.
> BTW: If we can solve the problem of changing enum member variables from the
> kernel this time, crash tool won't face similar issues in the future. :-)
I don't think so. Some types of changes can be followed with reading enum
values from kernel, but the other types cannot be followed automatically,
e.g. name change of enum. Then, we need to let crash know the name change
to tie the enum to a variable in crash.
So my thought is that it would be better to update crash depending on how
kernel changes.
>
>>
>>>
>>> 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?
>>>>
>>>>
>>> I got a warning with g++ based on your test case:
>>>
>>> $ g++ -o enum enum.c
>>> enum.c: In function ‘int main(int, char**)’:
>>> enum.c:9:29: error: invalid conversion from ‘int’ to ‘test_enum’
>>> [-fpermissive]
>>> 9 | enum test_enum x = 10;
>>> | ^~
>>> | |
>>> | int
>>>
>>> BTW: crash-utility may be built by various/different compilers.
>>
>> gcc and clang don't emit a warning. Can g++ compile crash?
>>
>>
> It's true. I only see the warning based on the g++ and clang++ for now.
>
> # clang++ -o enum enum.c
> enum.c:9:25: error: cannot initialize a variable of type 'enum test_enum'
> with an rvalue of type 'int'
> enum test_enum x = 10;
> ^ ~~
> 1 error generated.
>
> Actually, some code in crash-utility is compiled by g++ compiler, such as
> the gdb-related code(crash_target.c, etc). If the macro definition
> "for_each_mod_mem_type" is put in the defs.h, it may be helpful to check
> the warning/error, because the defs.h header file may be included by the
> source files(compiled with g++ compiler), otherwise it doesn't affect.
>
> Anyway, it depends on where the above macro definition is placed.
>
> And eventually you can make a decision.
OK, anyway defining the enum mod_mem_type in crash is not flexible,
will go with macros for now.
Thanks,
Kazu
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
