[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH 3/4] xen/include: add pure and const attributes
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?
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |