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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

Opinions?