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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan