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

From: Jan Beulich <jbeulich@suse.com>
To: Simone Ballarin <simone.ballarin@bugseng.com>
Cc: consulting@bugseng.com, sstabellini@kernel.org,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Julien Grall <julien@xen.org>, Wei Liu <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [XEN PATCH 3/4] xen/include: add pure and const attributes
Date: Mon, 23 Oct 2023 15:34:46 +0200	[thread overview]
Message-ID: <89778285-5cba-8fb5-70bc-710b6dd30a10@suse.com> (raw)
In-Reply-To: <fd5421162a00aa782e0776324ff6497193c1e3d3.1697638210.git.simone.ballarin@bugseng.com>

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.

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

Additionally - what about the sibling function pfn_to_pdx()?

Jan