Re: Build failure with v4.9-rc1 and GCC trunk -- compiler weirdness

[Joe Perches]

On Wed, 2017-02-01 at 19:31 +0000, Ard Biesheuvel wrote:
> On 1 February 2017 at 19:04, Joe Perches <joe@xxxxxxxxxxx> wrote:
> > On Wed, 2017-02-01 at 18:19 +0000, Ard Biesheuvel wrote:
> > > On 1 February 2017 at 17:36, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> 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);			\
})