Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

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

229 views
Skip to first unread message

John Stultz

unread,
Feb 24, 2017, 4:30:05 PM2/24/17
to
On Thu, Feb 23, 2017 at 10:43 AM, Laura Abbott <lab...@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?

thanks
-john

Ard Biesheuvel

unread,
Feb 24, 2017, 4:50:05 PM2/24/17
to
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 : \

Laura Abbott

unread,
Feb 24, 2017, 6:40:04 PM2/24/17
to
No, still see the same issue.

Thanks,
Laura

Markus Trippelsdorf

unread,
Feb 25, 2017, 3:20:06 AM2/25/17
to
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

Ard Biesheuvel

unread,
Feb 25, 2017, 4:20:04 AM2/25/17
to
On 25 February 2017 at 08:18, Markus Trippelsdorf
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.

Markus Trippelsdorf

unread,
Feb 25, 2017, 6:20:04 AM2/25/17
to
On 2017.02.25 at 09:11 +0000, Ard Biesheuvel wrote:
> On 25 February 2017 at 08:18, Markus Trippelsdorf <mar...@trippelsdorf.de> wrote:
> >
> > 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.

In the case of kernel/time/timekeeping.c it is clearly a false positive.
Was ever incorrect code spotted by ____ilog2_NaN in the past?

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

We had a long discussion in:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=72785
As you can see there is no real consensus.
But ilog2 seems to be the only place where this ever popped up.
(There were several distro-wide mass rebuilds with gcc-7 and no other
__builtin_constant_p() issue was found yet.)

--
Markus

Ard Biesheuvel

unread,
Feb 25, 2017, 6:40:05 AM2/25/17
to
On 25 February 2017 at 11:09, Markus Trippelsdorf
Well, given that it is really dead code that is being emitted, and
that log2(0) is really undefined, perhaps we should simply replace
ilog2_NaN() with __builtin_unreachable()?

Ard Biesheuvel

unread,
Feb 25, 2017, 7:00:05 AM2/25/17
to
... or perhaps it is better to just pass the constant == 0 to the runtime implementation?

The second ilog2_NaN is really unreachable, given that it deals with unsigned values >0 without a single bit set.

Laura Abbott

unread,
Feb 28, 2017, 8:20:05 PM2/28/17
to
naively throwing in __builtin_unreachable() doesn't seem to
work:

./include/linux/log2.h: In function ‘__order_base_2’:
./include/linux/log2.h:155:10: error: void value not ignored as it ought to be

I'm guessing unreachable is treated as void instead of all
possible types and therefore gcc assumes that the entire
function must be void?

Thanks,
Laura

Ard Biesheuvel

unread,
Mar 1, 2017, 1:20:05 PM3/1/17
to
Something like this perhaps? This will at least prevent incorrect uses
from being silently ignored, but maybe it is a bit overkill.

diff --git a/include/linux/log2.h b/include/linux/log2.h
index ef3d4f67118c..c670b3dfd5ca 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h
@@ -18,8 +18,8 @@
/*
* deal with unrepresentable constant logarithms
*/
-extern __attribute__((const, noreturn))
-int ____ilog2_NaN(void);
+static noinline __attribute__((noreturn, warning("ilog2(0) is undefined!")))
+int ____ilog2_NaN(void) { unreachable(); }

Ard Biesheuvel

