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

[Ard Biesheuvel]

On 1 February 2017 at 21:50, Laura Abbott <labbott@xxxxxxxxxx> wrote:
> On 02/01/2017 09:36 AM, Ard Biesheuvel wrote:
>> On 1 February 2017 at 16:58, Laura Abbott <labbott@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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