From owner-svn-src-all@freebsd.org Sat Feb 22 18:06:04 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 D7132247AED; Sat, 22 Feb 2020 18:06:04 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 48Px8X5LLkz3DCq; Sat, 22 Feb 2020 18:06:04 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: from mail-qk1-f169.google.com (mail-qk1-f169.google.com [209.85.222.169]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) (Authenticated sender: kevans) by smtp.freebsd.org (Postfix) with ESMTPSA id A36076643; Sat, 22 Feb 2020 18:06:04 +0000 (UTC) (envelope-from kevans@freebsd.org) Received: by mail-qk1-f169.google.com with SMTP id d11so5039567qko.8; Sat, 22 Feb 2020 10:06:04 -0800 (PST) X-Gm-Message-State: APjAAAWzE6/8CK8Kw3IZq68uqpg2Ftzs99KErky6qSAIQL+qYQoruzfE dBrxJTbL0TuT6Q15f/uAlwbdVOJ/KDMwIrVfbA0= X-Google-Smtp-Source: APXvYqzWvbQCSRrogS2fUeTtnqFH+dPSa8Pieo9TFKlbICm/PrvIBFIDAYoM0zmU1Opz+IFcU1bMXeiQN/F6eVK5Pj0= X-Received: by 2002:a37:717:: with SMTP id 23mr37125404qkh.34.1582394764039; Sat, 22 Feb 2020 10:06:04 -0800 (PST) MIME-Version: 1.0 References: <202002221620.01MGK46E072303@repo.freebsd.org> In-Reply-To: From: Kyle Evans Date: Sat, 22 Feb 2020 12:05:53 -0600 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r358248 - head/sys/vm To: Mateusz Guzik Cc: svn-src-head , svn-src-all , src-committers Content-Type: text/plain; charset="UTF-8" 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 18:06:04 -0000 On Sat, Feb 22, 2020 at 11:18 AM Mateusz Guzik wrote: > > 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 > The ddb one reads like crap at first, but the context does make it a little better- within that context, it's easily proven that all of the reachable cases are already handled. amdtemp looks more like an unexpected bug waiting to happen. > 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")). > I don't see much use in adding a panic for this specific call site - it was deliberately structured for optimization, and the odds of it getting refactored in a way that anything after the switch becomes unreachable unintentionally are likely pretty slim. However, especially given the other cases you've pointed out, I do see the utility in general and I'm preparing a diff that will more generally turn __unreachable into a panic under INVARIANTS instead of dirtying up this particular call-site with the logic. Thanks, Kyle Evans