Re: Build failure with v4.9-rc1 and GCC trunk -- compiler weirdness
From: Ard Biesheuvel
Date: Thu Feb 02 2017 - 10:45:53 EST
On 2 February 2017 at 15:43, Laura Abbott <labbott@xxxxxxxxxx> wrote:
> On 02/02/2017 01:17 AM, Ard Biesheuvel wrote:
>> 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?
>>
>
> 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.