From: Markus Trippelsdorf <markus@trippelsdorf.de>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Laura Abbott <labbott@redhat.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
Will Deacon <will.deacon@arm.com>,
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: Thu, 2 Mar 2017 11:11:15 +0100 [thread overview]
Message-ID: <20170302101115.GB293@x4> (raw)
In-Reply-To: <CAKv+Gu8Vvbzefq9zs1MaKhFxToytfgAV5+A=gEv5Re_Qn0EWzQ@mail.gmail.com>
On 2017.03.01 at 17:39 +0000, Ard Biesheuvel wrote:
> On 1 March 2017 at 00:00, Laura Abbott <labbott@redhat.com> wrote:
> > On 02/25/2017 03:50 AM, Ard Biesheuvel wrote:
> >>
> >>
> >>> On 25 Feb 2017, at 11:23, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >>>
> >>> On 25 February 2017 at 11:09, Markus Trippelsdorf
> >>> <markus@trippelsdorf.de> wrote:
> >>>> On 2017.02.25 at 09:11 +0000, Ard Biesheuvel wrote:
> >>>>>> On 25 February 2017 at 08:18, Markus Trippelsdorf <markus@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.)
> >>>>
> >>>
> >>> 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()?
> >>
> >> ... 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.
> >>
> >
> > 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?
> >
>
> 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(); }
>
> /*
> * non-constant log of base 2 calculators
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);
^~~~~
--
Markus
next prev parent reply other threads:[~2017-03-02 14:13 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
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 [this message]
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=20170302101115.GB293@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 \
--cc=torvalds@linux-foundation.org \
--cc=will.deacon@arm.com \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.