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

[Jan Beulich]

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?

> --- 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?

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.

> --- 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?

Jan