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

Commit Message

Ard Biesheuvel Feb. 1, 2017, 6:19 p.m. UTC

On 1 February 2017 at 17:36, Ard Biesheuvel <[e-mail]> wrote:
> On 1 February 2017 at 16:58, Laura Abbott <[e-mail]> 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
>>>> <[e-mail]> 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:

Comments

Joe Perches Feb. 1, 2017, 7:04 p.m. UTC | #1

On Wed, 2017-02-01 at 18:19 +0000, Ard Biesheuvel wrote:
> On 1 February 2017 at 17:36, Ard Biesheuvel <[e-mail]> 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?

Ard Biesheuvel Feb. 1, 2017, 7:31 p.m. UTC | #2

On 1 February 2017 at 19:04, Joe Perches <[e-mail]> wrote:
> On Wed, 2017-02-01 at 18:19 +0000, Ard Biesheuvel wrote:
>> On 1 February 2017 at 17:36, Ard Biesheuvel <[e-mail]> 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.

Joe Perches Feb. 1, 2017, 7:49 p.m. UTC | #3

On Wed, 2017-02-01 at 19:31 +0000, Ard Biesheuvel wrote:
> On 1 February 2017 at 19:04, Joe Perches <[e-mail]> wrote:
> > On Wed, 2017-02-01 at 18:19 +0000, Ard Biesheuvel wrote:
> > > On 1 February 2017 at 17:36, Ard Biesheuvel <[e-mail]> 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);			\
})

Ard Biesheuvel Feb. 1, 2017, 7:53 p.m. UTC | #4

On 1 February 2017 at 19:49, Joe Perches <[e-mail]> wrote:
> On Wed, 2017-02-01 at 19:31 +0000, Ard Biesheuvel wrote:
>> On 1 February 2017 at 19:04, Joe Perches <[e-mail]> wrote:
>> > On Wed, 2017-02-01 at 18:19 +0000, Ard Biesheuvel wrote:
>> > > On 1 February 2017 at 17:36, Ard Biesheuvel <[e-mail]> 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.

Joe Perches Feb. 1, 2017, 8:34 p.m. UTC | #5

On Wed, 2017-02-01 at 19:53 +0000, Ard Biesheuvel wrote:
> On 1 February 2017 at 19:49, Joe Perches <[e-mail]> 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?

Ard Biesheuvel Feb. 1, 2017, 9:11 p.m. UTC | #6

On 1 February 2017 at 20:34, Joe Perches <[e-mail]> wrote:
> On Wed, 2017-02-01 at 19:53 +0000, Ard Biesheuvel wrote:
>> On 1 February 2017 at 19:49, Joe Perches <[e-mail]> 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()

Peter Zijlstra Feb. 2, 2017, 9:02 a.m. UTC | #7

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?

Patch

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 */