Re: [PATCH] misra: address Rule 11.1 violation in cmpxchgptr()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 14 Aug 2025, Teddy Astie wrote:
> Hello,
> 
> Le 13/08/2025 à 20:07, Dmytro Prokopchuk1 a écrit :
> > Misra Rule 11.1 states: "Conversions shall not be performed between a
> > pointer to a function and any other type."
> >
> > The violation occurs in the macro:
> >      __typeof__(**(ptr)) *const o_ = (o);                                \
> >      __typeof__(**(ptr)) *n_ = (n);                                      \
> >      ((__typeof__(*(ptr)))__cmpxchg(ptr, (unsigned long)o_,              \
> >                                     (unsigned long)n_, sizeof(*(ptr)))); \
> > })
> > when it is used for handling function pointers of type:
> > typedef void (*)(struct vcpu *, unsigned int).
> > The issue happens because the '__cmpxchg()' function returns an 'unsigned
> > long', which is then converted back into a function pointer, causing a
> > violation of Rule 11.1. In this particular usage, the return value of the
> > macro 'cmpxchgptr()' is not required. To address the violation, the macro
> > has been replaced to discard the return value of '__cmpxchg()', preventing
> > the conversion.
> >
> > Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@xxxxxxxx>
> > ---
> > Probably separate macro is too much for this single case.
> >
> > And the following will be enought:
> > __cmpxchg(&xen_consumers[i], (unsigned long)NULL, (unsigned long)fn, 
> > sizeof(*(&xen_consumers[i])));
> > ---
> >   xen/common/event_channel.c | 15 +++++++++++++--
> >   1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> > index 67700b050a..2094338b28 100644
> > --- a/xen/common/event_channel.c
> > +++ b/xen/common/event_channel.c
> > @@ -93,6 +93,17 @@ static void cf_check default_xen_notification_fn(
> >           vcpu_wake(v);
> >   }
> >
> > +/*
> > + * A slightly more updated variant of cmpxchgptr() where old value
> > + * is not returned.
> > + */
> > +#define cmpxchgptr_noret(ptr, o, n) ({                  \
> > +    __typeof__(**(ptr)) *const o_ = (o);                \
> > +    __typeof__(**(ptr)) *n_ = (n);                      \
> > +    (void)__cmpxchg(ptr, (unsigned long)o_,             \
> > +                    (unsigned long)n_, sizeof(*(ptr))); \
> > +})
> > +
> >   /*
> >    * Given a notification function, return the value to stash in
> >    * the evtchn->xen_consumer field.
> > @@ -106,9 +117,9 @@ static uint8_t 
> > get_xen_consumer(xen_event_channel_notification_t fn)
> >
> >       for ( i = 0; i < ARRAY_SIZE(xen_consumers); i++ )
> >       {
> > -        /* Use cmpxchgptr() in lieu of a global lock. */
> > +        /* Use cmpxchgptr_noret() in lieu of a global lock. */
> >           if ( xen_consumers[i] == NULL )
> > -            cmpxchgptr(&xen_consumers[i], NULL, fn);
> > +            cmpxchgptr_noret(&xen_consumers[i], NULL, fn);
> >           if ( xen_consumers[i] == fn )
> >               break;
> >       }
> 
> AFAICS, Rule 11.1 has a deviation which allows this specific case.
> 
> In docs/misra/deviations.rst
> > * - R11.1
> >   - The conversion from a function pointer to unsigned long or (void \*) 
> > does
> >     not lose any information, provided that the target type has enough bits
> >     to store it.
> >   - Tagged as `safe` for ECLAIR.
> 
> Here, we are constructing a function pointer from a unsigned long. I
> assume this rule goes the other way it says, and allow converting a
> unsigned long into a function pointer as long as its value is a valid
> function pointer.

You are right, we should need to update the deviation instead here:


-doc_begin="The conversion from a function pointer to unsigned long or (void *) 
does not lose any information, provided that the target type has enough bits to 
store it."
-config=MC3A2.R11.1,casts+={safe,
  "from(type(canonical(__function_pointer_types)))
   &&to(type(canonical(builtin(unsigned long)||pointer(builtin(void)))))
   &&relation(definitely_preserves_value)"
}
-doc_end