Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752836AbdCASPE (ORCPT ); Wed, 1 Mar 2017 13:15:04 -0500 Received: from mail-io0-f174.google.com ([209.85.223.174]:32794 "EHLO mail-io0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751088AbdCASPA (ORCPT ); Wed, 1 Mar 2017 13:15:00 -0500 MIME-Version: 1.0 In-Reply-To: <3b82c2d6-7abc-d533-0a65-6e72298cf639@redhat.com> References: <9c74d635-d0d1-0893-8093-ce20b0933fc7@redhat.com> <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> From: Ard Biesheuvel Date: Wed, 1 Mar 2017 17:39:32 +0000 Message-ID: Subject: Re: gcc7 log2 compile issues in kernel/time/timekeeping.c To: Laura Abbott Cc: Markus Trippelsdorf , 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 v21IFYcQ025969 Content-Length: 2743 Lines: 70 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