From: Markus Trippelsdorf <markus@trippelsdorf.de>
To: Laura Abbott <labbott@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
John Stultz <john.stultz@linaro.org>,
Thomas Gleixner <tglx@linutronix.de>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: gcc7 log2 compile issues in kernel/time/timekeeping.c
Date: Sat, 25 Feb 2017 09:18:10 +0100 [thread overview]
Message-ID: <20170225081810.GA1364@x4> (raw)
In-Reply-To: <51e768e7-91af-86c5-3732-2e529364d376@redhat.com>
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
next prev parent reply other threads:[~2017-02-25 8:18 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-23 18:43 gcc7 log2 compile issues in kernel/time/timekeeping.c Laura Abbott
2017-02-24 21:25 ` John Stultz
2017-02-24 21:45 ` Ard Biesheuvel
2017-02-24 23:33 ` Laura Abbott
2017-02-25 8:18 ` Markus Trippelsdorf [this message]
2017-02-25 9:11 ` Ard Biesheuvel
2017-02-25 11:09 ` Markus Trippelsdorf
2017-02-25 11:23 ` Ard Biesheuvel
2017-02-25 11:50 ` Ard Biesheuvel
2017-03-01 0:00 ` Laura Abbott
2017-03-01 17:39 ` Ard Biesheuvel
2017-03-02 10:11 ` Markus Trippelsdorf
2017-03-02 10:38 ` Ard Biesheuvel
2017-03-02 20:19 ` Linus Torvalds
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170225081810.GA1364@x4 \
--to=markus@trippelsdorf.de \
--cc=ard.biesheuvel@linaro.org \
--cc=john.stultz@linaro.org \
--cc=labbott@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).