Re: [PATCH] misra: address MISRA C Rule 18.3 compliance

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.07.2025 23:34, Dmytro Prokopchuk1 wrote:
> On 7/23/25 16:58, Jan Beulich wrote:
>> On 23.07.2025 12:12, Dmytro Prokopchuk1 wrote:
>>> --- a/xen/common/sched/core.c
>>> +++ b/xen/common/sched/core.c
>>> @@ -360,7 +360,7 @@ static always_inline void sched_spin_lock_double(
>>>       {
>>>           *flags = _spin_lock_irqsave(lock1);
>>>       }
>>> -    else if ( lock1 < lock2 )
>>> +    else if ( (uintptr_t)lock1 < (uintptr_t)lock2 )
>>
>> Similarly, no matter what C or Misra may have to say here, imo such casts are
>> simply dangerous. Especially when open-coded.
> Well, this function 'sched_spin_lock_double' has the following description:
> "If locks are different, take the one with the lower address first."
> 
> I think it's save to compare the integer representations of 'lock1' and 
> 'lock2' addresses explicitly (casting the pointers values to an integer 
> type). We need to find the 'lower address'.
> Any risks here?

These instances of casts are of comparably little risk, yes, but almost every
cast comes with some risk. If only to set a bad precedent that someone the
more or less blindly copies.

But in the end it'll be the scheduler maintainers to judge here.

>>> --- a/xen/common/virtual_region.c
>>> +++ b/xen/common/virtual_region.c
>>> @@ -83,8 +83,8 @@ const struct virtual_region *find_text_region(unsigned 
>>> long addr)
>>>       rcu_read_lock(&rcu_virtual_region_lock);
>>>       list_for_each_entry_rcu ( iter, &virtual_region_list, list )
>>>       {
>>> -        if ( (void *)addr >= iter->text_start &&
>>> -             (void *)addr <  iter->text_end )
>>> +        if ( addr >= (unsigned long)iter->text_start &&
>>> +             addr <  (unsigned long)iter->text_end )
>>
>> Considering further casts to unsigned long of the same struct fields, was it
>> considered to alter the type of the struct fields instead?
> There are present casts of struct fields 'text_start' and 'text_end' in 
> the file 'xen/common/virtual_region.c'.
> 
>          modify_xen_mappings_lite((unsigned long)region->text_start,
>                                   (unsigned long)region->text_end,
>                                   PAGE_HYPERVISOR_RWX);
> 
> Changing fields type to 'unsigned long' will give the opportunity to 
> remove casts from source code (mentioned before),
> and remove '(void*)' casts from here:
> 
>          if ( (void *)addr >= iter->text_start &&
>               (void *)addr <  iter->text_end )
> 
> Unfortunately there are places where these fields are initialized, so 
> cast to the 'unsigned long' should be there.
> Example:
>      .text_start = _sinittext,
>      .text_end = _einittext,
> and
>      .text_start = _sinittext,
>      .text_end = _einittext,
> 
> where
>      extern char _sinittext[], _einittext[];
>      extern char _stext[], _etext[];
> 
> So, my proposition is to add cast to 'unsigned long' as I proposed in 
> this patch.

My take is that the solution with, ultimately, fewer casts overall wants using.
Plus my personal view is that casts in initializers are a little less "bad".

Jan