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

[Juergen Gross <jgross@xxxxxxxx>]

On 20.08.25 11:18, Dmytro Prokopchuk1 wrote:
> On 7/30/25 01:09, Stefano Stabellini wrote:
> > 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 )
> Could we assume that these 'lock1' and 'lock2' pointers belong to the
> same allocation region - 'sched_resource' ?
No, they can be either in sched_resource, in a per-scheduler private memory
area, or even in the .data section of the hypervisor.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature