| Message ID | 1396868824-16769-1-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | Superseded |
| Headers | show |
>>> On 07.04.14 at 13:07, <ian.campbell@citrix.com> wrote: > According to http://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html > __builtin_expect has the prototype: > long __builtin_expect (long exp, long c) > > If sizeof(exp) > sizeof(long) then this will effectively mask off the top bits > of exp, meaning that the if in "if (unlikey(x))" will see the masked version, > which might be false when true was expected, likely has the same issue. > > With the x86_32 hypervisor no longer existing this is mostly likely to affect > arm32 builds. A quick grep however shows that all the existing arm32 uses of > both likely and unlikely already pass a boolean. I noticed this with an as yet > unposted patch which did not have this property. Good catch, except that at least in the 4.2 tree we still care for the x86_32 hypervisor, and I already spotted a case having the same issue (in mtrr_var_range_msr_set()). I.e. for the purposes of backporting it might be better to make the statement above a little less tailored to arm32. I wonder how we managed to miss the similar change in Linux - even 2.6.5 already has it that way. Reviewed-by: Jan Beulich <jbeulich@suse.com> > --- a/xen/include/xen/compiler.h > +++ b/xen/include/xen/compiler.h > @@ -7,8 +7,8 @@ > > #define barrier() __asm__ __volatile__("": : :"memory") > > -#define likely(x) __builtin_expect((x),1) > -#define unlikely(x) __builtin_expect((x),0) > +#define likely(x) __builtin_expect(!!(x),1) > +#define unlikely(x) __builtin_expect(!!(x),0) > > #define inline __inline__ > #define always_inline __inline__ __attribute__ ((always_inline))
On Mon, 2014-04-07 at 13:36 +0100, Jan Beulich wrote: > >>> On 07.04.14 at 13:07, <ian.campbell@citrix.com> wrote: > > According to http://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html > > __builtin_expect has the prototype: > > long __builtin_expect (long exp, long c) > > > > If sizeof(exp) > sizeof(long) then this will effectively mask off the top bits > > of exp, meaning that the if in "if (unlikey(x))" will see the masked version, > > which might be false when true was expected, likely has the same issue. > > > > With the x86_32 hypervisor no longer existing this is mostly likely to affect > > arm32 builds. A quick grep however shows that all the existing arm32 uses of > > both likely and unlikely already pass a boolean. I noticed this with an as yet > > unposted patch which did not have this property. > > Good catch, I tore out some hair before I got there of course ;-) > except that at least in the 4.2 tree we still care for the > x86_32 hypervisor, and I already spotted a case having the same > issue (in mtrr_var_range_msr_set()). I.e. for the purposes of > backporting it might be better to make the statement above a little > less tailored to arm32. That's fine by me. Do you mean for me to extend it now or did you want to change it as part of the backport process? Either is fine by me. How about: This is mostly likely to affect x86_32 and arm32 builds. x86_32 is not present on 4.3 onwards and a quick grep of current staging shows that all the existing arm32... <etc as before> ? > I wonder how we managed to miss the similar change in Linux - even > 2.6.5 already has it that way. I gave up looking at v2.12-rc1 (first git commit), but yeah, don't know how we missed it. > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > > --- a/xen/include/xen/compiler.h > > +++ b/xen/include/xen/compiler.h > > @@ -7,8 +7,8 @@ > > > > #define barrier() __asm__ __volatile__("": : :"memory") > > > > -#define likely(x) __builtin_expect((x),1) > > -#define unlikely(x) __builtin_expect((x),0) > > +#define likely(x) __builtin_expect(!!(x),1) > > +#define unlikely(x) __builtin_expect(!!(x),0) > > > > #define inline __inline__ > > #define always_inline __inline__ __attribute__ ((always_inline)) > >
>>> On 07.04.14 at 14:42, <Ian.Campbell@citrix.com> wrote: > On Mon, 2014-04-07 at 13:36 +0100, Jan Beulich wrote: >> >>> On 07.04.14 at 13:07, <ian.campbell@citrix.com> wrote: >> > According to http://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html >> > __builtin_expect has the prototype: >> > long __builtin_expect (long exp, long c) >> > >> > If sizeof(exp) > sizeof(long) then this will effectively mask off the top > bits >> > of exp, meaning that the if in "if (unlikey(x))" will see the masked > version, >> > which might be false when true was expected, likely has the same issue. >> > >> > With the x86_32 hypervisor no longer existing this is mostly likely to > affect >> > arm32 builds. A quick grep however shows that all the existing arm32 uses > of >> > both likely and unlikely already pass a boolean. I noticed this with an as > yet >> > unposted patch which did not have this property. >> >> Good catch, > > I tore out some hair before I got there of course ;-) > >> except that at least in the 4.2 tree we still care for the >> x86_32 hypervisor, and I already spotted a case having the same >> issue (in mtrr_var_range_msr_set()). I.e. for the purposes of >> backporting it might be better to make the statement above a little >> less tailored to arm32. > > That's fine by me. Do you mean for me to extend it now or did you want > to change it as part of the backport process? Either is fine by me. > > How about: > This is mostly likely to affect x86_32 and arm32 builds. x86_32 > is not present on 4.3 onwards and a quick grep of current > staging shows that all the existing arm32... <etc as before> > ? Yes, that reads fine for both -unstable and the backport. Jan
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h index 6e07990..4b3472d 100644 --- a/xen/include/xen/compiler.h +++ b/xen/include/xen/compiler.h @@ -7,8 +7,8 @@ #define barrier() __asm__ __volatile__("": : :"memory") -#define likely(x) __builtin_expect((x),1) -#define unlikely(x) __builtin_expect((x),0) +#define likely(x) __builtin_expect(!!(x),1) +#define unlikely(x) __builtin_expect(!!(x),0) #define inline __inline__ #define always_inline __inline__ __attribute__ ((always_inline))
According to http://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html __builtin_expect has the prototype: long __builtin_expect (long exp, long c) If sizeof(exp) > sizeof(long) then this will effectively mask off the top bits of exp, meaning that the if in "if (unlikey(x))" will see the masked version, which might be false when true was expected, likely has the same issue. With the x86_32 hypervisor no longer existing this is mostly likely to affect arm32 builds. A quick grep however shows that all the existing arm32 uses of both likely and unlikely already pass a boolean. I noticed this with an as yet unposted patch which did not have this property. Also the defintion of likely might not have had the expected affect for cases where a true value > 1 might be passed. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Cc: Keir Fraser <keir@xen.org> Cc: Jan Beulich <jbeulich@suse.com> Cc: Tim Deegan <tim@xen.org> --- xen/include/xen/compiler.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)