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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 25 Jul 2025, Dmytro Prokopchuk1 wrote:
> 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[];
> 

Everything related to stext/etext, sinittext/einittext, etc should be
deviated as those are not even pointers: they are linker symbols. Also,
they do refer to the same "object": the Xen text.