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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.02.2024 00:48, Stefano Stabellini wrote:
> On Mon, 26 Feb 2024, Jan Beulich wrote:
>> On 23.02.2024 23:36, Stefano Stabellini wrote:
>>> On Fri, 23 Feb 2024, Jan Beulich wrote:
>>>> On 23.02.2024 02:32, Stefano Stabellini wrote:
>>>>> On Tue, 24 Oct 2023, Jan Beulich wrote:
>>>>>> 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.
>>>>>
>>>>> Let me resurrect this thread.
>>>>>
>>>>> Could we use something like "pure" that we #define as we want?
>>>>>
>>>>> Depending on the compiler version or other options we could #define pure
>>>>> to __attribute_pure__ or to nothing.
>>>>
>>>> While we can do about anything, I don't think it's a good idea to overload
>>>> a well known term with something having somewhat different meaning. If a
>>>> differently named custom attribute helps, that might be a possible option.
>>>
>>> It doesn't have a different meaning. If it had a different meaning I'd
>>> agree with you.
>>
>> Then we need to sort this aspect first: If there was no difference in
>> meaning, we ought to be using the real attribute, not a pseudo
>> surrogate. Yet the earlier discussion, according to my understanding,
>> has led to the understanding that for the given example the real
>> attribute cannot be applied entirely legitimately. Hence why the
>> thinking of alternatives actually started. What am I missing?
> 
> There are two different questions:
> 1) using __attribute_pure__ in general when appropriate
> 2) using __attribute_pure__ in pdx_to_pfn as this patch does
> 
> 
> I was talking about 1): as a general approach it looks like a good idea
> to use __attribute_pure__ when possible and appropriate.
> 
> Now let's talk about 2). The latest definition of __attribute_pure__ is:
> 
> """
> The pure attribute prohibits a function from modifying the state of the 
> program that is observable by means other than inspecting the function’s 
> return value. 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.
> """
> 
> So there are two interesting issues:
> 
> a) While this documentation explicitly allows for reading global vars,
> older versions of the docs are less clear. What do we do about them?
> 
> b) Jan wrote that he interprets the statements above to be only valid in
> a single-threaded environment
> 
> 
> To be honest, I am not convinced by b). Jan, is there a statement in the
> GCC docs that says that all the attributes (pure being one of them) only
> apply to a single-thread environment?

It would need to be the other way around, I'm afraid: C99 defines its
abstract machine in a way not even considering possible parallel
execution (except for other external agents, e.g. when "volatile" is
necessary for memory accesses when that memory may also be modified
by such an external agent, e.g. a device on the bus). Hence we'd need
to find an explicit statement in gcc docs which relaxes that globally
or for certain aspects.

> That would be extremely limiting
> for something like __attribute_pure__. I think we should take the
> documentation of attribute pure at face value. To me, it clearly applies
> to pdx_to_pfn. Roberto and the team at Bugseng came to the same
> conclusion.
> 
> On the other end, I think a) is important. Older version of GCC don't
> clarify the behavior toward global variables. From the documentation, I
> would use __attribute_pure__ only with GCC 9 or later. Which is why we
> need the #define.

Right, this is a position we can take. As said, I think we'd then limit
ourselves more than necessary. Otoh the number of people using gcc8 or
older to build up-to-date Xen should be constantly decreasing ...

Jan

>>> The goal is for the #define to have exactly the same meaning as the gcc
>>> definition from gcc 9 onward. However, other versions of gcc or other
>>> compilers could have different semantics. Also we might not want to
>>> allow gcc to perform the optimizations that it might want to do if the
>>> attribute is passed.
>>>
>>> So the definition would be clear and 100% aligned with the modern gcc
>>> definition. However we would be able to control the behavior better.
>>
>> If we feared older gcc didn't implement "pure" suitably, we should
>> simply make __attribute_pure__ expand to nothing there. (Still use of
>> the attribute then would need limiting to cases where it can validly
>> be applied.)
> 
> That's fine by me