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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.10.2023 15:51, Andrew Cooper wrote:
> On 23/10/2023 2:34 pm, 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)?
> 
> I was actually going to point this out, but hadn't found the words.
> 
> pdx_to_pfn() is not pure.  It violates the requirements for being
> declared pure, in a way that the compiler can see.

Hmm, I think you're remembering old wording in the doc. Nowadays it says
"However, functions declared with the pure attribute can safely read any
 nonvolatile objects, and modify the value of objects in a way that does
 not affect their return value or the observable state of the program."

To me this reads as if reads of globals are okay, constrained by the
generally single-threaded view a compiler takes.

Irrespective I agree that without said further checking (and perhaps
even some way of making sure the requirements can't be violated by
future changes), ...

> Right now, this will cause GCC to ignore the attribute, but who's to say
> that future GCCs don't start emitting a diagnostic (in which case we'd
> have to delete them to make them compile), or start honouring them (at
> which point this logic will start to malfunction around the boot time
> modification to the masks).
> 
> 
> It is undefined behaviour to intentionally lie to the compiler using
> attributes.  This is intentionally introducing undefined behaviour to
> placate Eclair.

... we'd effectively be lying to the compiler, putting ourselves at risk.

Jan

> So why are we bending over backwards to remove UB in other areas, but
> deliberately introducing here?  How does that conform with the spirit of
> MISRA?
> 
> ~Andrew