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

[Laura Abbott <labbott@redhat.com>]

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