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

[Markus Trippelsdorf <markus@trippelsdorf.de>]

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@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.

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