Re: [XEN PATCH 3/4] xen/include: add pure and const attributes

[Simone Ballarin <simone.ballarin@xxxxxxxxxxx>]

On 23/10/23 15:34, Jan Beulich wrote:
> On 18.10.2023 16:18, Simone Ballarin wrote:
> > Add const and pure attributes to address reports
> > of MISRA C:2012 Rule 13.1: Initializer lists shall
> > not contain persistent side effects
> >
> > Add pure attribute to function pdx_to_pfn.
> > Add const attribute to functions generated by TYPE_SAFE.
> >
> > These functions are used in initializer lists: adding
> > the attributes ensures that no effect will be performed
> > by them.
> Adding the attribute does, according to my understanding, ensure nothing
> The compiler may (but isn't required to) diagnose wrong uses of the
> attributes, but it may also make use of the attributes (on the
> declaration) without regard to the attribute potentially being wrongly
> applied. Since further for inline functions the compiler commonly infers
> attributes from the expanded code (discarding the attribute), the only
> thing achieved here is a documentation aspect, I think.
Yes, you're right. I will rephrase the commit message.


> > --- a/xen/include/xen/pdx.h
> > +++ b/xen/include/xen/pdx.h
> > @@ -164,7 +164,7 @@ static inline unsigned long pfn_to_pdx(unsigned long pfn)
> >    * @param pdx Page index
> >    * @return Obtained pfn after decompressing the pdx
> >    */
> > -static inline unsigned long pdx_to_pfn(unsigned long pdx)
> > +static inline __attribute_pure__ unsigned long pdx_to_pfn(unsigned long pdx)
> >   {
> >       return (pdx & pfn_pdx_bottom_mask) |
> >              ((pdx << pfn_pdx_hole_shift) & pfn_top_mask);
> Taking this as an example for what I've said above: The compiler can't
> know that the globals used by the functions won't change value. Even
> within Xen it is only by convention that these variables are assigned
> their values during boot, and then aren't changed anymore. Which makes
> me wonder: Did you check carefully that around the time the variables
> have their values established, no calls to the functions exist (which
> might then be subject to folding)?
There is no need to check that, the GCC documentation explicitly says:

However, functions declared with the pure attribute *can safely read any 
non-volatile objects*, and modify the value of objects in a way that 
does not affect their return value or the observable state of the program.
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html

> Additionally - what about the sibling function pfn_to_pdx()?
>
> Jan
I will add the attribute also to the sibling.

--
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)