| Message ID | 20161019162222.GT9193@arm.com |
|---|---|
| State | New |
| Headers | show |
On 10/19/2016 09:22 AM, Will Deacon wrote: > On Wed, Oct 19, 2016 at 09:01:33AM -0700, Linus Torvalds wrote: >> On Wed, Oct 19, 2016 at 8:56 AM, Markus Trippelsdorf >> <markus@trippelsdorf.de> wrote: >>> On 2016.10.19 at 08:55 -0700, Linus Torvalds wrote: >>>> >>>> Well, in the meantime we apparently have to live with it. Unless Will >>>> is using some unreleased gcc version that nobody else is using and we >>>> can just ignore it? >>> >>> Yes, he is using gcc-7 that is unreleased. (It will be released April >>> next year.) >> >> Ahh, self-built? So it's not part of some experimental ARM distro >> setup and this will be annoying lots of people? > > Our friendly compiler guys built it, but it's just a snapshot of trunk, > so it's all heading towards GCC 7.0. AFAIU, the problematic optimisation > is also a mid-end pass, so it would affect other architectures too. > >> If so, still think that we could just get rid of the ____ilog2_NaN() >> thing as it's not _that_ important, but it's certainly not very >> high-priority. Will can do it in his tree too for testing, and it can >> remind people to get the gcc problem fixed. > > I'm carrying the diff below, which fixes arm64 defconfig, but I'm worried > that we might be relying on this trick elsewhere. The arm __bad_cmpxchg > function, for example. > > Will > > --->8 > > diff --git a/include/linux/log2.h b/include/linux/log2.h > index fd7ff3d91e6a..9cf5ad69065d 100644 > --- a/include/linux/log2.h > +++ b/include/linux/log2.h > @@ -16,12 +16,6 @@ > #include <linux/bitops.h> > > /* > - * deal with unrepresentable constant logarithms > - */ > -extern __attribute__((const, noreturn)) > -int ____ilog2_NaN(void); > - > -/* > * non-constant log of base 2 calculators > * - the arch may override these in asm/bitops.h if they can be implemented > * more efficiently than using fls() and fls64() > @@ -85,7 +79,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n) > #define ilog2(n) \ > ( \ > __builtin_constant_p(n) ? ( \ > - (n) < 1 ? ____ilog2_NaN() : \ > + (n) < 1 ? 0 : \ > (n) & (1ULL << 63) ? 63 : \ > (n) & (1ULL << 62) ? 62 : \ > (n) & (1ULL << 61) ? 61 : \ > @@ -149,9 +143,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n) > (n) & (1ULL << 3) ? 3 : \ > (n) & (1ULL << 2) ? 2 : \ > (n) & (1ULL << 1) ? 1 : \ > - (n) & (1ULL << 0) ? 0 : \ > - ____ilog2_NaN() \ > - ) : \ > + 0) : \ > (sizeof(n) <= 4) ? \ > __ilog2_u32(n) : \ > __ilog2_u64(n) \ > @@ -194,7 +186,6 @@ unsigned long __rounddown_pow_of_two(unsigned long n) > * @n: parameter > * > * The first few values calculated by this routine: > - * ob2(0) = 0 > * ob2(1) = 0 > * ob2(2) = 1 > * ob2(3) = 2 > Reviving this thread as gcc 7 has now hit Fedora rawhide and has this same issue. I pulled in the above patch from Will as a temporary work around for building. It didn't look like there was consensus on a permanent solution though from the thread. Thanks, Laura
On 1 February 2017 at 16:58, Laura Abbott <labbott@redhat.com> wrote: > On 10/19/2016 09:22 AM, Will Deacon wrote: >> On Wed, Oct 19, 2016 at 09:01:33AM -0700, Linus Torvalds wrote: >>> On Wed, Oct 19, 2016 at 8:56 AM, Markus Trippelsdorf >>> <markus@trippelsdorf.de> wrote: >>>> On 2016.10.19 at 08:55 -0700, Linus Torvalds wrote: >>>>> >>>>> Well, in the meantime we apparently have to live with it. Unless Will >>>>> is using some unreleased gcc version that nobody else is using and we >>>>> can just ignore it? >>>> >>>> Yes, he is using gcc-7 that is unreleased. (It will be released April >>>> next year.) >>> >>> Ahh, self-built? So it's not part of some experimental ARM distro >>> setup and this will be annoying lots of people? >> >> Our friendly compiler guys built it, but it's just a snapshot of trunk, >> so it's all heading towards GCC 7.0. AFAIU, the problematic optimisation >> is also a mid-end pass, so it would affect other architectures too. >> >>> If so, still think that we could just get rid of the ____ilog2_NaN() >>> thing as it's not _that_ important, but it's certainly not very >>> high-priority. Will can do it in his tree too for testing, and it can >>> remind people to get the gcc problem fixed. >> >> I'm carrying the diff below, which fixes arm64 defconfig, but I'm worried >> that we might be relying on this trick elsewhere. The arm __bad_cmpxchg >> function, for example. >> >> Will >> >> --->8 >> >> diff --git a/include/linux/log2.h b/include/linux/log2.h >> index fd7ff3d91e6a..9cf5ad69065d 100644 >> --- a/include/linux/log2.h >> +++ b/include/linux/log2.h >> @@ -16,12 +16,6 @@ >> #include <linux/bitops.h> >> >> /* >> - * deal with unrepresentable constant logarithms >> - */ >> -extern __attribute__((const, noreturn)) >> -int ____ilog2_NaN(void); >> - >> -/* >> * non-constant log of base 2 calculators >> * - the arch may override these in asm/bitops.h if they can be implemented >> * more efficiently than using fls() and fls64() >> @@ -85,7 +79,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >> #define ilog2(n) \ >> ( \ >> __builtin_constant_p(n) ? ( \ >> - (n) < 1 ? ____ilog2_NaN() : \ >> + (n) < 1 ? 0 : \ >> (n) & (1ULL << 63) ? 63 : \ >> (n) & (1ULL << 62) ? 62 : \ >> (n) & (1ULL << 61) ? 61 : \ >> @@ -149,9 +143,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >> (n) & (1ULL << 3) ? 3 : \ >> (n) & (1ULL << 2) ? 2 : \ >> (n) & (1ULL << 1) ? 1 : \ >> - (n) & (1ULL << 0) ? 0 : \ >> - ____ilog2_NaN() \ >> - ) : \ >> + 0) : \ >> (sizeof(n) <= 4) ? \ >> __ilog2_u32(n) : \ >> __ilog2_u64(n) \ >> @@ -194,7 +186,6 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >> * @n: parameter >> * >> * The first few values calculated by this routine: >> - * ob2(0) = 0 >> * ob2(1) = 0 >> * ob2(2) = 1 >> * ob2(3) = 2 >> > > Reviving this thread as gcc 7 has now hit Fedora rawhide and has this > same issue. I pulled in the above patch from Will as a temporary work > around for building. It didn't look like there was consensus on a > permanent solution though from the thread. > I still think order_base_2() is broken, since it may invoke roundup_pow_of_two() with an input value that is documented as producing undefined output. I would argue that the below is the correct fix.diff --git a/include/linux/log2.h b/include/linux/log2.h index fd7ff3d91e6a..46523731bec0 100644 --- a/include/linux/log2.h +++ b/include/linux/log2.h @@ -203,6 +203,18 @@ unsigned long __rounddown_pow_of_two(unsigned long n) * ... and so on. */ -#define order_base_2(n) ilog2(roundup_pow_of_two(n)) +static inline __attribute__((__const__)) +unsigned long __order_base_2(unsigned long n) +{ + return n ? 1UL << fls_long(n - 1) : 1; +} + +#define order_base_2(n) \ +( \ + __builtin_constant_p(n) ? ( \ + ((n) < 2) ? (n) : \ + ilog2((n) - 1) + 1) : \ + ilog2(__order_base_2(n)) \ + ) #endif /* _LINUX_LOG2_H */
On 1 February 2017 at 17:36, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 1 February 2017 at 16:58, Laura Abbott <labbott@redhat.com> wrote: >> On 10/19/2016 09:22 AM, Will Deacon wrote: >>> On Wed, Oct 19, 2016 at 09:01:33AM -0700, Linus Torvalds wrote: >>>> On Wed, Oct 19, 2016 at 8:56 AM, Markus Trippelsdorf >>>> <markus@trippelsdorf.de> wrote: >>>>> On 2016.10.19 at 08:55 -0700, Linus Torvalds wrote: >>>>>> >>>>>> Well, in the meantime we apparently have to live with it. Unless Will >>>>>> is using some unreleased gcc version that nobody else is using and we >>>>>> can just ignore it? >>>>> >>>>> Yes, he is using gcc-7 that is unreleased. (It will be released April >>>>> next year.) >>>> >>>> Ahh, self-built? So it's not part of some experimental ARM distro >>>> setup and this will be annoying lots of people? >>> >>> Our friendly compiler guys built it, but it's just a snapshot of trunk, >>> so it's all heading towards GCC 7.0. AFAIU, the problematic optimisation >>> is also a mid-end pass, so it would affect other architectures too. >>> >>>> If so, still think that we could just get rid of the ____ilog2_NaN() >>>> thing as it's not _that_ important, but it's certainly not very >>>> high-priority. Will can do it in his tree too for testing, and it can >>>> remind people to get the gcc problem fixed. >>> >>> I'm carrying the diff below, which fixes arm64 defconfig, but I'm worried >>> that we might be relying on this trick elsewhere. The arm __bad_cmpxchg >>> function, for example. >>> >>> Will >>> >>> --->8 >>> >>> diff --git a/include/linux/log2.h b/include/linux/log2.h >>> index fd7ff3d91e6a..9cf5ad69065d 100644 >>> --- a/include/linux/log2.h >>> +++ b/include/linux/log2.h >>> @@ -16,12 +16,6 @@ >>> #include <linux/bitops.h> >>> >>> /* >>> - * deal with unrepresentable constant logarithms >>> - */ >>> -extern __attribute__((const, noreturn)) >>> -int ____ilog2_NaN(void); >>> - >>> -/* >>> * non-constant log of base 2 calculators >>> * - the arch may override these in asm/bitops.h if they can be implemented >>> * more efficiently than using fls() and fls64() >>> @@ -85,7 +79,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>> #define ilog2(n) \ >>> ( \ >>> __builtin_constant_p(n) ? ( \ >>> - (n) < 1 ? ____ilog2_NaN() : \ >>> + (n) < 1 ? 0 : \ >>> (n) & (1ULL << 63) ? 63 : \ >>> (n) & (1ULL << 62) ? 62 : \ >>> (n) & (1ULL << 61) ? 61 : \ >>> @@ -149,9 +143,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>> (n) & (1ULL << 3) ? 3 : \ >>> (n) & (1ULL << 2) ? 2 : \ >>> (n) & (1ULL << 1) ? 1 : \ >>> - (n) & (1ULL << 0) ? 0 : \ >>> - ____ilog2_NaN() \ >>> - ) : \ >>> + 0) : \ >>> (sizeof(n) <= 4) ? \ >>> __ilog2_u32(n) : \ >>> __ilog2_u64(n) \ >>> @@ -194,7 +186,6 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>> * @n: parameter >>> * >>> * The first few values calculated by this routine: >>> - * ob2(0) = 0 >>> * ob2(1) = 0 >>> * ob2(2) = 1 >>> * ob2(3) = 2 >>> >> >> Reviving this thread as gcc 7 has now hit Fedora rawhide and has this >> same issue. I pulled in the above patch from Will as a temporary work >> around for building. It didn't look like there was consensus on a >> permanent solution though from the thread. >> > > I still think order_base_2() is broken, since it may invoke > roundup_pow_of_two() with an input value that is documented as > producing undefined output. I would argue that the below is the > correct fix. > > diff --git a/include/linux/log2.h b/include/linux/log2.h > index fd7ff3d91e6a..46523731bec0 100644 > --- a/include/linux/log2.h > +++ b/include/linux/log2.h > @@ -203,6 +203,18 @@ unsigned long __rounddown_pow_of_two(unsigned long n) > * ... and so on. > */ > > -#define order_base_2(n) ilog2(roundup_pow_of_two(n)) > +static inline __attribute__((__const__)) > +unsigned long __order_base_2(unsigned long n) > +{ > + return n ? 1UL << fls_long(n - 1) : 1; > +} > + > +#define order_base_2(n) \ > +( \ > + __builtin_constant_p(n) ? ( \ > + ((n) < 2) ? (n) : \ > + ilog2((n) - 1) + 1) : \ > + ilog2(__order_base_2(n)) \ > + ) > > #endif /* _LINUX_LOG2_H */ Actually, there is a still a redundant shift/fls() in there, this is even simpler:diff --git a/include/linux/log2.h b/include/linux/log2.h index fd7ff3d91e6a..4741534bd7af 100644 --- a/include/linux/log2.h +++ b/include/linux/log2.h @@ -203,6 +203,18 @@ unsigned long __rounddown_pow_of_two(unsigned long n) * ... and so on. */ -#define order_base_2(n) ilog2(roundup_pow_of_two(n)) +static inline __attribute__((__const__)) +unsigned long __order_base_2(unsigned long n) +{ + return n > 1 ? ilog2(n - 1) + 1 : 0; +} + +#define order_base_2(n) \ +( \ + __builtin_constant_p(n) ? ( \ + ((n) < 2) ? (n) : \ + ilog2((n) - 1) + 1) : \ + __order_base_2(n) \ + ) #endif /* _LINUX_LOG2_H */
On Wed, 2017-02-01 at 18:19 +0000, Ard Biesheuvel wrote: > On 1 February 2017 at 17:36, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > I still think order_base_2() is broken, since it may invoke > > roundup_pow_of_two() with an input value that is documented as > > producing undefined output. I would argue that the below is the > > correct fix. > > > > diff --git a/include/linux/log2.h b/include/linux/log2.h > > index fd7ff3d91e6a..46523731bec0 100644 > > --- a/include/linux/log2.h > > +++ b/include/linux/log2.h > > @@ -203,6 +203,18 @@ unsigned long __rounddown_pow_of_two(unsigned long n) > > * ... and so on. > > */ > > > > -#define order_base_2(n) ilog2(roundup_pow_of_two(n)) > > +static inline __attribute__((__const__)) > > +unsigned long __order_base_2(unsigned long n) > > +{ > > + return n ? 1UL << fls_long(n - 1) : 1; > > +} > > + > > +#define order_base_2(n) \ > > +( \ > > + __builtin_constant_p(n) ? ( \ > > + ((n) < 2) ? (n) : \ > > + ilog2((n) - 1) + 1) : \ > > + ilog2(__order_base_2(n)) \ > > + ) > > > > #endif /* _LINUX_LOG2_H */ > > Actually, there is a still a redundant shift/fls() in there, this is > even simpler: > > diff --git a/include/linux/log2.h b/include/linux/log2.h > index fd7ff3d91e6a..4741534bd7af 100644 > --- a/include/linux/log2.h > +++ b/include/linux/log2.h > @@ -203,6 +203,18 @@ unsigned long __rounddown_pow_of_two(unsigned long n) > * ... and so on. > */ > > -#define order_base_2(n) ilog2(roundup_pow_of_two(n)) > +static inline __attribute__((__const__)) commonly __attribute_const__ > +unsigned long __order_base_2(unsigned long n) > +{ > + return n > 1 ? ilog2(n - 1) + 1 : 0; > +} > + > +#define order_base_2(n) \ > +( \ > + __builtin_constant_p(n) ? ( \ > + ((n) < 2) ? (n) : \ > + ilog2((n) - 1) + 1) : \ > + __order_base_2(n) \ > + ) Does this work properly when n is a signed negative value?
On 1 February 2017 at 19:04, Joe Perches <joe@perches.com> wrote: > On Wed, 2017-02-01 at 18:19 +0000, Ard Biesheuvel wrote: >> On 1 February 2017 at 17:36, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> > I still think order_base_2() is broken, since it may invoke >> > roundup_pow_of_two() with an input value that is documented as >> > producing undefined output. I would argue that the below is the >> > correct fix. >> > >> > diff --git a/include/linux/log2.h b/include/linux/log2.h >> > index fd7ff3d91e6a..46523731bec0 100644 >> > --- a/include/linux/log2.h >> > +++ b/include/linux/log2.h >> > @@ -203,6 +203,18 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >> > * ... and so on. >> > */ >> > >> > -#define order_base_2(n) ilog2(roundup_pow_of_two(n)) >> > +static inline __attribute__((__const__)) >> > +unsigned long __order_base_2(unsigned long n) >> > +{ >> > + return n ? 1UL << fls_long(n - 1) : 1; >> > +} >> > + >> > +#define order_base_2(n) \ >> > +( \ >> > + __builtin_constant_p(n) ? ( \ >> > + ((n) < 2) ? (n) : \ >> > + ilog2((n) - 1) + 1) : \ >> > + ilog2(__order_base_2(n)) \ >> > + ) >> > >> > #endif /* _LINUX_LOG2_H */ >> >> Actually, there is a still a redundant shift/fls() in there, this is >> even simpler: >> >> diff --git a/include/linux/log2.h b/include/linux/log2.h >> index fd7ff3d91e6a..4741534bd7af 100644 >> --- a/include/linux/log2.h >> +++ b/include/linux/log2.h >> @@ -203,6 +203,18 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >> * ... and so on. >> */ >> >> -#define order_base_2(n) ilog2(roundup_pow_of_two(n)) >> +static inline __attribute__((__const__)) > > commonly __attribute_const__ > ... except in <linux/ilog2.h>, which probably predates that. >> +unsigned long __order_base_2(unsigned long n) >> +{ >> + return n > 1 ? ilog2(n - 1) + 1 : 0; >> +} >> + >> +#define order_base_2(n) \ >> +( \ >> + __builtin_constant_p(n) ? ( \ >> + ((n) < 2) ? (n) : \ >> + ilog2((n) - 1) + 1) : \ >> + __order_base_2(n) \ >> + ) > > Does this work properly when n is a signed negative value? > No, but order_base_2() is explicitly only defined for inputs [0, ->), so its behavior for negative inputs is best left undefined.
On Wed, 2017-02-01 at 19:31 +0000, Ard Biesheuvel wrote: > On 1 February 2017 at 19:04, Joe Perches <joe@perches.com> wrote: > > On Wed, 2017-02-01 at 18:19 +0000, Ard Biesheuvel wrote: > > > On 1 February 2017 at 17:36, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > > > I still think order_base_2() is broken, since it may invoke > > > > roundup_pow_of_two() with an input value that is documented as > > > > producing undefined output. I would argue that the below is the > > > > correct fix. > > > > > > > > diff --git a/include/linux/log2.h b/include/linux/log2.h > > > > index fd7ff3d91e6a..46523731bec0 100644 > > > > --- a/include/linux/log2.h > > > > +++ b/include/linux/log2.h > > > > @@ -203,6 +203,18 @@ unsigned long __rounddown_pow_of_two(unsigned long n) > > > > * ... and so on. > > > > */ > > > > > > > > -#define order_base_2(n) ilog2(roundup_pow_of_two(n)) > > > > +static inline __attribute__((__const__)) > > > > +unsigned long __order_base_2(unsigned long n) > > > > +{ > > > > + return n ? 1UL << fls_long(n - 1) : 1; > > > > +} > > > > + > > > > +#define order_base_2(n) \ > > > > +( \ > > > > + __builtin_constant_p(n) ? ( \ > > > > + ((n) < 2) ? (n) : \ > > > > + ilog2((n) - 1) + 1) : \ > > > > + ilog2(__order_base_2(n)) \ > > > > + ) > > > > > > > > #endif /* _LINUX_LOG2_H */ > > > > > > Actually, there is a still a redundant shift/fls() in there, this is > > > even simpler: > > > > > > diff --git a/include/linux/log2.h b/include/linux/log2.h > > > index fd7ff3d91e6a..4741534bd7af 100644 > > > --- a/include/linux/log2.h > > > +++ b/include/linux/log2.h > > > @@ -203,6 +203,18 @@ unsigned long __rounddown_pow_of_two(unsigned long n) > > > * ... and so on. > > > */ > > > > > > -#define order_base_2(n) ilog2(roundup_pow_of_two(n)) > > > +static inline __attribute__((__const__)) > > > > commonly __attribute_const__ > > > > ... except in <linux/ilog2.h>, which probably predates that. > > > > +unsigned long __order_base_2(unsigned long n) > > > +{ > > > + return n > 1 ? ilog2(n - 1) + 1 : 0; > > > +} > > > + > > > +#define order_base_2(n) \ > > > +( \ > > > + __builtin_constant_p(n) ? ( \ > > > + ((n) < 2) ? (n) : \ > > > + ilog2((n) - 1) + 1) : \ > > > + __order_base_2(n) \ > > > + ) > > > > Does this work properly when n is a signed negative value? > > > > No, but order_base_2() is explicitly only defined for inputs [0, ->), where? > so its behavior for negative inputs is best left undefined. Or maybe add a BUILD_BUG_ON something like: #define order_base_2(n) \ ({ \ typeof(n) _n = n; \ BUILD_BUG_ON(__builtin_constant_p(_n) && _n < 0); \ __builtin_constant_p(_n) ? (_n < 2 ? _n : ilog2((_n) - 1) + 1)) \ : __order_base_2(_n); \ })
On 1 February 2017 at 19:49, Joe Perches <joe@perches.com> wrote: > On Wed, 2017-02-01 at 19:31 +0000, Ard Biesheuvel wrote: >> On 1 February 2017 at 19:04, Joe Perches <joe@perches.com> wrote: >> > On Wed, 2017-02-01 at 18:19 +0000, Ard Biesheuvel wrote: >> > > On 1 February 2017 at 17:36, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> > > > I still think order_base_2() is broken, since it may invoke >> > > > roundup_pow_of_two() with an input value that is documented as >> > > > producing undefined output. I would argue that the below is the >> > > > correct fix. >> > > > >> > > > diff --git a/include/linux/log2.h b/include/linux/log2.h >> > > > index fd7ff3d91e6a..46523731bec0 100644 >> > > > --- a/include/linux/log2.h >> > > > +++ b/include/linux/log2.h >> > > > @@ -203,6 +203,18 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >> > > > * ... and so on. >> > > > */ >> > > > >> > > > -#define order_base_2(n) ilog2(roundup_pow_of_two(n)) >> > > > +static inline __attribute__((__const__)) >> > > > +unsigned long __order_base_2(unsigned long n) >> > > > +{ >> > > > + return n ? 1UL << fls_long(n - 1) : 1; >> > > > +} >> > > > + >> > > > +#define order_base_2(n) \ >> > > > +( \ >> > > > + __builtin_constant_p(n) ? ( \ >> > > > + ((n) < 2) ? (n) : \ >> > > > + ilog2((n) - 1) + 1) : \ >> > > > + ilog2(__order_base_2(n)) \ >> > > > + ) >> > > > >> > > > #endif /* _LINUX_LOG2_H */ >> > > >> > > Actually, there is a still a redundant shift/fls() in there, this is >> > > even simpler: >> > > >> > > diff --git a/include/linux/log2.h b/include/linux/log2.h >> > > index fd7ff3d91e6a..4741534bd7af 100644 >> > > --- a/include/linux/log2.h >> > > +++ b/include/linux/log2.h >> > > @@ -203,6 +203,18 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >> > > * ... and so on. >> > > */ >> > > >> > > -#define order_base_2(n) ilog2(roundup_pow_of_two(n)) >> > > +static inline __attribute__((__const__)) >> > >> > commonly __attribute_const__ >> > >> >> ... except in <linux/ilog2.h>, which probably predates that. >> >> > > +unsigned long __order_base_2(unsigned long n) >> > > +{ >> > > + return n > 1 ? ilog2(n - 1) + 1 : 0; >> > > +} >> > > + >> > > +#define order_base_2(n) \ >> > > +( \ >> > > + __builtin_constant_p(n) ? ( \ >> > > + ((n) < 2) ? (n) : \ >> > > + ilog2((n) - 1) + 1) : \ >> > > + __order_base_2(n) \ >> > > + ) >> > >> > Does this work properly when n is a signed negative value? >> > >> >> No, but order_base_2() is explicitly only defined for inputs [0, ->), > > where? > The comment describes it as follows /** * order_base_2 - calculate the (rounded up) base 2 order of the argument * @n: parameter * * The first few values calculated by this routine: * ob2(0) = 0 * ob2(1) = 0 * ob2(2) = 1 * ob2(3) = 2 * ob2(4) = 2 * ob2(5) = 3 * ... and so on. */ i.e., it defines the output for inputs 0, 1, 2, 3, 4, 5, ..., and not for negative inputs, hence undefined. >> so its behavior for negative inputs is best left undefined. > > Or maybe add a BUILD_BUG_ON something like: > > #define order_base_2(n) \ > ({ \ > typeof(n) _n = n; \ > BUILD_BUG_ON(__builtin_constant_p(_n) && _n < 0); \ > __builtin_constant_p(_n) ? (_n < 2 ? _n : ilog2((_n) - 1) + 1)) \ > : __order_base_2(_n); \ > }) > This would interfere with the ability to use order_base_2() in initializers for global variables.
On Wed, 2017-02-01 at 19:53 +0000, Ard Biesheuvel wrote: > On 1 February 2017 at 19:49, Joe Perches <joe@perches.com> wrote: [] > > Or maybe add a BUILD_BUG_ON something like: > > > > #define order_base_2(n) \ > > ({ \ > > typeof(n) _n = n; \ > > BUILD_BUG_ON(__builtin_constant_p(_n) && _n < 0); \ > > __builtin_constant_p(_n) ? (_n < 2 ? _n : ilog2((_n) - 1) + 1)) \ > > : __order_base_2(_n); \ > > }) > > > > This would interfere with the ability to use order_base_2() in > initializers for global variables. There aren't any as far as I can tell and would using order_base_2() for a global initializer make sense?
On 1 February 2017 at 20:34, Joe Perches <joe@perches.com> wrote: > On Wed, 2017-02-01 at 19:53 +0000, Ard Biesheuvel wrote: >> On 1 February 2017 at 19:49, Joe Perches <joe@perches.com> wrote: > [] >> > Or maybe add a BUILD_BUG_ON something like: >> > >> > #define order_base_2(n) \ >> > ({ \ >> > typeof(n) _n = n; \ >> > BUILD_BUG_ON(__builtin_constant_p(_n) && _n < 0); \ >> > __builtin_constant_p(_n) ? (_n < 2 ? _n : ilog2((_n) - 1) + 1)) \ >> > : __order_base_2(_n); \ >> > }) >> > >> >> This would interfere with the ability to use order_base_2() in >> initializers for global variables. > > There aren't any as far as I can tell and would using > order_base_2() for a global initializer make sense? > Why wouldn't it make sense? In any case, we could also solve this by doing this instead #define order_base_2(n) \ ( \ __builtin_constant_p(n) ? ( \ ((n) == 0 || (n) == 1) ? 0 : \ ilog2((n) - 1) + 1) : \ __order_base_2(n) \ ) which will emit the usual unresolveable __ilog2_NaN reference when constants < 0 are passed to order_base_2()
On 02/01/2017 09:36 AM, Ard Biesheuvel wrote: > On 1 February 2017 at 16:58, Laura Abbott <labbott@redhat.com> wrote: >> On 10/19/2016 09:22 AM, Will Deacon wrote: >>> On Wed, Oct 19, 2016 at 09:01:33AM -0700, Linus Torvalds wrote: >>>> On Wed, Oct 19, 2016 at 8:56 AM, Markus Trippelsdorf >>>> <markus@trippelsdorf.de> wrote: >>>>> On 2016.10.19 at 08:55 -0700, Linus Torvalds wrote: >>>>>> >>>>>> Well, in the meantime we apparently have to live with it. Unless Will >>>>>> is using some unreleased gcc version that nobody else is using and we >>>>>> can just ignore it? >>>>> >>>>> Yes, he is using gcc-7 that is unreleased. (It will be released April >>>>> next year.) >>>> >>>> Ahh, self-built? So it's not part of some experimental ARM distro >>>> setup and this will be annoying lots of people? >>> >>> Our friendly compiler guys built it, but it's just a snapshot of trunk, >>> so it's all heading towards GCC 7.0. AFAIU, the problematic optimisation >>> is also a mid-end pass, so it would affect other architectures too. >>> >>>> If so, still think that we could just get rid of the ____ilog2_NaN() >>>> thing as it's not _that_ important, but it's certainly not very >>>> high-priority. Will can do it in his tree too for testing, and it can >>>> remind people to get the gcc problem fixed. >>> >>> I'm carrying the diff below, which fixes arm64 defconfig, but I'm worried >>> that we might be relying on this trick elsewhere. The arm __bad_cmpxchg >>> function, for example. >>> >>> Will >>> >>> --->8 >>> >>> diff --git a/include/linux/log2.h b/include/linux/log2.h >>> index fd7ff3d91e6a..9cf5ad69065d 100644 >>> --- a/include/linux/log2.h >>> +++ b/include/linux/log2.h >>> @@ -16,12 +16,6 @@ >>> #include <linux/bitops.h> >>> >>> /* >>> - * deal with unrepresentable constant logarithms >>> - */ >>> -extern __attribute__((const, noreturn)) >>> -int ____ilog2_NaN(void); >>> - >>> -/* >>> * non-constant log of base 2 calculators >>> * - the arch may override these in asm/bitops.h if they can be implemented >>> * more efficiently than using fls() and fls64() >>> @@ -85,7 +79,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>> #define ilog2(n) \ >>> ( \ >>> __builtin_constant_p(n) ? ( \ >>> - (n) < 1 ? ____ilog2_NaN() : \ >>> + (n) < 1 ? 0 : \ >>> (n) & (1ULL << 63) ? 63 : \ >>> (n) & (1ULL << 62) ? 62 : \ >>> (n) & (1ULL << 61) ? 61 : \ >>> @@ -149,9 +143,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>> (n) & (1ULL << 3) ? 3 : \ >>> (n) & (1ULL << 2) ? 2 : \ >>> (n) & (1ULL << 1) ? 1 : \ >>> - (n) & (1ULL << 0) ? 0 : \ >>> - ____ilog2_NaN() \ >>> - ) : \ >>> + 0) : \ >>> (sizeof(n) <= 4) ? \ >>> __ilog2_u32(n) : \ >>> __ilog2_u64(n) \ >>> @@ -194,7 +186,6 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>> * @n: parameter >>> * >>> * The first few values calculated by this routine: >>> - * ob2(0) = 0 >>> * ob2(1) = 0 >>> * ob2(2) = 1 >>> * ob2(3) = 2 >>> >> >> Reviving this thread as gcc 7 has now hit Fedora rawhide and has this >> same issue. I pulled in the above patch from Will as a temporary work >> around for building. It didn't look like there was consensus on a >> permanent solution though from the thread. >> > > I still think order_base_2() is broken, since it may invoke > roundup_pow_of_two() with an input value that is documented as > producing undefined output. I would argue that the below is the > correct fix. > > diff --git a/include/linux/log2.h b/include/linux/log2.h > index fd7ff3d91e6a..46523731bec0 100644 > --- a/include/linux/log2.h > +++ b/include/linux/log2.h > @@ -203,6 +203,18 @@ unsigned long __rounddown_pow_of_two(unsigned long n) > * ... and so on. > */ > > -#define order_base_2(n) ilog2(roundup_pow_of_two(n)) > +static inline __attribute__((__const__)) > +unsigned long __order_base_2(unsigned long n) > +{ > + return n ? 1UL << fls_long(n - 1) : 1; > +} > + > +#define order_base_2(n) \ > +( \ > + __builtin_constant_p(n) ? ( \ > + ((n) < 2) ? (n) : \ > + ilog2((n) - 1) + 1) : \ > + ilog2(__order_base_2(n)) \ > + ) > > #endif /* _LINUX_LOG2_H */ > This fixes the problem although the comments should be updated as well. Is it worth fixing this for ilog2 as well just to avoid the link nonsense there as well? Thanks, Laura
On Wed, Feb 01, 2017 at 11:04:54AM -0800, Joe Perches wrote: > > +#define order_base_2(n) \ > > +( \ > > + __builtin_constant_p(n) ? ( \ > > + ((n) < 2) ? (n) : \ > > + ilog2((n) - 1) + 1) : \ > > + __order_base_2(n) \ > > + ) > > Does this work properly when n is a signed negative value? Do you see it returning a complex number?
On 1 February 2017 at 21:50, Laura Abbott <labbott@redhat.com> wrote: > On 02/01/2017 09:36 AM, Ard Biesheuvel wrote: >> On 1 February 2017 at 16:58, Laura Abbott <labbott@redhat.com> wrote: >>> On 10/19/2016 09:22 AM, Will Deacon wrote: >>>> On Wed, Oct 19, 2016 at 09:01:33AM -0700, Linus Torvalds wrote: >>>>> On Wed, Oct 19, 2016 at 8:56 AM, Markus Trippelsdorf >>>>> <markus@trippelsdorf.de> wrote: >>>>>> On 2016.10.19 at 08:55 -0700, Linus Torvalds wrote: >>>>>>> >>>>>>> Well, in the meantime we apparently have to live with it. Unless Will >>>>>>> is using some unreleased gcc version that nobody else is using and we >>>>>>> can just ignore it? >>>>>> >>>>>> Yes, he is using gcc-7 that is unreleased. (It will be released April >>>>>> next year.) >>>>> >>>>> Ahh, self-built? So it's not part of some experimental ARM distro >>>>> setup and this will be annoying lots of people? >>>> >>>> Our friendly compiler guys built it, but it's just a snapshot of trunk, >>>> so it's all heading towards GCC 7.0. AFAIU, the problematic optimisation >>>> is also a mid-end pass, so it would affect other architectures too. >>>> >>>>> If so, still think that we could just get rid of the ____ilog2_NaN() >>>>> thing as it's not _that_ important, but it's certainly not very >>>>> high-priority. Will can do it in his tree too for testing, and it can >>>>> remind people to get the gcc problem fixed. >>>> >>>> I'm carrying the diff below, which fixes arm64 defconfig, but I'm worried >>>> that we might be relying on this trick elsewhere. The arm __bad_cmpxchg >>>> function, for example. >>>> >>>> Will >>>> >>>> --->8 >>>> >>>> diff --git a/include/linux/log2.h b/include/linux/log2.h >>>> index fd7ff3d91e6a..9cf5ad69065d 100644 >>>> --- a/include/linux/log2.h >>>> +++ b/include/linux/log2.h >>>> @@ -16,12 +16,6 @@ >>>> #include <linux/bitops.h> >>>> >>>> /* >>>> - * deal with unrepresentable constant logarithms >>>> - */ >>>> -extern __attribute__((const, noreturn)) >>>> -int ____ilog2_NaN(void); >>>> - >>>> -/* >>>> * non-constant log of base 2 calculators >>>> * - the arch may override these in asm/bitops.h if they can be implemented >>>> * more efficiently than using fls() and fls64() >>>> @@ -85,7 +79,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>>> #define ilog2(n) \ >>>> ( \ >>>> __builtin_constant_p(n) ? ( \ >>>> - (n) < 1 ? ____ilog2_NaN() : \ >>>> + (n) < 1 ? 0 : \ >>>> (n) & (1ULL << 63) ? 63 : \ >>>> (n) & (1ULL << 62) ? 62 : \ >>>> (n) & (1ULL << 61) ? 61 : \ >>>> @@ -149,9 +143,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>>> (n) & (1ULL << 3) ? 3 : \ >>>> (n) & (1ULL << 2) ? 2 : \ >>>> (n) & (1ULL << 1) ? 1 : \ >>>> - (n) & (1ULL << 0) ? 0 : \ >>>> - ____ilog2_NaN() \ >>>> - ) : \ >>>> + 0) : \ >>>> (sizeof(n) <= 4) ? \ >>>> __ilog2_u32(n) : \ >>>> __ilog2_u64(n) \ >>>> @@ -194,7 +186,6 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>>> * @n: parameter >>>> * >>>> * The first few values calculated by this routine: >>>> - * ob2(0) = 0 >>>> * ob2(1) = 0 >>>> * ob2(2) = 1 >>>> * ob2(3) = 2 >>>> >>> >>> Reviving this thread as gcc 7 has now hit Fedora rawhide and has this >>> same issue. I pulled in the above patch from Will as a temporary work >>> around for building. It didn't look like there was consensus on a >>> permanent solution though from the thread. >>> >> >> I still think order_base_2() is broken, since it may invoke >> roundup_pow_of_two() with an input value that is documented as >> producing undefined output. I would argue that the below is the >> correct fix. >> >> diff --git a/include/linux/log2.h b/include/linux/log2.h >> index fd7ff3d91e6a..46523731bec0 100644 >> --- a/include/linux/log2.h >> +++ b/include/linux/log2.h >> @@ -203,6 +203,18 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >> * ... and so on. >> */ >> >> -#define order_base_2(n) ilog2(roundup_pow_of_two(n)) >> +static inline __attribute__((__const__)) >> +unsigned long __order_base_2(unsigned long n) >> +{ >> + return n ? 1UL << fls_long(n - 1) : 1; >> +} >> + >> +#define order_base_2(n) \ >> +( \ >> + __builtin_constant_p(n) ? ( \ >> + ((n) < 2) ? (n) : \ >> + ilog2((n) - 1) + 1) : \ >> + ilog2(__order_base_2(n)) \ >> + ) >> >> #endif /* _LINUX_LOG2_H */ >> > > This fixes the problem although the comments should be updated > as well. This brings order_base_2() in line with the comments, so I am not sure what you'd want to update here? > Is it worth fixing this for ilog2 as well just to avoid > the link nonsense there as well? > ilog2() is truly undefined for input 0, so the link nonsense is justified there imo
On 02/02/2017 01:17 AM, Ard Biesheuvel wrote: > On 1 February 2017 at 21:50, Laura Abbott <labbott@redhat.com> wrote: >> On 02/01/2017 09:36 AM, Ard Biesheuvel wrote: >>> On 1 February 2017 at 16:58, Laura Abbott <labbott@redhat.com> wrote: >>>> On 10/19/2016 09:22 AM, Will Deacon wrote: >>>>> On Wed, Oct 19, 2016 at 09:01:33AM -0700, Linus Torvalds wrote: >>>>>> On Wed, Oct 19, 2016 at 8:56 AM, Markus Trippelsdorf >>>>>> <markus@trippelsdorf.de> wrote: >>>>>>> On 2016.10.19 at 08:55 -0700, Linus Torvalds wrote: >>>>>>>> >>>>>>>> Well, in the meantime we apparently have to live with it. Unless Will >>>>>>>> is using some unreleased gcc version that nobody else is using and we >>>>>>>> can just ignore it? >>>>>>> >>>>>>> Yes, he is using gcc-7 that is unreleased. (It will be released April >>>>>>> next year.) >>>>>> >>>>>> Ahh, self-built? So it's not part of some experimental ARM distro >>>>>> setup and this will be annoying lots of people? >>>>> >>>>> Our friendly compiler guys built it, but it's just a snapshot of trunk, >>>>> so it's all heading towards GCC 7.0. AFAIU, the problematic optimisation >>>>> is also a mid-end pass, so it would affect other architectures too. >>>>> >>>>>> If so, still think that we could just get rid of the ____ilog2_NaN() >>>>>> thing as it's not _that_ important, but it's certainly not very >>>>>> high-priority. Will can do it in his tree too for testing, and it can >>>>>> remind people to get the gcc problem fixed. >>>>> >>>>> I'm carrying the diff below, which fixes arm64 defconfig, but I'm worried >>>>> that we might be relying on this trick elsewhere. The arm __bad_cmpxchg >>>>> function, for example. >>>>> >>>>> Will >>>>> >>>>> --->8 >>>>> >>>>> diff --git a/include/linux/log2.h b/include/linux/log2.h >>>>> index fd7ff3d91e6a..9cf5ad69065d 100644 >>>>> --- a/include/linux/log2.h >>>>> +++ b/include/linux/log2.h >>>>> @@ -16,12 +16,6 @@ >>>>> #include <linux/bitops.h> >>>>> >>>>> /* >>>>> - * deal with unrepresentable constant logarithms >>>>> - */ >>>>> -extern __attribute__((const, noreturn)) >>>>> -int ____ilog2_NaN(void); >>>>> - >>>>> -/* >>>>> * non-constant log of base 2 calculators >>>>> * - the arch may override these in asm/bitops.h if they can be implemented >>>>> * more efficiently than using fls() and fls64() >>>>> @@ -85,7 +79,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>>>> #define ilog2(n) \ >>>>> ( \ >>>>> __builtin_constant_p(n) ? ( \ >>>>> - (n) < 1 ? ____ilog2_NaN() : \ >>>>> + (n) < 1 ? 0 : \ >>>>> (n) & (1ULL << 63) ? 63 : \ >>>>> (n) & (1ULL << 62) ? 62 : \ >>>>> (n) & (1ULL << 61) ? 61 : \ >>>>> @@ -149,9 +143,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>>>> (n) & (1ULL << 3) ? 3 : \ >>>>> (n) & (1ULL << 2) ? 2 : \ >>>>> (n) & (1ULL << 1) ? 1 : \ >>>>> - (n) & (1ULL << 0) ? 0 : \ >>>>> - ____ilog2_NaN() \ >>>>> - ) : \ >>>>> + 0) : \ >>>>> (sizeof(n) <= 4) ? \ >>>>> __ilog2_u32(n) : \ >>>>> __ilog2_u64(n) \ >>>>> @@ -194,7 +186,6 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>>>> * @n: parameter >>>>> * >>>>> * The first few values calculated by this routine: >>>>> - * ob2(0) = 0 >>>>> * ob2(1) = 0 >>>>> * ob2(2) = 1 >>>>> * ob2(3) = 2 >>>>> >>>> >>>> Reviving this thread as gcc 7 has now hit Fedora rawhide and has this >>>> same issue. I pulled in the above patch from Will as a temporary work >>>> around for building. It didn't look like there was consensus on a >>>> permanent solution though from the thread. >>>> >>> >>> I still think order_base_2() is broken, since it may invoke >>> roundup_pow_of_two() with an input value that is documented as >>> producing undefined output. I would argue that the below is the >>> correct fix. >>> >>> diff --git a/include/linux/log2.h b/include/linux/log2.h >>> index fd7ff3d91e6a..46523731bec0 100644 >>> --- a/include/linux/log2.h >>> +++ b/include/linux/log2.h >>> @@ -203,6 +203,18 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>> * ... and so on. >>> */ >>> >>> -#define order_base_2(n) ilog2(roundup_pow_of_two(n)) >>> +static inline __attribute__((__const__)) >>> +unsigned long __order_base_2(unsigned long n) >>> +{ >>> + return n ? 1UL << fls_long(n - 1) : 1; >>> +} >>> + >>> +#define order_base_2(n) \ >>> +( \ >>> + __builtin_constant_p(n) ? ( \ >>> + ((n) < 2) ? (n) : \ >>> + ilog2((n) - 1) + 1) : \ >>> + ilog2(__order_base_2(n)) \ >>> + ) >>> >>> #endif /* _LINUX_LOG2_H */ >>> >> >> This fixes the problem although the comments should be updated >> as well. > > This brings order_base_2() in line with the comments, so I am not sure > what you'd want to update here? > ob2(1) = 1 for the __builtin_constant_p case which doesn't match the comment of ob2(1) = 0. So my statement should actually be is this correct? Thanks, Laura
On 2 February 2017 at 15:43, Laura Abbott <labbott@redhat.com> wrote: > On 02/02/2017 01:17 AM, Ard Biesheuvel wrote: >> On 1 February 2017 at 21:50, Laura Abbott <labbott@redhat.com> wrote: >>> On 02/01/2017 09:36 AM, Ard Biesheuvel wrote: >>>> On 1 February 2017 at 16:58, Laura Abbott <labbott@redhat.com> wrote: >>>>> On 10/19/2016 09:22 AM, Will Deacon wrote: >>>>>> On Wed, Oct 19, 2016 at 09:01:33AM -0700, Linus Torvalds wrote: >>>>>>> On Wed, Oct 19, 2016 at 8:56 AM, Markus Trippelsdorf >>>>>>> <markus@trippelsdorf.de> wrote: >>>>>>>> On 2016.10.19 at 08:55 -0700, Linus Torvalds wrote: >>>>>>>>> >>>>>>>>> Well, in the meantime we apparently have to live with it. Unless Will >>>>>>>>> is using some unreleased gcc version that nobody else is using and we >>>>>>>>> can just ignore it? >>>>>>>> >>>>>>>> Yes, he is using gcc-7 that is unreleased. (It will be released April >>>>>>>> next year.) >>>>>>> >>>>>>> Ahh, self-built? So it's not part of some experimental ARM distro >>>>>>> setup and this will be annoying lots of people? >>>>>> >>>>>> Our friendly compiler guys built it, but it's just a snapshot of trunk, >>>>>> so it's all heading towards GCC 7.0. AFAIU, the problematic optimisation >>>>>> is also a mid-end pass, so it would affect other architectures too. >>>>>> >>>>>>> If so, still think that we could just get rid of the ____ilog2_NaN() >>>>>>> thing as it's not _that_ important, but it's certainly not very >>>>>>> high-priority. Will can do it in his tree too for testing, and it can >>>>>>> remind people to get the gcc problem fixed. >>>>>> >>>>>> I'm carrying the diff below, which fixes arm64 defconfig, but I'm worried >>>>>> that we might be relying on this trick elsewhere. The arm __bad_cmpxchg >>>>>> function, for example. >>>>>> >>>>>> Will >>>>>> >>>>>> --->8 >>>>>> >>>>>> diff --git a/include/linux/log2.h b/include/linux/log2.h >>>>>> index fd7ff3d91e6a..9cf5ad69065d 100644 >>>>>> --- a/include/linux/log2.h >>>>>> +++ b/include/linux/log2.h >>>>>> @@ -16,12 +16,6 @@ >>>>>> #include <linux/bitops.h> >>>>>> >>>>>> /* >>>>>> - * deal with unrepresentable constant logarithms >>>>>> - */ >>>>>> -extern __attribute__((const, noreturn)) >>>>>> -int ____ilog2_NaN(void); >>>>>> - >>>>>> -/* >>>>>> * non-constant log of base 2 calculators >>>>>> * - the arch may override these in asm/bitops.h if they can be implemented >>>>>> * more efficiently than using fls() and fls64() >>>>>> @@ -85,7 +79,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>>>>> #define ilog2(n) \ >>>>>> ( \ >>>>>> __builtin_constant_p(n) ? ( \ >>>>>> - (n) < 1 ? ____ilog2_NaN() : \ >>>>>> + (n) < 1 ? 0 : \ >>>>>> (n) & (1ULL << 63) ? 63 : \ >>>>>> (n) & (1ULL << 62) ? 62 : \ >>>>>> (n) & (1ULL << 61) ? 61 : \ >>>>>> @@ -149,9 +143,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>>>>> (n) & (1ULL << 3) ? 3 : \ >>>>>> (n) & (1ULL << 2) ? 2 : \ >>>>>> (n) & (1ULL << 1) ? 1 : \ >>>>>> - (n) & (1ULL << 0) ? 0 : \ >>>>>> - ____ilog2_NaN() \ >>>>>> - ) : \ >>>>>> + 0) : \ >>>>>> (sizeof(n) <= 4) ? \ >>>>>> __ilog2_u32(n) : \ >>>>>> __ilog2_u64(n) \ >>>>>> @@ -194,7 +186,6 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>>>>> * @n: parameter >>>>>> * >>>>>> * The first few values calculated by this routine: >>>>>> - * ob2(0) = 0 >>>>>> * ob2(1) = 0 >>>>>> * ob2(2) = 1 >>>>>> * ob2(3) = 2 >>>>>> >>>>> >>>>> Reviving this thread as gcc 7 has now hit Fedora rawhide and has this >>>>> same issue. I pulled in the above patch from Will as a temporary work >>>>> around for building. It didn't look like there was consensus on a >>>>> permanent solution though from the thread. >>>>> >>>> >>>> I still think order_base_2() is broken, since it may invoke >>>> roundup_pow_of_two() with an input value that is documented as >>>> producing undefined output. I would argue that the below is the >>>> correct fix. >>>> >>>> diff --git a/include/linux/log2.h b/include/linux/log2.h >>>> index fd7ff3d91e6a..46523731bec0 100644 >>>> --- a/include/linux/log2.h >>>> +++ b/include/linux/log2.h >>>> @@ -203,6 +203,18 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>>> * ... and so on. >>>> */ >>>> >>>> -#define order_base_2(n) ilog2(roundup_pow_of_two(n)) >>>> +static inline __attribute__((__const__)) >>>> +unsigned long __order_base_2(unsigned long n) >>>> +{ >>>> + return n ? 1UL << fls_long(n - 1) : 1; >>>> +} >>>> + >>>> +#define order_base_2(n) \ >>>> +( \ >>>> + __builtin_constant_p(n) ? ( \ >>>> + ((n) < 2) ? (n) : \ >>>> + ilog2((n) - 1) + 1) : \ >>>> + ilog2(__order_base_2(n)) \ >>>> + ) >>>> >>>> #endif /* _LINUX_LOG2_H */ >>>> >>> >>> This fixes the problem although the comments should be updated >>> as well. >> >> This brings order_base_2() in line with the comments, so I am not sure >> what you'd want to update here? >> > > ob2(1) = 1 for the __builtin_constant_p case which doesn't > match the comment of ob2(1) = 0. So my statement should actually > be is this correct? > > Thanks, > Laura You are right. But the final one I proposed is correct: #define order_base_2(n) \ ( \ __builtin_constant_p(n) ? ( \ ((n) == 0 || (n) == 1) ? 0 : \ ilog2((n) - 1) + 1) : \ __order_base_2(n) \ ) and solves Joe's issue as well. I will submit this as a proper patch. Thanks, Ard.
diff --git a/include/linux/log2.h b/include/linux/log2.h index fd7ff3d91e6a..9cf5ad69065d 100644 --- a/include/linux/log2.h +++ b/include/linux/log2.h @@ -16,12 +16,6 @@ #include <linux/bitops.h> /* - * deal with unrepresentable constant logarithms - */ -extern __attribute__((const, noreturn)) -int ____ilog2_NaN(void); - -/* * non-constant log of base 2 calculators * - the arch may override these in asm/bitops.h if they can be implemented * more efficiently than using fls() and fls64() @@ -85,7 +79,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n) #define ilog2(n) \ ( \ __builtin_constant_p(n) ? ( \ - (n) < 1 ? ____ilog2_NaN() : \ + (n) < 1 ? 0 : \ (n) & (1ULL << 63) ? 63 : \ (n) & (1ULL << 62) ? 62 : \ (n) & (1ULL << 61) ? 61 : \ @@ -149,9 +143,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n) (n) & (1ULL << 3) ? 3 : \ (n) & (1ULL << 2) ? 2 : \ (n) & (1ULL << 1) ? 1 : \ - (n) & (1ULL << 0) ? 0 : \ - ____ilog2_NaN() \ - ) : \ + 0) : \ (sizeof(n) <= 4) ? \ __ilog2_u32(n) : \ __ilog2_u64(n) \ @@ -194,7 +186,6 @@ unsigned long __rounddown_pow_of_two(unsigned long n) * @n: parameter * * The first few values calculated by this routine: - * ob2(0) = 0 * ob2(1) = 0 * ob2(2) = 1 * ob2(3) = 2