unread,
Mar 2, 2017, 6:10:04 AM3/2/17
to
On 2 March 2017 at 10:11, Markus Trippelsdorf <mar...@trippelsdorf.de> wrote:
> Hmm, this will result in the following warning.
>
> In file included from ./include/linux/kernel.h:11:0,
> from ./include/linux/list.h:8,
> from ./include/linux/preempt.h:10,
> from ./include/linux/spinlock.h:50,
> from ./include/linux/seqlock.h:35,
> from ./include/linux/time.h:5,
> from ./include/uapi/linux/timex.h:56,
> from ./include/linux/timex.h:56,
> from ./include/linux/clocksource.h:12,
> from ./include/linux/timekeeper_internal.h:9,
> from kernel/time/timekeeping.c:11:
> kernel/time/timekeeping.c: In function ‘update_wall_time’:
> ./include/linux/log2.h:88:29: warning: call to ‘____ilog2_NaN’ declared with attribute warning: ilog2(0) is undefined!
> __builtin_constant_p(n) ? ( \
> ~~~~
> (n) < 1 ? ____ilog2_NaN() : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
> (n) & (1ULL << 63) ? 63 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 62) ? 62 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 61) ? 61 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 60) ? 60 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 59) ? 59 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 58) ? 58 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 57) ? 57 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 56) ? 56 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 55) ? 55 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 54) ? 54 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 53) ? 53 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 52) ? 52 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 51) ? 51 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 50) ? 50 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 49) ? 49 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 48) ? 48 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 47) ? 47 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 46) ? 46 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 45) ? 45 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 44) ? 44 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 43) ? 43 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 42) ? 42 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 41) ? 41 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 40) ? 40 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 39) ? 39 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 38) ? 38 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 37) ? 37 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 36) ? 36 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 35) ? 35 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 34) ? 34 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 33) ? 33 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 32) ? 32 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 31) ? 31 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 30) ? 30 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 29) ? 29 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 28) ? 28 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 27) ? 27 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 26) ? 26 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 25) ? 25 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 24) ? 24 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 23) ? 23 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 22) ? 22 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 21) ? 21 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 20) ? 20 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 19) ? 19 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 18) ? 18 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 17) ? 17 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 16) ? 16 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 15) ? 15 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 14) ? 14 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 13) ? 13 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 12) ? 12 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 11) ? 11 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 10) ? 10 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 9) ? 9 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 8) ? 8 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 7) ? 7 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 6) ? 6 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 5) ? 5 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 4) ? 4 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 3) ? 3 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 2) ? 2 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 1) ? 1 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> (n) & (1ULL << 0) ? 0 : \
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ____ilog2_NaN() \
> ~~~~~~~~~~~~~~~~~~~
> ) : \
> ~
> kernel/time/timekeeping.c:2051:10: note: in expansion of macro ‘ilog2’
> shift = ilog2(offset) - ilog2(tk->cycle_interval);
> ^~~~~
>

It is slightly noisier than I expected, but it emphasizes the fact
that it is GCC that is emitting a const '0' into ilog2() and not the
programmer.

Markus Trippelsdorf

unread,
Mar 2, 2017, 9:20:06 AM3/2/17
to
Hmm, this will result in the following warning.

In file included from ./include/linux/kernel.h:11:0,
from ./include/linux/list.h:8,
from ./include/linux/preempt.h:10,
from ./include/linux/spinlock.h:50,
from ./include/linux/seqlock.h:35,
from ./include/linux/time.h:5,
from ./include/uapi/linux/timex.h:56,
from ./include/linux/timex.h:56,
from ./include/linux/clocksource.h:12,
from ./include/linux/timekeeper_internal.h:9,
from kernel/time/timekeeping.c:11:
kernel/time/timekeeping.c: In function ‘update_wall_time’:
./include/linux/log2.h:88:29: warning: call to ‘____ilog2_NaN’ declared with attribute warning: ilog2(0) is undefined!
__builtin_constant_p(n) ? ( \
~~~~
(n) < 1 ? ____ilog2_NaN() : \
~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
(n) & (1ULL << 63) ? 63 : \
~~~~~~~~~~~~~~~~~~~~~~~~~~~
(n) & (1ULL << 62) ? 62 : \
~~~~~~~~~~~~~~~~~~~~~~~~~~~
(n) & (1ULL << 61) ? 61 : \
(n) & (1ULL << 3) ? 3 : \
~~~~~~~~~~~~~~~~~~~~~~~~~~~
(n) & (1ULL << 2) ? 2 : \
~~~~~~~~~~~~~~~~~~~~~~~~~~~
(n) & (1ULL << 1) ? 1 : \
~~~~~~~~~~~~~~~~~~~~~~~~~~~
(n) & (1ULL << 0) ? 0 : \
~~~~~~~~~~~~~~~~~~~~~~~~~~~
____ilog2_NaN() \
~~~~~~~~~~~~~~~~~~~
) : \
~
kernel/time/timekeeping.c:2051:10: note: in expansion of macro ‘ilog2’
shift = ilog2(offset) - ilog2(tk->cycle_interval);
^~~~~

--
Markus

Linus Torvalds

unread,
Mar 2, 2017, 3:30:07 PM3/2/17
to
I can't take this compiler braindamage any more, so I just decided to
remove the ____ilog2_NaN checks, and return 0 for any constant value <
2.

It's commit 474c90156c8d ("give up on gcc ilog2() constant
optimizations") in my tree. I'll push it out later after my usual
build tests (and I have a few other pulls to do).

Linus
0 new messages