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

[Juergen Gross]

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

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

+-doc_begin="Consider relational pointer comparisons in kernel-related sections as safe and compliant."

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

OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

OpenPGP_signature.asc
Description: OpenPGP digital signature