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

From: Jan Beulich <jbeulich@suse.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: Simone Ballarin <simone.ballarin@bugseng.com>,
	consulting@bugseng.com, 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: Tue, 24 Oct 2023 08:24:03 +0200	[thread overview]
Message-ID: <8f8141e2-c55c-2e58-a078-d5d8764a02df@suse.com> (raw)
In-Reply-To: <alpine.DEB.2.22.394.2310231417260.3516@ubuntu-linux-20-04-desktop>

On 24.10.2023 00:05, Stefano Stabellini wrote:
> 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.

The absence of documentation doesn't mean the attribute had different
(or even undefined) meaning in earlier versions. Instead it means one
would need to consult other places (source code?) to figure out whether
there was any behavioral difference (I don't think there was).

That said, ...

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

... I agree here. We just don't want to take chances.

Jan