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

[Dmytro Prokopchuk1 <dmytro_prokopchuk1@xxxxxxxx>]

On 7/23/25 16:58, Jan Beulich wrote:
> On 23.07.2025 12:12, Dmytro Prokopchuk1 wrote:
>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> @@ -568,6 +568,14 @@ C99 Undefined Behaviour 45: Pointers that do not point 
>> into, or just beyond, the
>>   -config=MC3A2.R18.2,reports+={safe, 
>> "any_area(any_loc(any_exp(macro(^page_to_mfn$))))"}
>>   -doc_end
>>   
>> +-doc_begin="Consider relational pointer comparisons in kernel-related 
>> sections as safe and compliant."
>> +-config=MC3R1.R18.3,reports+={safe, 
>> "any_area(any_loc(any_exp(macro(name(is_kernel||is_kernel_text||is_kernel_rodata||is_kernel_inittext)))))"}
>> +-doc_end
>> +
>> +-doc_begin="Allow deviations for pointer comparisons in BUG_ON and ASSERT 
>> macros, treating them as safe for debugging and validation."
>> +-config=MC3R1.R18.3,reports+={safe, 
>> "any_area(any_loc(any_exp(macro(name(BUG_ON||ASSERT)))))"}
>> +-doc_end
> 
> Nit: Drop "deviations for" from the verbal description?
Ok.

> 
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -461,7 +461,8 @@ static void __init efi_arch_edd(void)
>>                   params->device_path_info_length =
>>                       sizeof(struct edd_device_params) -
>>                       offsetof(struct edd_device_params, key);
>> -                for ( p = (const u8 *)&params->key; p < &params->checksum; 
>> ++p )
>> +                for ( p = (const u8 *)&params->key;
>> +                      (uintptr_t)p < (uintptr_t)&params->checksum; ++p )
> 
> There mere addition of such casts makes code more fragile imo. What about the
> alternative of using != instead of < here? I certainly prefer < in such 
> situations,
> but functionally != ought to be equivalent (and less constrained by C and 
> Misra).
> 
> As mentioned several times when discussing these rules, it's also not easy to 
> see
> how "pointers of different objects" could be involved here: It's all within
> *params, isn't it?
Hard to say something. Let's hold this so far.

> 
> Finally, when touching such code it would be nice if u<N> was converted to
> uint<N>_t.
> 
>> --- 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?

Dmytro
> 
>> --- 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.

Dmytro
> 
> Jan