Re: gcc7 log2 compile issues in kernel/time/timekeeping.c

Laura Abbott
2017-02-24T23:33:42Z

On 02/24/2017 01:45 PM, Ard Biesheuvel wrote:
> On 24 February 2017 at 21:25, John Stultz <john.stultz@linaro.org> wrote:
>> On Thu, Feb 23, 2017 at 10:43 AM, Laura Abbott <labbott@redhat.com> wrote:
>>> Hi,
>>>
>>> Fedora was previously carrying a workaround for a gcc7 issue reported
>>> on arm64 http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/461597.html.
>>> The workaround got rid of __ilog2_NaN. I dropped the patch this morning
>>> because a proper fix (29905b52fad0 ("log2: make order_base_2() behave
>>> correctly on const input value zero")) was merged. This fixed the arm64
>>> problem linked in the thread but there seems to be another issue in
>>> timekeeping.c:
>>>
>>> /kernel/time/timekeeping.c:2051: undefined reference to `____ilog2_NaN'
>>>
>>> Fedora enables CONFIG_CLOCKSOURCE_VALIDATE_LAST_CYCLE so I think the
>>> compiler is calculating a possible constant of 0 once again.
>>>
>>> Any ideas about a proper fix?
>>
>> Huh. So if I understand this, its because we don't explicit checks for
>> offsec or cycle_interval being zero in:
>>
>> shift = ilog2(offset) - ilog2(tk->cycle_interval);
>>
>> Right?
>>
>> Clearly that case isn't expected to happen, but if it did we'd want
>> the result of ilog2 to return zero. So I'm not sure if that
>> order_base_2() function is maybe the right function to use as it has
>> an explict zero check?
>>
>
> The problem is really that GCC splits off a constant folded clone
> where one of these variables is a constant 0. In the order_base_2()
> case, we could sidestep it by fixing an existing issue with the
> function itself, but in this case, it is ilog2() itself that is
> affected.
>
> Laura, does the below make any difference at all?
>
> diff --git a/include/linux/log2.h b/include/linux/log2.h
> index fd7ff3d91e6a..cf4e5bb662bd 100644
> --- a/include/linux/log2.h
> +++ b/include/linux/log2.h
> @@ -85,7 +85,8 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
> #define ilog2(n) \
> ( \
> __builtin_constant_p(n) ? ( \
> - (n) < 1 ? ____ilog2_NaN() : \
> + __builtin_expect((n) < 1, 0) ? \
> + ____ilog2_NaN() : \
> (n) & (1ULL << 63) ? 63 : \
> (n) & (1ULL << 62) ? 62 : \
> (n) & (1ULL << 61) ? 61 : \
>

No, still see the same issue.

Thanks,
Laura