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

From: Stefano Stabellini <sstabellini@kernel.org>
To: Jan Beulich <jbeulich@suse.com>
Cc: Simone Ballarin <simone.ballarin@bugseng.com>,
	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:05:59 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.22.394.2310231417260.3516@ubuntu-linux-20-04-desktop> (raw)
In-Reply-To: <e8bf9817-fd54-9bf4-4302-dcee682f9172@suse.com>

[-- Attachment #1: Type: text/plain, Size: 4746 bytes --]

On Mon, 23 Oct 2023, Jan Beulich wrote:
> On 23.10.2023 17:23, Simone Ballarin wrote:
> > On 23/10/23 15:34, Jan Beulich wrote:
> >> On 18.10.2023 16:18, Simone Ballarin wrote:
> >>> --- 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.
> 
> I did quote this same text in response to what Andrew has said, but I also
> did note there that this needs to be taken with a grain of salt: The
> compiler generally assumes a single-threaded environment, i.e. no changes
> to globals behind the back of the code it is processing.

Let's start from the beginning. The reason for Simone to add
__attribute_pure__ to pdx_to_pfn and other functions is for
documentation purposes. It is OK if it doesn't serve any purpose other
than documentation.

Andrew, for sure we do not want to lie to the compiler and introduce
undefined behavior. If we think there is a risk of it, we should not do
it.

So, what do we want to document? We want to document that the function
does not have side effects according to MISRA's definition of it, which
might subtly differ from GCC's definition.

Looking at GCC's definition of __attribute_pure__, with the
clarification statement copy/pasted above by both Simone and Jan, it
seems that __attribute_pure__ matches MISRA's definition of a function
without side effects. It also seems that pdx_to_pfn abides to that
definition.

Jan has a point that GCC might be making other assumptions
(single-thread execution) that might not hold true in our case. Given
the way the GCC statement is written I think this is low risk. But maybe
not all GCC versions we want to support in the project might have the
same definition of __attribute_pure__. So we could end up using
__attribute_pure__ correctly for the GCC version used for safety (GCC
12.1, see docs/misra/C-language-toolchain.rst) but it might actually
break an older GCC version.

So Option#1 is to use __attribute_pure__ taking the risk that a GCC or
Clang version might interpret __attribute_pure__ differently and
potentially misbehave.

Option#2 is to avoid this risk, by not using __attribute_pure__.
Instead, we can use SAF-xx-safe or deviations.rst to document that
pdx_to_pfn and other functions like it are without side effects
according to MISRA's definition.

Both options have pros and cons. To me the most important factor is how
many GCC versions come with the statement "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".

I checked and these are the results:
- gcc 4.0.2: no statement
- gcc 5.1.0: no statement
- gcc 6.1.0: no statement
- gcc 7.1.0: no statement
- gcc 8.1.0: alternative statement "The pure attribute imposes similar
  but looser restrictions on a function’s definition than the const
  attribute: it allows the function to read global variables."
- gcc 9.1.0: yes statement

So based on the above, __attribute_pure__ comes with its current
definition only from gcc 9 onward. I don't know if as a Xen community we
clearly declare a range of supported compilers, but I would imagine we
would still want to support gcc versions older than 9? (Not to mention
clang, which I haven't checked.)

It doesn't seem to me that __attribute_pure__ could be correctly used on
pdx_to_pfn with GCC 7.1.0 for example.

So in conclusion, I think it is better to avoid __attribute_pure__ and
use SAF-xx-safe or an alternative approach instead.