From owner-svn-src-all@freebsd.org Sat Feb 22 17:18:42 2020 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id F28C3245B0D; Sat, 22 Feb 2020 17:18:42 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wm1-x331.google.com (mail-wm1-x331.google.com [IPv6:2a00:1450:4864:20::331]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 48Pw5t5tPVz4BGh; Sat, 22 Feb 2020 17:18:42 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wm1-x331.google.com with SMTP id q9so4934765wmj.5; Sat, 22 Feb 2020 09:18:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=SruA3ng4et92XslFExa0wRujb2kg7LPlACitx+qsKQ0=; b=BOy/NqTru918vtNwViU2Vj9ZEg4fYHgT2Sfi3BvU+24qaBcQTJP5m6i/2GvJdXiWNC DHEnJVvZyCbaKCXC82AS0hod/XQd3VtOQD94KygjF+aCbMcnGDBggOui8x2UEE0NyE0D 12Dxbgejvs7S0KoMXoFCa9O6ov6kkxmk44bEyL9rC4/RBlnIV/n9aGgRqmdlD1uMEI4K FMruJ3XUEXs8FS02SCmQMeIHyxSmCQrqeG6xpPHeE0PheSbFjV2xkY8cHR7vO4rlInfk Q8q+X4zo5jl2Z/ItwgHlP+HuwI2PF4w6r9+AiIfoZjAmXu0HUzAA8lMzCkrQEybu2T/u NKpQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=SruA3ng4et92XslFExa0wRujb2kg7LPlACitx+qsKQ0=; b=fTuGiEks97E32IORuuLM1H4FckTO1KLBK07F6mkLyq5Zgwwz5UwK0Giab9U3ItSnLt lbkGgxXvFbgsKmfS1evxrnqW5zWQ61ygBzOVRWP8CrPO1FDbf4awM7m1t8BLNGX4i1Gc tvTZ8hXyo/Snex2oMT+3Mqs/HIGtlLno1u8nWOnjE1yESksTC1j+swiZ/eznz6x0hgWZ Ik6NDsg0hM71UgKsc1y1OObbb1P/oh/Ia93egoIK8ddPPO8+s38SsGn1j/b1WrtCvcCj ZDYOA5a/E6ihTZAOglmcWQkD/qYBpnEcFYUy5vHGT80UitYeAH5BJBKDvukMTJDVmT0Z TC0A== X-Gm-Message-State: APjAAAV4DvlPNeftvq6CZzM8sVlMXzekOCRoF8Z9vcRrP1PMp1utJeyV pAf3WZu8nK/XcV1Anfi27ennL4AlHPGrkJZT4rC44w== X-Google-Smtp-Source: APXvYqwetvxFh12UaNeaI+dB8XncoeX88FUXaRd3frWR1R6E9eN8KfpuHRhiIC2lTA2mkMEWcO1gKGBdMbjVQRcghnA= X-Received: by 2002:a1c:e483:: with SMTP id b125mr11382403wmh.187.1582391919775; Sat, 22 Feb 2020 09:18:39 -0800 (PST) MIME-Version: 1.0 Received: by 2002:a5d:6b02:0:0:0:0:0 with HTTP; Sat, 22 Feb 2020 09:18:39 -0800 (PST) In-Reply-To: References: <202002221620.01MGK46E072303@repo.freebsd.org> From: Mateusz Guzik Date: Sat, 22 Feb 2020 18:18:39 +0100 Message-ID: Subject: Re: svn commit: r358248 - head/sys/vm To: Kyle Evans Cc: Ian Lepore , svn-src-head , svn-src-all , src-committers Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 48Pw5t5tPVz4BGh X-Spamd-Bar: ----- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-6.00 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-0.999,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 22 Feb 2020 17:18:43 -0000 On 2/22/20, Kyle Evans wrote: > On Sat, Feb 22, 2020 at 10:44 AM Mateusz Guzik wrote: >> >> On 2/22/20, Kyle Evans wrote: >> > On Sat, Feb 22, 2020 at 10:25 AM Ian Lepore wrote: >> >> >> >> On Sat, 2020-02-22 at 16:20 +0000, Kyle Evans wrote: >> >> > Author: kevans >> >> > Date: Sat Feb 22 16:20:04 2020 >> >> > New Revision: 358248 >> >> > URL: https://svnweb.freebsd.org/changeset/base/358248 >> >> > >> >> > Log: >> >> > vm_radix: prefer __builtin_unreachable() to an unreachable panic() >> >> > >> >> > This provides the needed hint to GCC and offers an annotation for >> >> > readers to >> >> > observe that it's in-fact impossible to hit this point. We'll get >> >> > hit >> >> > with a >> >> > a -Wswitch error if the enum applicable to the switch above were >> >> > to >> >> > get >> >> > expanded without the new value(s) being handled. >> >> > >> >> > Modified: >> >> > head/sys/vm/vm_radix.c >> >> > >> >> > Modified: head/sys/vm/vm_radix.c >> >> > ============================================================================== >> >> > --- head/sys/vm/vm_radix.c Sat Feb 22 13:23:27 2020 >> >> > (r358247) >> >> > +++ head/sys/vm/vm_radix.c Sat Feb 22 16:20:04 2020 >> >> > (r358248) >> >> > @@ -208,8 +208,7 @@ vm_radix_node_load(smrnode_t *p, enum >> >> > vm_radix_access >> >> > case SMR: >> >> > return (smr_entered_load(p, vm_radix_smr)); >> >> > } >> >> > - /* This is unreachable, silence gcc. */ >> >> > - panic("vm_radix_node_get: Unknown access type"); >> >> > + __unreachable(); >> >> > } >> >> > >> >> > static __inline void >> >> >> >> What does __unreachable() do if the code ever becomes reachable? Like >> >> if a new enum value is added and this switch doesn't get updated? >> >> >> > >> > __unreachable doesn't help here, but the compiler will error out on >> > the switch() if all enum values aren't addressed and there's no >> > default: case. >> > >> > IMO, compilers could/should become smart enough to error if there's an >> > explicit __builtin_unreachable() and they can trivially determine that >> > all paths will terminate before this, independent of -Werror=switch*. >> > _______________________________________________ >> >> I think this is way too iffy, check this program: >> >> >> #include >> >> int >> main(void) >> { >> >> __builtin_unreachable(); >> printf("test\n"); >> } >> >> Neither clang nor gcc warn about this and both stop code generation >> past the statement. Thus I think for production kernels __unreachable >> can expand to to the builtin, but for debug it should be a panic with >> func/file/line. This would work fine in terms of analysis since panic >> is noreturn or so. > > I guess I'll repeat this again: our build will error out if this > becomes reachable, because we compile with -Werror=switch. There's no > point in having a panic that cannot physically be reached, you will > never see the func/file/line. > The keyword in the current lends itself towards misuse and according to my grep this is precisely what happened in the tree: ddb/db_expr.c: switch(t) { .. default: __unreachable(); } similarly in dev/amdtemp/amdtemp.c Another shady user is in dev/nvdimm/nvdimm.c -- read_label has few loops and the function ends with __unreachable() Seems to be in all these cases the intent was to "warn at compilation time if we ever get here and panic at runtime at least with debug" In contrast, the keyword is to explicitly tell the compiler that given piece of code will never be executed no matter it thinks. Thus the least which can be done is injecting a panic into it, according to the original review which added it https://reviews.freebsd.org/D2536 llvm developers do an equivalent (assert(0 && "blah")). -- Mateusz Guzik