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.stu...@linaro.org> wrote:
> >> On Thu, Feb 23, 2017 at 10:43 AM, Laura Abbott <labb...@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.
Why not simply get rid of the ____ilog2_NaN thing altogether?
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
