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

[Dmytro Prokopchuk1 <dmytro_prokopchuk1@xxxxxxxx>]

On 7/28/25 11:06, Jan Beulich wrote:
> 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

As for me the following changes look not good (if change struct item 
type to 'unsigned long').
Just code snips.

-    const void *text_start;
-    const void *text_end;
+    unsigned long    text_start;
+    unsigned long    text_end;

-    const void *rodata_start;
-    const void *rodata_end;
+    unsigned long    rodata_start;
+    unsigned long    rodata_end;


-    region->text_start = payload->text_addr;
-    region->text_end = payload->text_addr + payload->text_size;
+    region->text_start = (unsigned long)payload->text_addr;
+    region->text_end = (unsigned long)(payload->text_addr + 
payload->text_size);


-        region->rodata_start = payload->ro_addr;
-        region->rodata_end = payload->ro_addr + payload->ro_size;
+        region->rodata_start = (unsigned long)payload->ro_addr;
+        region->rodata_end = (unsigned long)(payload->ro_addr + 
payload->ro_size);


-    .text_start = _stext,
-    .text_end = _etext,
-    .rodata_start = _srodata,
-    .rodata_end = _erodata,
+    .text_start = (unsigned long)_stext,
+    .text_end = (unsigned long)_etext,
+    .rodata_start = (unsigned long)_srodata,
+    .rodata_end = (unsigned long)_erodata,


-    .text_start = _sinittext,
-    .text_end = _einittext,
+    .text_start = (unsigned long)_sinittext,
+    .text_end = (unsigned long)_einittext,


-        if ( (void *)addr >= iter->text_start &&   <<<<< Actually the 
violation was only here
-             (void *)addr <  iter->text_end )    <<<<<< and here
+        if ( addr >= iter->text_start &&
+             addr <  iter->text_end )


-        modify_xen_mappings_lite((unsigned long)region->text_start,
-                                 (unsigned long)region->text_end,
+        modify_xen_mappings_lite(region->text_start,
+                                 region->text_end,



          if ( region->rodata_start )
-            modify_xen_mappings_lite((unsigned long)region->rodata_start,
-                                     (unsigned long)region->rodata_end,
+            modify_xen_mappings_lite(region->rodata_start,
+                                     region->rodata_end,


-        modify_xen_mappings_lite((unsigned long)region->text_start,
-                                 (unsigned long)region->text_end,
+        modify_xen_mappings_lite(region->text_start,
+                                 region->text_end,


          if ( region->rodata_start )
-            modify_xen_mappings_lite((unsigned long)region->rodata_start,
-                                     (unsigned long)region->rodata_end,
+            modify_xen_mappings_lite(region->rodata_start,
+                                     region->rodata_end,


Here are run-time casts, and especially I don't like this 'if' statements:
  'if ( region->rodata_start )' and 'if ( region->rodata_start )'.

It intended to be NULL ptr check, but now it's not.
Probably it will work as expected, assuming these integer values are 
zero, but I'm not sure at all.

Dmytro.