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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>,
	Simone Ballarin <simone.ballarin@bugseng.com>
Cc: consulting@bugseng.com, sstabellini@kernel.org,
	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: Mon, 23 Oct 2023 14:51:14 +0100	[thread overview]
Message-ID: <7b28331d-b1d6-4c6b-b299-34de9ba65e0d@citrix.com> (raw)
In-Reply-To: <89778285-5cba-8fb5-70bc-710b6dd30a10@suse.com>

[-- Attachment #1: Type: text/plain, Size: 1869 bytes --]

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.

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.

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

[-- Attachment #2: Type: text/html, Size: 2526 bytes --]