| Message ID | fd5421162a00aa782e0776324ff6497193c1e3d3.1697638210.git.simone.ballarin@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: address violations of Rule 13.1 | expand |
On Wed, 18 Oct 2023, Simone Ballarin wrote: > Add const and pure attributes to address reports > of MISRA C:2012 Rule 13.1: Initializer lists shall > not contain persistent side effects > > Add pure attribute to function pdx_to_pfn. > Add const attribute to functions generated by TYPE_SAFE. > > These functions are used in initializer lists: adding > the attributes ensures that no effect will be performed > by them. > > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> However one comment below... > --- > Function attributes pure and const do not need to be added as GCC > attributes, they can be added using ECLAIR configurations. > --- > xen/include/xen/pdx.h | 2 +- > xen/include/xen/typesafe.h | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/xen/include/xen/pdx.h b/xen/include/xen/pdx.h > index f3fbc4273a..5d1050967a 100644 > --- 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); > diff --git a/xen/include/xen/typesafe.h b/xen/include/xen/typesafe.h > index 7ecd3b4a8d..615df6f944 100644 > --- a/xen/include/xen/typesafe.h > +++ b/xen/include/xen/typesafe.h > @@ -21,8 +21,8 @@ > > #define TYPE_SAFE(_type, _name) \ > typedef struct { _type _name; } _name##_t; \ > - static inline _name##_t _##_name(_type n) { return (_name##_t) { n }; } \ > - static inline _type _name##_x(_name##_t n) { return n._name; } > + static inline __attribute_const__ _name##_t _##_name(_type n) { return (_name##_t) { n }; } \ > + static inline __attribute_const__ _type _name##_x(_name##_t n) { return n._name; } > > #else I think we should also add __attribute_const__ in the NDEBUG definitions just below.
On 19/10/23 03:02, Stefano Stabellini wrote: > On Wed, 18 Oct 2023, Simone Ballarin wrote: >> Add const and pure attributes to address reports >> of MISRA C:2012 Rule 13.1: Initializer lists shall >> not contain persistent side effects >> >> Add pure attribute to function pdx_to_pfn. >> Add const attribute to functions generated by TYPE_SAFE. >> >> These functions are used in initializer lists: adding >> the attributes ensures that no effect will be performed >> by them. >> >> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > However one comment below... > >> --- >> Function attributes pure and const do not need to be added as GCC >> attributes, they can be added using ECLAIR configurations. >> --- >> xen/include/xen/pdx.h | 2 +- >> xen/include/xen/typesafe.h | 4 ++-- >> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/xen/include/xen/pdx.h b/xen/include/xen/pdx.h >> index f3fbc4273a..5d1050967a 100644 >> --- 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); >> diff --git a/xen/include/xen/typesafe.h b/xen/include/xen/typesafe.h >> index 7ecd3b4a8d..615df6f944 100644 >> --- a/xen/include/xen/typesafe.h >> +++ b/xen/include/xen/typesafe.h >> @@ -21,8 +21,8 @@ >> >> #define TYPE_SAFE(_type, _name) \ >> typedef struct { _type _name; } _name##_t; \ >> - static inline _name##_t _##_name(_type n) { return (_name##_t) { n }; } \ >> - static inline _type _name##_x(_name##_t n) { return n._name; } >> + static inline __attribute_const__ _name##_t _##_name(_type n) { return (_name##_t) { n }; } \ >> + static inline __attribute_const__ _type _name##_x(_name##_t n) { return n._name; } >> >> #else > > I think we should also add __attribute_const__ in the NDEBUG definitions > just below. Ok.
On 18.10.2023 16:18, Simone Ballarin wrote: > Add const and pure attributes to address reports > of MISRA C:2012 Rule 13.1: Initializer lists shall > not contain persistent side effects > > Add pure attribute to function pdx_to_pfn. > Add const attribute to functions generated by TYPE_SAFE. > > These functions are used in initializer lists: adding > the attributes ensures that no effect will be performed > by them. Adding the attribute does, according to my understanding, ensure nothing. The compiler may (but isn't required to) diagnose wrong uses of the attributes, but it may also make use of the attributes (on the declaration) without regard to the attribute potentially being wrongly applied. Since further for inline functions the compiler commonly infers attributes from the expanded code (discarding the attribute), the only thing achieved here is a documentation aspect, I think. > --- 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)? Additionally - what about the sibling function pfn_to_pdx()? Jan
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
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
On 23/10/23 15:34, Jan Beulich wrote: > On 18.10.2023 16:18, Simone Ballarin wrote: >> Add const and pure attributes to address reports >> of MISRA C:2012 Rule 13.1: Initializer lists shall >> not contain persistent side effects >> >> Add pure attribute to function pdx_to_pfn. >> Add const attribute to functions generated by TYPE_SAFE. >> >> These functions are used in initializer lists: adding >> the attributes ensures that no effect will be performed >> by them. > > Adding the attribute does, according to my understanding, ensure nothing > The compiler may (but isn't required to) diagnose wrong uses of the > attributes, but it may also make use of the attributes (on the > declaration) without regard to the attribute potentially being wrongly > applied. Since further for inline functions the compiler commonly infers > attributes from the expanded code (discarding the attribute), the only > thing achieved here is a documentation aspect, I think. Yes, you're right. I will rephrase the commit message. > >> --- 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. https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html > Additionally - what about the sibling function pfn_to_pdx()? > > Jan I will add the attribute also to the sibling.
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. Jan
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. So in conclusion, I think it is better to avoid __attribute_pure__ and use SAF-xx-safe or an alternative approach instead.
> It doesn't seem to me that __attribute_pure__ could be correctly used on > pdx_to_pfn with GCC 7.1.0 for example. > > So in conclusion, I think it is better to avoid __attribute_pure__ and > use SAF-xx-safe or an alternative approach instead. I was thinking that another option would be to introduce a macro "pure" that is defined as __attribute_pure__ for GCC 9 or later and defined to nothing for GCC 8 or older.
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. Jan
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?
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
On Fri, 23 Feb 2024, Jan Beulich wrote: > 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. It doesn't have a different meaning. If it had a different meaning I'd agree with you. The goal is for the #define to have exactly the same meaning as the gcc definition from gcc 9 onward. However, other versions of gcc or other compilers could have different semantics. Also we might not want to allow gcc to perform the optimizations that it might want to do if the attribute is passed. So the definition would be clear and 100% aligned with the modern gcc definition. However we would be able to control the behavior better.
On 23.02.2024 23:36, Stefano Stabellini wrote: > On Fri, 23 Feb 2024, Jan Beulich wrote: >> 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. > > It doesn't have a different meaning. If it had a different meaning I'd > agree with you. Then we need to sort this aspect first: If there was no difference in meaning, we ought to be using the real attribute, not a pseudo surrogate. Yet the earlier discussion, according to my understanding, has led to the understanding that for the given example the real attribute cannot be applied entirely legitimately. Hence why the thinking of alternatives actually started. What am I missing? > The goal is for the #define to have exactly the same meaning as the gcc > definition from gcc 9 onward. However, other versions of gcc or other > compilers could have different semantics. Also we might not want to > allow gcc to perform the optimizations that it might want to do if the > attribute is passed. > > So the definition would be clear and 100% aligned with the modern gcc > definition. However we would be able to control the behavior better. If we feared older gcc didn't implement "pure" suitably, we should simply make __attribute_pure__ expand to nothing there. (Still use of the attribute then would need limiting to cases where it can validly be applied.) Jan
On Mon, 26 Feb 2024, Jan Beulich wrote: > On 23.02.2024 23:36, Stefano Stabellini wrote: > > On Fri, 23 Feb 2024, Jan Beulich wrote: > >> 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. > > > > It doesn't have a different meaning. If it had a different meaning I'd > > agree with you. > > Then we need to sort this aspect first: If there was no difference in > meaning, we ought to be using the real attribute, not a pseudo > surrogate. Yet the earlier discussion, according to my understanding, > has led to the understanding that for the given example the real > attribute cannot be applied entirely legitimately. Hence why the > thinking of alternatives actually started. What am I missing? There are two different questions: 1) using __attribute_pure__ in general when appropriate 2) using __attribute_pure__ in pdx_to_pfn as this patch does I was talking about 1): as a general approach it looks like a good idea to use __attribute_pure__ when possible and appropriate. Now let's talk about 2). The latest definition of __attribute_pure__ is: """ The pure attribute prohibits a function from modifying the state of the program that is observable by means other than inspecting the function’s return value. 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. """ So there are two interesting issues: a) While this documentation explicitly allows for reading global vars, older versions of the docs are less clear. What do we do about them? b) Jan wrote that he interprets the statements above to be only valid in a single-threaded environment To be honest, I am not convinced by b). Jan, is there a statement in the GCC docs that says that all the attributes (pure being one of them) only apply to a single-thread environment? That would be extremely limiting for something like __attribute_pure__. I think we should take the documentation of attribute pure at face value. To me, it clearly applies to pdx_to_pfn. Roberto and the team at Bugseng came to the same conclusion. On the other end, I think a) is important. Older version of GCC don't clarify the behavior toward global variables. From the documentation, I would use __attribute_pure__ only with GCC 9 or later. Which is why we need the #define. > > The goal is for the #define to have exactly the same meaning as the gcc > > definition from gcc 9 onward. However, other versions of gcc or other > > compilers could have different semantics. Also we might not want to > > allow gcc to perform the optimizations that it might want to do if the > > attribute is passed. > > > > So the definition would be clear and 100% aligned with the modern gcc > > definition. However we would be able to control the behavior better. > > If we feared older gcc didn't implement "pure" suitably, we should > simply make __attribute_pure__ expand to nothing there. (Still use of > the attribute then would need limiting to cases where it can validly > be applied.) That's fine by me
On 27.02.2024 00:48, Stefano Stabellini wrote: > On Mon, 26 Feb 2024, Jan Beulich wrote: >> On 23.02.2024 23:36, Stefano Stabellini wrote: >>> On Fri, 23 Feb 2024, Jan Beulich wrote: >>>> 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. >>> >>> It doesn't have a different meaning. If it had a different meaning I'd >>> agree with you. >> >> Then we need to sort this aspect first: If there was no difference in >> meaning, we ought to be using the real attribute, not a pseudo >> surrogate. Yet the earlier discussion, according to my understanding, >> has led to the understanding that for the given example the real >> attribute cannot be applied entirely legitimately. Hence why the >> thinking of alternatives actually started. What am I missing? > > There are two different questions: > 1) using __attribute_pure__ in general when appropriate > 2) using __attribute_pure__ in pdx_to_pfn as this patch does > > > I was talking about 1): as a general approach it looks like a good idea > to use __attribute_pure__ when possible and appropriate. > > Now let's talk about 2). The latest definition of __attribute_pure__ is: > > """ > The pure attribute prohibits a function from modifying the state of the program that is observable by means other than inspecting the function’s return value. 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. > """ > > So there are two interesting issues: > > a) While this documentation explicitly allows for reading global vars, > older versions of the docs are less clear. What do we do about them? > > b) Jan wrote that he interprets the statements above to be only valid in > a single-threaded environment > > > To be honest, I am not convinced by b). Jan, is there a statement in the > GCC docs that says that all the attributes (pure being one of them) only > apply to a single-thread environment? It would need to be the other way around, I'm afraid: C99 defines its abstract machine in a way not even considering possible parallel execution (except for other external agents, e.g. when "volatile" is necessary for memory accesses when that memory may also be modified by such an external agent, e.g. a device on the bus). Hence we'd need to find an explicit statement in gcc docs which relaxes that globally or for certain aspects. > That would be extremely limiting > for something like __attribute_pure__. I think we should take the > documentation of attribute pure at face value. To me, it clearly applies > to pdx_to_pfn. Roberto and the team at Bugseng came to the same > conclusion. > > On the other end, I think a) is important. Older version of GCC don't > clarify the behavior toward global variables. From the documentation, I > would use __attribute_pure__ only with GCC 9 or later. Which is why we > need the #define. Right, this is a position we can take. As said, I think we'd then limit ourselves more than necessary. Otoh the number of people using gcc8 or older to build up-to-date Xen should be constantly decreasing ... Jan >>> The goal is for the #define to have exactly the same meaning as the gcc >>> definition from gcc 9 onward. However, other versions of gcc or other >>> compilers could have different semantics. Also we might not want to >>> allow gcc to perform the optimizations that it might want to do if the >>> attribute is passed. >>> >>> So the definition would be clear and 100% aligned with the modern gcc >>> definition. However we would be able to control the behavior better. >> >> If we feared older gcc didn't implement "pure" suitably, we should >> simply make __attribute_pure__ expand to nothing there. (Still use of >> the attribute then would need limiting to cases where it can validly >> be applied.) > > That's fine by me
On Tue, 27 Feb 2024, Jan Beulich wrote: > On 27.02.2024 00:48, Stefano Stabellini wrote: > > On Mon, 26 Feb 2024, Jan Beulich wrote: > >> On 23.02.2024 23:36, Stefano Stabellini wrote: > >>> On Fri, 23 Feb 2024, Jan Beulich wrote: > >>>> 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. > >>> > >>> It doesn't have a different meaning. If it had a different meaning I'd > >>> agree with you. > >> > >> Then we need to sort this aspect first: If there was no difference in > >> meaning, we ought to be using the real attribute, not a pseudo > >> surrogate. Yet the earlier discussion, according to my understanding, > >> has led to the understanding that for the given example the real > >> attribute cannot be applied entirely legitimately. Hence why the > >> thinking of alternatives actually started. What am I missing? > > > > There are two different questions: > > 1) using __attribute_pure__ in general when appropriate > > 2) using __attribute_pure__ in pdx_to_pfn as this patch does > > > > > > I was talking about 1): as a general approach it looks like a good idea > > to use __attribute_pure__ when possible and appropriate. > > > > Now let's talk about 2). The latest definition of __attribute_pure__ is: > > > > """ > > The pure attribute prohibits a function from modifying the state of the program that is observable by means other than inspecting the function’s return value. 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. > > """ > > > > So there are two interesting issues: > > > > a) While this documentation explicitly allows for reading global vars, > > older versions of the docs are less clear. What do we do about them? > > > > b) Jan wrote that he interprets the statements above to be only valid in > > a single-threaded environment > > > > > > To be honest, I am not convinced by b). Jan, is there a statement in the > > GCC docs that says that all the attributes (pure being one of them) only > > apply to a single-thread environment? > > It would need to be the other way around, I'm afraid: C99 defines its > abstract machine in a way not even considering possible parallel > execution (except for other external agents, e.g. when "volatile" is > necessary for memory accesses when that memory may also be modified > by such an external agent, e.g. a device on the bus). Hence we'd need > to find an explicit statement in gcc docs which relaxes that globally > or for certain aspects. I don't think so: the way C99 defines its abstract machine is not tightly coupled with the way gcc extensions are defined. Roberto, you have worked with gcc extensions quite a bit, what's your take on this? Other maintainers, please express your view. > > That would be extremely limiting > > for something like __attribute_pure__. I think we should take the > > documentation of attribute pure at face value. To me, it clearly applies > > to pdx_to_pfn. Roberto and the team at Bugseng came to the same > > conclusion. > > > > On the other end, I think a) is important. Older version of GCC don't > > clarify the behavior toward global variables. From the documentation, I > > would use __attribute_pure__ only with GCC 9 or later. Which is why we > > need the #define. > > Right, this is a position we can take. As said, I think we'd then limit > ourselves more than necessary. Otoh the number of people using gcc8 or > older to build up-to-date Xen should be constantly decreasing ... That is true, but I think it is an OK limitation to have.
diff --git a/xen/include/xen/pdx.h b/xen/include/xen/pdx.h index f3fbc4273a..5d1050967a 100644 --- 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); diff --git a/xen/include/xen/typesafe.h b/xen/include/xen/typesafe.h index 7ecd3b4a8d..615df6f944 100644 --- a/xen/include/xen/typesafe.h +++ b/xen/include/xen/typesafe.h @@ -21,8 +21,8 @@ #define TYPE_SAFE(_type, _name) \ typedef struct { _type _name; } _name##_t; \ - static inline _name##_t _##_name(_type n) { return (_name##_t) { n }; } \ - static inline _type _name##_x(_name##_t n) { return n._name; } + static inline __attribute_const__ _name##_t _##_name(_type n) { return (_name##_t) { n }; } \ + static inline __attribute_const__ _type _name##_x(_name##_t n) { return n._name; } #else
Add const and pure attributes to address reports of MISRA C:2012 Rule 13.1: Initializer lists shall not contain persistent side effects Add pure attribute to function pdx_to_pfn. Add const attribute to functions generated by TYPE_SAFE. These functions are used in initializer lists: adding the attributes ensures that no effect will be performed by them. Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> --- Function attributes pure and const do not need to be added as GCC attributes, they can be added using ECLAIR configurations. --- xen/include/xen/pdx.h | 2 +- xen/include/xen/typesafe.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)