Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751978AbdCBLGr (ORCPT ); Thu, 2 Mar 2017 06:06:47 -0500 Received: from mail-it0-f48.google.com ([209.85.214.48]:36904 "EHLO mail-it0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751790AbdCBLGl (ORCPT ); Thu, 2 Mar 2017 06:06:41 -0500 MIME-Version: 1.0 In-Reply-To: <20170302101115.GB293@x4> References: <51e768e7-91af-86c5-3732-2e529364d376@redhat.com> <20170225081810.GA1364@x4> <20170225110928.GB1364@x4> <4AD8C33C-8B3F-4FE5-993B-C73F334B2507@linaro.org> <3b82c2d6-7abc-d533-0a65-6e72298cf639@redhat.com> <20170302101115.GB293@x4> From: Ard Biesheuvel Date: Thu, 2 Mar 2017 10:38:41 +0000 Message-ID: Subject: Re: gcc7 log2 compile issues in kernel/time/timekeeping.c To: Markus Trippelsdorf Cc: Laura Abbott , Linus Torvalds , Will Deacon , John Stultz , Thomas Gleixner , Linux Kernel Mailing List Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v22B6wMJ011277 Content-Length: 8671 Lines: 231 On 2 March 2017 at 10:11, Markus Trippelsdorf wrote: > On 2017.03.01 at 17:39 +0000, Ard Biesheuvel wrote: >> On 1 March 2017 at 00:00, Laura Abbott wrote: >> > On 02/25/2017 03:50 AM, Ard Biesheuvel wrote: >> >> >> >> >> >>> On 25 Feb 2017, at 11:23, Ard Biesheuvel wrote: >> >>> >> >>> On 25 February 2017 at 11:09, Markus Trippelsdorf >> >>> wrote: >> >>>> On 2017.02.25 at 09:11 +0000, Ard Biesheuvel wrote: >> >>>>>> On 25 February 2017 at 08:18, Markus Trippelsdorf 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); > ^~~~~ > 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.