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

[Ard Biesheuvel]

On 25 February 2017 at 08:18, Markus Trippelsdorf
<markus@xxxxxxxxxxxxxxx> wrote:
> On 2017.02.24 at 15:33 -0800, Laura Abbott wrote:
>> On 02/24/2017 01:45 PM, Ard Biesheuvel wrote:
>> > On 24 February 2017 at 21:25, John Stultz <john.stultz@xxxxxxxxxx> wrote:
>> >> On Thu, Feb 23, 2017 at 10:43 AM, Laura Abbott <labbott@xxxxxxxxxx> 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.
>
> Why not simply get rid of the ____ilog2_NaN thing altogether?
>

That would remove the issue, sure. But we lose an opportunity to spot
incorrect code at compile time.

My concern is that it by not pushing back on changes to the semantics
of __builtin_constant_p() such as this one, we may start seeing other
issues where we can no longer use it, and we lose a very useful tool.

-- 
Ard.


> diff --git a/include/linux/log2.h b/include/linux/log2.h
> index ef3d4f67118c..07ef24eedf83 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)                          \
>
> --
> Markus