From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_IN_DEF_DKIM_WL autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5A281C64E7C for ; Wed, 25 Nov 2020 12:25:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id F4156206E5 for ; Wed, 25 Nov 2020 12:25:06 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="GyrkIL7r" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729279AbgKYMY4 (ORCPT ); Wed, 25 Nov 2020 07:24:56 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48662 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728548AbgKYMYj (ORCPT ); Wed, 25 Nov 2020 07:24:39 -0500 Received: from mail-pg1-x542.google.com (mail-pg1-x542.google.com [IPv6:2607:f8b0:4864:20::542]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7F654C061A56 for ; Wed, 25 Nov 2020 04:24:39 -0800 (PST) Received: by mail-pg1-x542.google.com with SMTP id s63so2307828pgc.8 for ; Wed, 25 Nov 2020 04:24:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=GyrkIL7rJc/Wrkz9wtYqXZYvGBry6qXFkQono0nmrBFDlUCiGmbX9ByD1wUhih87ZW XCd/8etF0h65aGuVNHVvGVnSoIRV2cIFxWeuMsMEKDZ+SIKsK6eM3KIHPaY2Au+pxfCB jFmSmO0a8jtSnIjbAi/709gkMW9hnqxggrhUNIGI/2GrlejcLn7tyz9MFlSEpE31y19Z 9ARaZhNBbaKZzII6ioDoEFmbbi01XI+4/fF65wWR3SGfZuCMoV2cgGUJ8Osa8sFeqdbz KLllcyBsC3gtRPDbq4Yc+z3inKaZT7D05cYTb7CBHNDdR/afLH0A7E5JHrBWilMAefn4 uS6g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=YpW8wjygkxXKRH3WG5oYagMJzsTEb/JpylXAaC5YTRxeexRdCMU5z8UowTT+xdJCOo bAA1PuUO2T/NXDack9J4YrLsAswhemN2xm/zBpgHD0veoRc4EKrYjn+PfFCjmsqX0WYZ QMNxeuD7UHGctX1dr9gEA6tTpxJ+spsuIE9Mz65p4oE8bd1UI7xvEY0c4EykwIlKrlZ+ dLQlf4DNzR9psoZY9Q0k8dFc8L6GHaHT5L4ujgrW2YFurMz9fLfAeDi9/bCbn9gDmakK Ow2hG3MCkv8+kddv0SNrtklwadEG86+liiOovVdxZsYjn8vBI60EiRHX3qrxIX+prKgD SHtg== X-Gm-Message-State: AOAM530O6T+q2VJ3V9JZ3Pgfb83eq6VpAqbDLP7QScbWDL9Mc7oiVmnf 6B+4jgZYD06A+9HoyY6F/ohlZBvQEPCPwH57u0xjuA== X-Google-Smtp-Source: ABdhPJySo35UzNwHodlreVMfJuWPwHO1z+zkcbFfSYU3Avf+sN4n16LJPBb97SBockWyJEKx3Xs8q1wCvzejZmwrmAM= X-Received: by 2002:a62:7905:0:b029:197:f300:5a2a with SMTP id u5-20020a6279050000b0290197f3005a2amr2898775pfc.30.1606307078380; Wed, 25 Nov 2020 04:24:38 -0800 (PST) MIME-Version: 1.0 References: <202011201129.B13FDB3C@keescook> <20201120115142.292999b2@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> <202011220816.8B6591A@keescook> <9b57fd4914b46f38d54087d75e072d6e947cb56d.camel@HansenPartnership.com> <0147972a72bc13f3629de8a32dee6f1f308994b5.camel@HansenPartnership.com> <20201123130348.GA3119@embeddedor> <8f5611bb015e044fa1c0a48147293923c2d904e4.camel@HansenPartnership.com> <202011241327.BB28F12F6@keescook> In-Reply-To: From: Nick Desaulniers Date: Wed, 25 Nov 2020 04:24:27 -0800 Message-ID: Subject: Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang To: James Bottomley Cc: Kees Cook , "Gustavo A. R. Silva" , Joe Perches , Jakub Kicinski , alsa-devel@alsa-project.org, linux-atm-general@lists.sourceforge.net, reiserfs-devel@vger.kernel.org, linux-iio@vger.kernel.org, linux-wireless , linux-fbdev@vger.kernel.org, dri-devel , LKML , Nathan Chancellor , linux-ide@vger.kernel.org, dm-devel@redhat.com, keyrings@vger.kernel.org, linux-mtd@lists.infradead.org, GR-everest-linux-l2@marvell.com, wcn36xx@lists.infradead.org, samba-technical@lists.samba.org, linux-i3c@lists.infradead.org, linux1394-devel@lists.sourceforge.net, linux-afs@lists.infradead.org, usb-storage@lists.one-eyed-alien.net, drbd-dev@lists.linbit.com, devel@driverdev.osuosl.org, linux-cifs@vger.kernel.org, rds-devel@oss.oracle.com, linux-scsi@vger.kernel.org, linux-rdma@vger.kernel.org, oss-drivers@netronome.com, bridge@lists.linux-foundation.org, linux-security-module@vger.kernel.org, amd-gfx list , linux-stm32@st-md-mailman.stormreply.com, cluster-devel@redhat.com, linux-acpi@vger.kernel.org, coreteam@netfilter.org, intel-wired-lan@lists.osuosl.org, linux-input@vger.kernel.org, Miguel Ojeda , tipc-discussion@lists.sourceforge.net, linux-ext4@vger.kernel.org, linux-media@vger.kernel.org, linux-watchdog@vger.kernel.org, selinux@vger.kernel.org, linux-arm-msm , intel-gfx@lists.freedesktop.org, linux-geode@lists.infradead.org, linux-can@vger.kernel.org, linux-block@vger.kernel.org, linux-gpio@vger.kernel.org, op-tee@lists.trustedfirmware.org, linux-mediatek@lists.infradead.org, xen-devel@lists.xenproject.org, nouveau@lists.freedesktop.org, linux-hams@vger.kernel.org, ceph-devel@vger.kernel.org, virtualization@lists.linux-foundation.org, Linux ARM , linux-hwmon@vger.kernel.org, "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" , linux-nfs@vger.kernel.org, GR-Linux-NIC-Dev@marvell.com, Linux Memory Management List , Network Development , linux-decnet-user@lists.sourceforge.net, linux-mmc@vger.kernel.org, Linux-Renesas , linux-sctp@vger.kernel.org, linux-usb@vger.kernel.org, netfilter-devel@vger.kernel.org, "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , patches@opensource.cirrus.com, linux-integrity@vger.kernel.org, target-devel@vger.kernel.org, linux-hardening@vger.kernel.org, Jonathan Cameron , Greg KH Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org On Tue, Nov 24, 2020 at 11:05 PM James Bottomley wrote: > > On Tue, 2020-11-24 at 13:32 -0800, Kees Cook wrote: > > We already enable -Wimplicit-fallthrough globally, so that's not the > > discussion. The issue is that Clang is (correctly) even more strict > > than GCC for this, so these are the remaining ones to fix for full > > Clang coverage too. > > > > People have spent more time debating this already than it would have > > taken to apply the patches. :) > > You mean we've already spent 90% of the effort to come this far so we > might as well go the remaining 10% because then at least we get some > return? It's certainly a clinching argument in defence procurement ... So developers and distributions using Clang can't have -Wimplicit-fallthrough enabled because GCC is less strict (which has been shown in this thread to lead to bugs)? We'd like to have nice things too, you know. I even agree that most of the churn comes from case 0: ++x; default: break; which I have a patch for: https://reviews.llvm.org/D91895. I agree that can never lead to bugs. But that's not the sole case of this series, just most of them. Though, note how the reviewer (C++ spec editor and clang front end owner) in https://reviews.llvm.org/D91895 even asks in that review how maybe a new flag would be more appropriate for a watered down/stylistic variant of the existing behavior. And if the current wording of Documentation/process/deprecated.rst around "fallthrough" is a straightforward rule of thumb, I kind of agree with him. > > > This is about robustness and language wrangling. It's a big code- > > base, and this is the price of our managing technical debt for > > permanent robustness improvements. (The numbers I ran from Gustavo's > > earlier patches were that about 10% of the places adjusted were > > identified as legitimate bugs being fixed. This final series may be > > lower, but there are still bugs being found from it -- we need to > > finish this and shut the door on it for good.) > > I got my six patches by analyzing the lwn.net report of the fixes that > was cited which had 21 of which 50% didn't actually change the emitted > code, and 25% didn't have a user visible effect. > > But the broader point I'm making is just because the compiler people > come up with a shiny new warning doesn't necessarily mean the problem That's not what this is though; you're attacking a strawman. I'd encourage you to bring that up when that actually occurs, unlike this case since it's actively hindering getting -Wimplicit-fallthrough enabled for Clang. This is not a shiny new warning; it's already on for GCC and has existed in both compilers for multiple releases. And I'll also note that warnings are warnings and not errors because they cannot be proven to be bugs in 100% of cases, but they have led to bugs in the past. They require a human to review their intent and remove ambiguities. If 97% of cases would end in a break ("Expert C Programming: Deep C Secrets" - Peter van der Linden), then it starts to look to me like a language defect; certainly an incorrectly chosen default. But the compiler can't know those 3% were intentional, unless you're explicit for those exceptional cases. > it's detecting is one that causes us actual problems in the code base. > I'd really be happier if we had a theory about what classes of CVE or > bug we could eliminate before we embrace the next new warning. We don't generally file CVEs and waiting for them to occur might be too reactive, but I agree that pointing to some additional documentation in commit messages about how a warning could lead to a bug would make it clearer to reviewers why being able to enable it treewide, even if there's no bug in their particular subsystem, is in the general interest of the commons. On Mon, Nov 23, 2020 at 7:58 AM James Bottomley wrote: > > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 > > The whole crux of your argument seems to be maintainers' time isn't > important so we should accept all trivial patches ... I'm pushing back > on that assumption in two places, firstly the valulessness of the time > and secondly that all trivial patches are valuable. It's critical to the longevity of any open source project that there are not single points of failure. If someone is not expendable or replaceable (or claims to be) then that's a risk to the project and a bottleneck. Not having a replacement in training or some form of redundancy is short sighted. If trivial patches are adding too much to your workload, consider training a co-maintainer or asking for help from one of your reviewers whom you trust. I don't doubt it's hard to find maintainers, but existing maintainers should go out of their way to entrust co-maintainers especially when they find their workload becomes too high. And reviewing/picking up trivial patches is probably a great way to get started. If we allow too much knowledge of any one subsystem to collect with one maintainer, what happens when that maintainer leaves the community (which, given a finite lifespan, is an inevitability)? -- Thanks, ~Nick Desaulniers From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_IN_DEF_DKIM_WL autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 11382C64E7B for ; Wed, 25 Nov 2020 12:24:43 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 709E120715 for ; Wed, 25 Nov 2020 12:24:42 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="GyrkIL7r" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 709E120715 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id BBEC06B0071; Wed, 25 Nov 2020 07:24:41 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B48E06B0072; Wed, 25 Nov 2020 07:24:41 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A10326B0075; Wed, 25 Nov 2020 07:24:41 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0159.hostedemail.com [216.40.44.159]) by kanga.kvack.org (Postfix) with ESMTP id 88D9E6B0071 for ; Wed, 25 Nov 2020 07:24:41 -0500 (EST) Received: from smtpin08.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 4D80D181AEF39 for ; Wed, 25 Nov 2020 12:24:41 +0000 (UTC) X-FDA: 77522859162.08.neck61_29065d727376 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin08.hostedemail.com (Postfix) with ESMTP id 30DA71819E798 for ; Wed, 25 Nov 2020 12:24:41 +0000 (UTC) X-HE-Tag: neck61_29065d727376 X-Filterd-Recvd-Size: 11604 Received: from mail-pf1-f193.google.com (mail-pf1-f193.google.com [209.85.210.193]) by imf09.hostedemail.com (Postfix) with ESMTP for ; Wed, 25 Nov 2020 12:24:40 +0000 (UTC) Received: by mail-pf1-f193.google.com with SMTP id 131so2151706pfb.9 for ; Wed, 25 Nov 2020 04:24:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=GyrkIL7rJc/Wrkz9wtYqXZYvGBry6qXFkQono0nmrBFDlUCiGmbX9ByD1wUhih87ZW XCd/8etF0h65aGuVNHVvGVnSoIRV2cIFxWeuMsMEKDZ+SIKsK6eM3KIHPaY2Au+pxfCB jFmSmO0a8jtSnIjbAi/709gkMW9hnqxggrhUNIGI/2GrlejcLn7tyz9MFlSEpE31y19Z 9ARaZhNBbaKZzII6ioDoEFmbbi01XI+4/fF65wWR3SGfZuCMoV2cgGUJ8Osa8sFeqdbz KLllcyBsC3gtRPDbq4Yc+z3inKaZT7D05cYTb7CBHNDdR/afLH0A7E5JHrBWilMAefn4 uS6g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=chNaquYftCf6Qn/By9bgs0SRzz+PpJr1l0VZnCY6gxbYqhyUXBiJGYdbbHbA6UUkWz 6tP10qQ4khKQjLtOu1pY8ALniZY7hcsIdx8EOWE1ByTU0qxliYX1s2a0ASXI7LrediCW /5iGbmNA38GQDh1/wYmWsNq2Xe/K/vjsX0reKoyZQe6FGcxZ9dkQbXlVfnPND7mKj613 uCc5mOaGbaaP/KqtrgDIXO/Wzl1hemqSQrhHxLe32A7uWsJbsQtf0U58QKvuf2FGO23P 5SeEqsYbgW3AOydczODNhzHn3Rbt3PYIDf6kWdYTptaF1DFkOCIahw6hc37JTKcRDUk4 jfFg== X-Gm-Message-State: AOAM530dCpm64QBGTkjSZcRQcmA3deM3eZm63vlEjpWaX7WMUE0dzGzJ vIcBUs6VRRyu3LDmPqP3/5NG1TdZiyTL7sjjOFnBzA== X-Google-Smtp-Source: ABdhPJySo35UzNwHodlreVMfJuWPwHO1z+zkcbFfSYU3Avf+sN4n16LJPBb97SBockWyJEKx3Xs8q1wCvzejZmwrmAM= X-Received: by 2002:a62:7905:0:b029:197:f300:5a2a with SMTP id u5-20020a6279050000b0290197f3005a2amr2898775pfc.30.1606307078380; Wed, 25 Nov 2020 04:24:38 -0800 (PST) MIME-Version: 1.0 References: <202011201129.B13FDB3C@keescook> <20201120115142.292999b2@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> <202011220816.8B6591A@keescook> <9b57fd4914b46f38d54087d75e072d6e947cb56d.camel@HansenPartnership.com> <0147972a72bc13f3629de8a32dee6f1f308994b5.camel@HansenPartnership.com> <20201123130348.GA3119@embeddedor> <8f5611bb015e044fa1c0a48147293923c2d904e4.camel@HansenPartnership.com> <202011241327.BB28F12F6@keescook> In-Reply-To: From: Nick Desaulniers Date: Wed, 25 Nov 2020 04:24:27 -0800 Message-ID: Subject: Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang To: James Bottomley Cc: Kees Cook , "Gustavo A. R. Silva" , Joe Perches , Jakub Kicinski , alsa-devel@alsa-project.org, linux-atm-general@lists.sourceforge.net, reiserfs-devel@vger.kernel.org, linux-iio@vger.kernel.org, linux-wireless , linux-fbdev@vger.kernel.org, dri-devel , LKML , Nathan Chancellor , linux-ide@vger.kernel.org, dm-devel@redhat.com, keyrings@vger.kernel.org, linux-mtd@lists.infradead.org, GR-everest-linux-l2@marvell.com, wcn36xx@lists.infradead.org, samba-technical@lists.samba.org, linux-i3c@lists.infradead.org, linux1394-devel@lists.sourceforge.net, linux-afs@lists.infradead.org, usb-storage@lists.one-eyed-alien.net, drbd-dev@lists.linbit.com, devel@driverdev.osuosl.org, linux-cifs@vger.kernel.org, rds-devel@oss.oracle.com, linux-scsi@vger.kernel.org, linux-rdma@vger.kernel.org, oss-drivers@netronome.com, bridge@lists.linux-foundation.org, linux-security-module@vger.kernel.org, amd-gfx list , linux-stm32@st-md-mailman.stormreply.com, cluster-devel@redhat.com, linux-acpi@vger.kernel.org, coreteam@netfilter.org, intel-wired-lan@lists.osuosl.org, linux-input@vger.kernel.org, Miguel Ojeda , tipc-discussion@lists.sourceforge.net, linux-ext4@vger.kernel.org, linux-media@vger.kernel.org, linux-watchdog@vger.kernel.org, selinux@vger.kernel.org, linux-arm-msm , intel-gfx@lists.freedesktop.org, linux-geode@lists.infradead.org, linux-can@vger.kernel.org, linux-block@vger.kernel.org, linux-gpio@vger.kernel.org, op-tee@lists.trustedfirmware.org, linux-mediatek@lists.infradead.org, xen-devel@lists.xenproject.org, nouveau@lists.freedesktop.org, linux-hams@vger.kernel.org, ceph-devel@vger.kernel.org, virtualization@lists.linux-foundation.org, Linux ARM , linux-hwmon@vger.kernel.org, "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" , linux-nfs@vger.kernel.org, GR-Linux-NIC-Dev@marvell.com, Linux Memory Management List , Network Development , linux-decnet-user@lists.sourceforge.net, linux-mmc@vger.kernel.org, Linux-Renesas , linux-sctp@vger.kernel.org, linux-usb@vger.kernel.org, netfilter-devel@vger.kernel.org, "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , patches@opensource.cirrus.com, linux-integrity@vger.kernel.org, target-devel@vger.kernel.org, linux-hardening@vger.kernel.org, Jonathan Cameron , Greg KH Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, Nov 24, 2020 at 11:05 PM James Bottomley wrote: > > On Tue, 2020-11-24 at 13:32 -0800, Kees Cook wrote: > > We already enable -Wimplicit-fallthrough globally, so that's not the > > discussion. The issue is that Clang is (correctly) even more strict > > than GCC for this, so these are the remaining ones to fix for full > > Clang coverage too. > > > > People have spent more time debating this already than it would have > > taken to apply the patches. :) > > You mean we've already spent 90% of the effort to come this far so we > might as well go the remaining 10% because then at least we get some > return? It's certainly a clinching argument in defence procurement ... So developers and distributions using Clang can't have -Wimplicit-fallthrough enabled because GCC is less strict (which has been shown in this thread to lead to bugs)? We'd like to have nice things too, you know. I even agree that most of the churn comes from case 0: ++x; default: break; which I have a patch for: https://reviews.llvm.org/D91895. I agree that can never lead to bugs. But that's not the sole case of this series, just most of them. Though, note how the reviewer (C++ spec editor and clang front end owner) in https://reviews.llvm.org/D91895 even asks in that review how maybe a new flag would be more appropriate for a watered down/stylistic variant of the existing behavior. And if the current wording of Documentation/process/deprecated.rst around "fallthrough" is a straightforward rule of thumb, I kind of agree with him. > > > This is about robustness and language wrangling. It's a big code- > > base, and this is the price of our managing technical debt for > > permanent robustness improvements. (The numbers I ran from Gustavo's > > earlier patches were that about 10% of the places adjusted were > > identified as legitimate bugs being fixed. This final series may be > > lower, but there are still bugs being found from it -- we need to > > finish this and shut the door on it for good.) > > I got my six patches by analyzing the lwn.net report of the fixes that > was cited which had 21 of which 50% didn't actually change the emitted > code, and 25% didn't have a user visible effect. > > But the broader point I'm making is just because the compiler people > come up with a shiny new warning doesn't necessarily mean the problem That's not what this is though; you're attacking a strawman. I'd encourage you to bring that up when that actually occurs, unlike this case since it's actively hindering getting -Wimplicit-fallthrough enabled for Clang. This is not a shiny new warning; it's already on for GCC and has existed in both compilers for multiple releases. And I'll also note that warnings are warnings and not errors because they cannot be proven to be bugs in 100% of cases, but they have led to bugs in the past. They require a human to review their intent and remove ambiguities. If 97% of cases would end in a break ("Expert C Programming: Deep C Secrets" - Peter van der Linden), then it starts to look to me like a language defect; certainly an incorrectly chosen default. But the compiler can't know those 3% were intentional, unless you're explicit for those exceptional cases. > it's detecting is one that causes us actual problems in the code base. > I'd really be happier if we had a theory about what classes of CVE or > bug we could eliminate before we embrace the next new warning. We don't generally file CVEs and waiting for them to occur might be too reactive, but I agree that pointing to some additional documentation in commit messages about how a warning could lead to a bug would make it clearer to reviewers why being able to enable it treewide, even if there's no bug in their particular subsystem, is in the general interest of the commons. On Mon, Nov 23, 2020 at 7:58 AM James Bottomley wrote: > > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 > > The whole crux of your argument seems to be maintainers' time isn't > important so we should accept all trivial patches ... I'm pushing back > on that assumption in two places, firstly the valulessness of the time > and secondly that all trivial patches are valuable. It's critical to the longevity of any open source project that there are not single points of failure. If someone is not expendable or replaceable (or claims to be) then that's a risk to the project and a bottleneck. Not having a replacement in training or some form of redundancy is short sighted. If trivial patches are adding too much to your workload, consider training a co-maintainer or asking for help from one of your reviewers whom you trust. I don't doubt it's hard to find maintainers, but existing maintainers should go out of their way to entrust co-maintainers especially when they find their workload becomes too high. And reviewing/picking up trivial patches is probably a great way to get started. If we allow too much knowledge of any one subsystem to collect with one maintainer, what happens when that maintainer leaves the community (which, given a finite lifespan, is an inevitability)? -- Thanks, ~Nick Desaulniers From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A1106C5519F for ; Wed, 25 Nov 2020 12:25:46 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 13340206E5 for ; Wed, 25 Nov 2020 12:25:45 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="VpDeC1mc"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=google.com header.i=@google.com header.b="GyrkIL7r" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 13340206E5 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-mtd-bounces+linux-mtd=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=hpcdaQlDVSZTrsCqDygPQ/AlJSqHmhey8SYVaMCVS/s=; b=VpDeC1mcB3FA/gTlbdbLkd4Jb pISxRSe5sPwTat2pHUr6X4vY8lOMRS7XldS8TmTUYtX+WEAS51ejNqHBq6tT4R3FgUWmfUFbjN97Q 6oBbFs3j0QdaJe5Ch5wIkx9A86tyyQafXUFPHnqTvqNTF5zK9A5iUJS0t3YyZiW1n3xFDbZsPtC2P tQZ60NhJ7nwYP+KgDlIJgo6ZDLr+spxKNLbrEa52MoBqlZ040dgC/dlpkZj4LNtdFIg27ytNZ1dbZ VLdqEyfPJ4DCs3YQzD7RMYXJoxuYROUa5362XDYHVqFSQWOlEbLDR255JGloUoFaWyneil2+4nKhs Vt2oi4xYw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1khtqt-0000AY-67; Wed, 25 Nov 2020 12:24:51 +0000 Received: from mail-pf1-x443.google.com ([2607:f8b0:4864:20::443]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1khtqj-0008WM-5h for linux-mtd@lists.infradead.org; Wed, 25 Nov 2020 12:24:46 +0000 Received: by mail-pf1-x443.google.com with SMTP id q10so2195326pfn.0 for ; Wed, 25 Nov 2020 04:24:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=GyrkIL7rJc/Wrkz9wtYqXZYvGBry6qXFkQono0nmrBFDlUCiGmbX9ByD1wUhih87ZW XCd/8etF0h65aGuVNHVvGVnSoIRV2cIFxWeuMsMEKDZ+SIKsK6eM3KIHPaY2Au+pxfCB jFmSmO0a8jtSnIjbAi/709gkMW9hnqxggrhUNIGI/2GrlejcLn7tyz9MFlSEpE31y19Z 9ARaZhNBbaKZzII6ioDoEFmbbi01XI+4/fF65wWR3SGfZuCMoV2cgGUJ8Osa8sFeqdbz KLllcyBsC3gtRPDbq4Yc+z3inKaZT7D05cYTb7CBHNDdR/afLH0A7E5JHrBWilMAefn4 uS6g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=af0yM+61vMhRzoMCcCbvYjb9x0K6zVhHzCXi6FLiGIirQp9/vOl4nOLAHMvo09D9Ca XvyKzAEXgYxVTaimWHVqJlC8V2Qs49EVBRd3e1cQz1+Sc7dcOB/gBIU2NjQGn7GKvx3m reOJtqaHVlH5v3pl3d03nH8gmXPGWE3TR5QIzRSks9Cm+xaflroTmho1yrRb5pR8cyaP c0wp3XnTmUgS9/Ha1vMQuor3x2ZPbiZ/UqcvIQXHDrx66T5+v1wwkWSxQeJnkm9Onrhx VLjbpXPwqKnFFBiliVIW44dUCEnxc1Y53wv9DAmW6Fx6FfovM8ZHo4JnNnvvvN8w88RU wKOQ== X-Gm-Message-State: AOAM532iBJ1C5Pp3T9oBzM25VOLuHaH71M9HDpb7sKB4QaXLabD9Trj7 0MZQkA+BgiEWySMn04ftErs2Tr6UTLbTHuZ5xrIPng== X-Google-Smtp-Source: ABdhPJySo35UzNwHodlreVMfJuWPwHO1z+zkcbFfSYU3Avf+sN4n16LJPBb97SBockWyJEKx3Xs8q1wCvzejZmwrmAM= X-Received: by 2002:a62:7905:0:b029:197:f300:5a2a with SMTP id u5-20020a6279050000b0290197f3005a2amr2898775pfc.30.1606307078380; Wed, 25 Nov 2020 04:24:38 -0800 (PST) MIME-Version: 1.0 References: <202011201129.B13FDB3C@keescook> <20201120115142.292999b2@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> <202011220816.8B6591A@keescook> <9b57fd4914b46f38d54087d75e072d6e947cb56d.camel@HansenPartnership.com> <0147972a72bc13f3629de8a32dee6f1f308994b5.camel@HansenPartnership.com> <20201123130348.GA3119@embeddedor> <8f5611bb015e044fa1c0a48147293923c2d904e4.camel@HansenPartnership.com> <202011241327.BB28F12F6@keescook> In-Reply-To: From: Nick Desaulniers Date: Wed, 25 Nov 2020 04:24:27 -0800 Message-ID: Subject: Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang To: James Bottomley X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201125_072441_265096_977F632D X-CRM114-Status: GOOD ( 36.70 ) X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: alsa-devel@alsa-project.org, bridge@lists.linux-foundation.org, target-devel@vger.kernel.org, Greg KH , linux-iio@vger.kernel.org, samba-technical@lists.samba.org, Jonathan Cameron , linux-fbdev@vger.kernel.org, dri-devel , "Gustavo A. R. Silva" , linux-ide@vger.kernel.org, dm-devel@redhat.com, keyrings@vger.kernel.org, linux-mtd@lists.infradead.org, GR-everest-linux-l2@marvell.com, wcn36xx@lists.infradead.org, linux-i3c@lists.infradead.org, linux1394-devel@lists.sourceforge.net, linux-afs@lists.infradead.org, drbd-dev@lists.linbit.com, devel@driverdev.osuosl.org, linux-cifs@vger.kernel.org, rds-devel@oss.oracle.com, linux-scsi@vger.kernel.org, linux-acpi@vger.kernel.org, linux-rdma@vger.kernel.org, oss-drivers@netronome.com, linux-atm-general@lists.sourceforge.net, ceph-devel@vger.kernel.org, amd-gfx list , linux-stm32@st-md-mailman.stormreply.com, cluster-devel@redhat.com, usb-storage@lists.one-eyed-alien.net, linux-mmc@vger.kernel.org, coreteam@netfilter.org, intel-wired-lan@lists.osuosl.org, linux-input@vger.kernel.org, Miguel Ojeda , Jakub Kicinski , linux-ext4@vger.kernel.org, virtualization@lists.linux-foundation.org, netfilter-devel@vger.kernel.org, linux-media@vger.kernel.org, Kees Cook , selinux@vger.kernel.org, linux-arm-msm , intel-gfx@lists.freedesktop.org, linux-sctp@vger.kernel.org, reiserfs-devel@vger.kernel.org, linux-geode@lists.infradead.org, linux-block@vger.kernel.org, linux-gpio@vger.kernel.org, op-tee@lists.trustedfirmware.org, linux-mediatek@lists.infradead.org, xen-devel@lists.xenproject.org, nouveau@lists.freedesktop.org, linux-hams@vger.kernel.org, Nathan Chancellor , linux-can@vger.kernel.org, Linux ARM , linux-hwmon@vger.kernel.org, "maintainer:X86 ARCHITECTURE \(32-BIT AND 64-BIT\)" , linux-watchdog@vger.kernel.org, GR-Linux-NIC-Dev@marvell.com, Linux Memory Management List , Network Development , linux-decnet-user@lists.sourceforge.net, linux-wireless , LKML , Linux-Renesas , linux-security-module@vger.kernel.org, linux-usb@vger.kernel.org, tipc-discussion@lists.sourceforge.net, "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , patches@opensource.cirrus.com, Joe Perches , linux-integrity@vger.kernel.org, linux-nfs@vger.kernel.org, linux-hardening@vger.kernel.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-mtd" Errors-To: linux-mtd-bounces+linux-mtd=archiver.kernel.org@lists.infradead.org On Tue, Nov 24, 2020 at 11:05 PM James Bottomley wrote: > > On Tue, 2020-11-24 at 13:32 -0800, Kees Cook wrote: > > We already enable -Wimplicit-fallthrough globally, so that's not the > > discussion. The issue is that Clang is (correctly) even more strict > > than GCC for this, so these are the remaining ones to fix for full > > Clang coverage too. > > > > People have spent more time debating this already than it would have > > taken to apply the patches. :) > > You mean we've already spent 90% of the effort to come this far so we > might as well go the remaining 10% because then at least we get some > return? It's certainly a clinching argument in defence procurement ... So developers and distributions using Clang can't have -Wimplicit-fallthrough enabled because GCC is less strict (which has been shown in this thread to lead to bugs)? We'd like to have nice things too, you know. I even agree that most of the churn comes from case 0: ++x; default: break; which I have a patch for: https://reviews.llvm.org/D91895. I agree that can never lead to bugs. But that's not the sole case of this series, just most of them. Though, note how the reviewer (C++ spec editor and clang front end owner) in https://reviews.llvm.org/D91895 even asks in that review how maybe a new flag would be more appropriate for a watered down/stylistic variant of the existing behavior. And if the current wording of Documentation/process/deprecated.rst around "fallthrough" is a straightforward rule of thumb, I kind of agree with him. > > > This is about robustness and language wrangling. It's a big code- > > base, and this is the price of our managing technical debt for > > permanent robustness improvements. (The numbers I ran from Gustavo's > > earlier patches were that about 10% of the places adjusted were > > identified as legitimate bugs being fixed. This final series may be > > lower, but there are still bugs being found from it -- we need to > > finish this and shut the door on it for good.) > > I got my six patches by analyzing the lwn.net report of the fixes that > was cited which had 21 of which 50% didn't actually change the emitted > code, and 25% didn't have a user visible effect. > > But the broader point I'm making is just because the compiler people > come up with a shiny new warning doesn't necessarily mean the problem That's not what this is though; you're attacking a strawman. I'd encourage you to bring that up when that actually occurs, unlike this case since it's actively hindering getting -Wimplicit-fallthrough enabled for Clang. This is not a shiny new warning; it's already on for GCC and has existed in both compilers for multiple releases. And I'll also note that warnings are warnings and not errors because they cannot be proven to be bugs in 100% of cases, but they have led to bugs in the past. They require a human to review their intent and remove ambiguities. If 97% of cases would end in a break ("Expert C Programming: Deep C Secrets" - Peter van der Linden), then it starts to look to me like a language defect; certainly an incorrectly chosen default. But the compiler can't know those 3% were intentional, unless you're explicit for those exceptional cases. > it's detecting is one that causes us actual problems in the code base. > I'd really be happier if we had a theory about what classes of CVE or > bug we could eliminate before we embrace the next new warning. We don't generally file CVEs and waiting for them to occur might be too reactive, but I agree that pointing to some additional documentation in commit messages about how a warning could lead to a bug would make it clearer to reviewers why being able to enable it treewide, even if there's no bug in their particular subsystem, is in the general interest of the commons. On Mon, Nov 23, 2020 at 7:58 AM James Bottomley wrote: > > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 > > The whole crux of your argument seems to be maintainers' time isn't > important so we should accept all trivial patches ... I'm pushing back > on that assumption in two places, firstly the valulessness of the time > and secondly that all trivial patches are valuable. It's critical to the longevity of any open source project that there are not single points of failure. If someone is not expendable or replaceable (or claims to be) then that's a risk to the project and a bottleneck. Not having a replacement in training or some form of redundancy is short sighted. If trivial patches are adding too much to your workload, consider training a co-maintainer or asking for help from one of your reviewers whom you trust. I don't doubt it's hard to find maintainers, but existing maintainers should go out of their way to entrust co-maintainers especially when they find their workload becomes too high. And reviewing/picking up trivial patches is probably a great way to get started. If we allow too much knowledge of any one subsystem to collect with one maintainer, what happens when that maintainer leaves the community (which, given a finite lifespan, is an inevitability)? -- Thanks, ~Nick Desaulniers ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2C5AFC83010 for ; Wed, 25 Nov 2020 12:25:03 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9A52920789 for ; Wed, 25 Nov 2020 12:25:02 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="ytfKgLzM"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=google.com header.i=@google.com header.b="GyrkIL7r" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9A52920789 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=c0+TTYKIQNiUqbUllBP/unjgQvMHCjxVxJzo35YNQBE=; b=ytfKgLzMBpyYJJ8NDHqeo/UdA 9GgPMdS/eMclKeIjh6toG2lohG31Wvswoe0MHhIgrUAc7Yoizjh0qNVjW0c+sHEz7bHkiFvgJdP4c RwedV3c5+csXjyHfjF7ngOZM2YfQRLun7aLH4sw8+Y1KY3bmQdmWJRDHD+oh/CtuaUhtjnYRKfZrK j1HQVMA98M/YD8UQj33wXnAjEgWCmjGSlSlRU+UCErYX0TIkm/hbwzQooHpMYdzX7Adus2Fd5u/ga mJdxquJWGqY97cLZkNnFfJd58srLc1le0ikCxCpcfIEjwdrq/q7aNkJcA0+2l2yTBCo/WDhSm9gdQ iQFAarang==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1khtqw-0000Bp-Jk; Wed, 25 Nov 2020 12:24:54 +0000 Received: from mail-pg1-x542.google.com ([2607:f8b0:4864:20::542]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1khtqm-0008WJ-Ce for linux-mediatek@lists.infradead.org; Wed, 25 Nov 2020 12:24:50 +0000 Received: by mail-pg1-x542.google.com with SMTP id j19so2320326pgg.5 for ; Wed, 25 Nov 2020 04:24:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=GyrkIL7rJc/Wrkz9wtYqXZYvGBry6qXFkQono0nmrBFDlUCiGmbX9ByD1wUhih87ZW XCd/8etF0h65aGuVNHVvGVnSoIRV2cIFxWeuMsMEKDZ+SIKsK6eM3KIHPaY2Au+pxfCB jFmSmO0a8jtSnIjbAi/709gkMW9hnqxggrhUNIGI/2GrlejcLn7tyz9MFlSEpE31y19Z 9ARaZhNBbaKZzII6ioDoEFmbbi01XI+4/fF65wWR3SGfZuCMoV2cgGUJ8Osa8sFeqdbz KLllcyBsC3gtRPDbq4Yc+z3inKaZT7D05cYTb7CBHNDdR/afLH0A7E5JHrBWilMAefn4 uS6g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=rpWXbiFV3VSjxgevhVsxe0RNDEOajq2BGhiHS59+m1hmTF5pk6WGhG09X9tzE7DvLq XPay66UcD+U7xTxX2mfQVF99LhUu4yxsrp1KAD7+d5BzxS8lLjkkn7Z+s4x0pRJzhGBG KQzYG8tFbyNdKb0RejsdmcqQTY8nR5X0Ussby3s5dZ/MhppeaOslsCZCTDpUAu9l7Kj9 MO9vp3RIAdiNYPdcTI/B5szou6gbCDINKIhlFUZQxnSrebfOYKf8A9UXIEqk5YcL6m6C 0GhQPwZc8Ua+EH8jDVVCHP/bpfbtCwbHl8Tx9158rqbbsheve0BO13UzNOreHxSUYHbU FZpg== X-Gm-Message-State: AOAM531ccMRPcFjGFQAgriJucTJvtJ48L5iTDhhXkV9b4g4N90R1Yjs+ ubK6cEx1rEK3ozSINOWhAnJYdXp1UG5zy4bwUeavIg== X-Google-Smtp-Source: ABdhPJySo35UzNwHodlreVMfJuWPwHO1z+zkcbFfSYU3Avf+sN4n16LJPBb97SBockWyJEKx3Xs8q1wCvzejZmwrmAM= X-Received: by 2002:a62:7905:0:b029:197:f300:5a2a with SMTP id u5-20020a6279050000b0290197f3005a2amr2898775pfc.30.1606307078380; Wed, 25 Nov 2020 04:24:38 -0800 (PST) MIME-Version: 1.0 References: <202011201129.B13FDB3C@keescook> <20201120115142.292999b2@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> <202011220816.8B6591A@keescook> <9b57fd4914b46f38d54087d75e072d6e947cb56d.camel@HansenPartnership.com> <0147972a72bc13f3629de8a32dee6f1f308994b5.camel@HansenPartnership.com> <20201123130348.GA3119@embeddedor> <8f5611bb015e044fa1c0a48147293923c2d904e4.camel@HansenPartnership.com> <202011241327.BB28F12F6@keescook> In-Reply-To: From: Nick Desaulniers Date: Wed, 25 Nov 2020 04:24:27 -0800 Message-ID: Subject: Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang To: James Bottomley X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201125_072444_482310_95E6E726 X-CRM114-Status: GOOD ( 35.78 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: alsa-devel@alsa-project.org, bridge@lists.linux-foundation.org, target-devel@vger.kernel.org, Greg KH , linux-iio@vger.kernel.org, samba-technical@lists.samba.org, Jonathan Cameron , linux-fbdev@vger.kernel.org, dri-devel , "Gustavo A. R. Silva" , linux-ide@vger.kernel.org, dm-devel@redhat.com, keyrings@vger.kernel.org, linux-mtd@lists.infradead.org, GR-everest-linux-l2@marvell.com, wcn36xx@lists.infradead.org, linux-i3c@lists.infradead.org, linux1394-devel@lists.sourceforge.net, linux-afs@lists.infradead.org, drbd-dev@lists.linbit.com, devel@driverdev.osuosl.org, linux-cifs@vger.kernel.org, rds-devel@oss.oracle.com, linux-scsi@vger.kernel.org, linux-acpi@vger.kernel.org, linux-rdma@vger.kernel.org, oss-drivers@netronome.com, linux-atm-general@lists.sourceforge.net, ceph-devel@vger.kernel.org, amd-gfx list , linux-stm32@st-md-mailman.stormreply.com, cluster-devel@redhat.com, usb-storage@lists.one-eyed-alien.net, linux-mmc@vger.kernel.org, coreteam@netfilter.org, intel-wired-lan@lists.osuosl.org, linux-input@vger.kernel.org, Miguel Ojeda , Jakub Kicinski , linux-ext4@vger.kernel.org, virtualization@lists.linux-foundation.org, netfilter-devel@vger.kernel.org, linux-media@vger.kernel.org, Kees Cook , selinux@vger.kernel.org, linux-arm-msm , intel-gfx@lists.freedesktop.org, linux-sctp@vger.kernel.org, reiserfs-devel@vger.kernel.org, linux-geode@lists.infradead.org, linux-block@vger.kernel.org, linux-gpio@vger.kernel.org, op-tee@lists.trustedfirmware.org, linux-mediatek@lists.infradead.org, xen-devel@lists.xenproject.org, nouveau@lists.freedesktop.org, linux-hams@vger.kernel.org, Nathan Chancellor , linux-can@vger.kernel.org, Linux ARM , linux-hwmon@vger.kernel.org, "maintainer:X86 ARCHITECTURE \(32-BIT AND 64-BIT\)" , linux-watchdog@vger.kernel.org, GR-Linux-NIC-Dev@marvell.com, Linux Memory Management List , Network Development , linux-decnet-user@lists.sourceforge.net, linux-wireless , LKML , Linux-Renesas , linux-security-module@vger.kernel.org, linux-usb@vger.kernel.org, tipc-discussion@lists.sourceforge.net, "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , patches@opensource.cirrus.com, Joe Perches , linux-integrity@vger.kernel.org, linux-nfs@vger.kernel.org, linux-hardening@vger.kernel.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org On Tue, Nov 24, 2020 at 11:05 PM James Bottomley wrote: > > On Tue, 2020-11-24 at 13:32 -0800, Kees Cook wrote: > > We already enable -Wimplicit-fallthrough globally, so that's not the > > discussion. The issue is that Clang is (correctly) even more strict > > than GCC for this, so these are the remaining ones to fix for full > > Clang coverage too. > > > > People have spent more time debating this already than it would have > > taken to apply the patches. :) > > You mean we've already spent 90% of the effort to come this far so we > might as well go the remaining 10% because then at least we get some > return? It's certainly a clinching argument in defence procurement ... So developers and distributions using Clang can't have -Wimplicit-fallthrough enabled because GCC is less strict (which has been shown in this thread to lead to bugs)? We'd like to have nice things too, you know. I even agree that most of the churn comes from case 0: ++x; default: break; which I have a patch for: https://reviews.llvm.org/D91895. I agree that can never lead to bugs. But that's not the sole case of this series, just most of them. Though, note how the reviewer (C++ spec editor and clang front end owner) in https://reviews.llvm.org/D91895 even asks in that review how maybe a new flag would be more appropriate for a watered down/stylistic variant of the existing behavior. And if the current wording of Documentation/process/deprecated.rst around "fallthrough" is a straightforward rule of thumb, I kind of agree with him. > > > This is about robustness and language wrangling. It's a big code- > > base, and this is the price of our managing technical debt for > > permanent robustness improvements. (The numbers I ran from Gustavo's > > earlier patches were that about 10% of the places adjusted were > > identified as legitimate bugs being fixed. This final series may be > > lower, but there are still bugs being found from it -- we need to > > finish this and shut the door on it for good.) > > I got my six patches by analyzing the lwn.net report of the fixes that > was cited which had 21 of which 50% didn't actually change the emitted > code, and 25% didn't have a user visible effect. > > But the broader point I'm making is just because the compiler people > come up with a shiny new warning doesn't necessarily mean the problem That's not what this is though; you're attacking a strawman. I'd encourage you to bring that up when that actually occurs, unlike this case since it's actively hindering getting -Wimplicit-fallthrough enabled for Clang. This is not a shiny new warning; it's already on for GCC and has existed in both compilers for multiple releases. And I'll also note that warnings are warnings and not errors because they cannot be proven to be bugs in 100% of cases, but they have led to bugs in the past. They require a human to review their intent and remove ambiguities. If 97% of cases would end in a break ("Expert C Programming: Deep C Secrets" - Peter van der Linden), then it starts to look to me like a language defect; certainly an incorrectly chosen default. But the compiler can't know those 3% were intentional, unless you're explicit for those exceptional cases. > it's detecting is one that causes us actual problems in the code base. > I'd really be happier if we had a theory about what classes of CVE or > bug we could eliminate before we embrace the next new warning. We don't generally file CVEs and waiting for them to occur might be too reactive, but I agree that pointing to some additional documentation in commit messages about how a warning could lead to a bug would make it clearer to reviewers why being able to enable it treewide, even if there's no bug in their particular subsystem, is in the general interest of the commons. On Mon, Nov 23, 2020 at 7:58 AM James Bottomley wrote: > > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 > > The whole crux of your argument seems to be maintainers' time isn't > important so we should accept all trivial patches ... I'm pushing back > on that assumption in two places, firstly the valulessness of the time > and secondly that all trivial patches are valuable. It's critical to the longevity of any open source project that there are not single points of failure. If someone is not expendable or replaceable (or claims to be) then that's a risk to the project and a bottleneck. Not having a replacement in training or some form of redundancy is short sighted. If trivial patches are adding too much to your workload, consider training a co-maintainer or asking for help from one of your reviewers whom you trust. I don't doubt it's hard to find maintainers, but existing maintainers should go out of their way to entrust co-maintainers especially when they find their workload becomes too high. And reviewing/picking up trivial patches is probably a great way to get started. If we allow too much knowledge of any one subsystem to collect with one maintainer, what happens when that maintainer leaves the community (which, given a finite lifespan, is an inevitability)? -- Thanks, ~Nick Desaulniers _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.5 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 69CEAC6379D for ; Wed, 25 Nov 2020 12:24:46 +0000 (UTC) Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9D8DF206F7 for ; Wed, 25 Nov 2020 12:24:45 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=google.com header.i=@google.com header.b="GyrkIL7r" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9D8DF206F7 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=lists.linux-foundation.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=virtualization-bounces@lists.linux-foundation.org Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 104C92E140; Wed, 25 Nov 2020 12:24:45 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id zQCBYuhYUUbN; Wed, 25 Nov 2020 12:24:42 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by silver.osuosl.org (Postfix) with ESMTP id 3A5B62E11F; Wed, 25 Nov 2020 12:24:42 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 2DB22C0891; Wed, 25 Nov 2020 12:24:42 +0000 (UTC) Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by lists.linuxfoundation.org (Postfix) with ESMTP id 42282C0052 for ; Wed, 25 Nov 2020 12:24:41 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 3CEEC8757D for ; Wed, 25 Nov 2020 12:24:41 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id xF-tDkqKFNei for ; Wed, 25 Nov 2020 12:24:39 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from mail-pg1-f195.google.com (mail-pg1-f195.google.com [209.85.215.195]) by hemlock.osuosl.org (Postfix) with ESMTPS id 62BFB8757E for ; Wed, 25 Nov 2020 12:24:39 +0000 (UTC) Received: by mail-pg1-f195.google.com with SMTP id t3so2291589pgi.11 for ; Wed, 25 Nov 2020 04:24:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=GyrkIL7rJc/Wrkz9wtYqXZYvGBry6qXFkQono0nmrBFDlUCiGmbX9ByD1wUhih87ZW XCd/8etF0h65aGuVNHVvGVnSoIRV2cIFxWeuMsMEKDZ+SIKsK6eM3KIHPaY2Au+pxfCB jFmSmO0a8jtSnIjbAi/709gkMW9hnqxggrhUNIGI/2GrlejcLn7tyz9MFlSEpE31y19Z 9ARaZhNBbaKZzII6ioDoEFmbbi01XI+4/fF65wWR3SGfZuCMoV2cgGUJ8Osa8sFeqdbz KLllcyBsC3gtRPDbq4Yc+z3inKaZT7D05cYTb7CBHNDdR/afLH0A7E5JHrBWilMAefn4 uS6g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=hlOC5+3X9aAbM8hKEOSW+KLU3UD3GqrG05UhQ3figMV82W16hS/IuUqc76LghtlAoU Omftelnw49k5HmD9BDYQz9M4TgCgJe8/YXGDrm9UJjFZszi0GY1vEMUYtUzNme+TbmiO jEy14UDcCWIhJFWPi+qMNChiL89Vxi/iI9h2mwr5l7EkgYZ3juGnBUhJ9d9uJvd7kI1G nkcWYBSvv6WbhmB39rdwnE3dCO61yvmiho7/p0ZD666xiDGB+ohRErTVkiyPn9xKd+GW 4R1Gh88G2tbp840faQq0/hwDYailcU3KVgu0NQkFuww28LPxBfa2BpfiZvp3DreR+mbW yDwg== X-Gm-Message-State: AOAM5332FwuJsXJIFcJYAk4Xy0Ft/bqiMBIQ5XIGHBXGwTwg5YrtL/dt clIcSV3GUt1byHh/30em7d6ww5PsadYGzLwYXMiEkg== X-Google-Smtp-Source: ABdhPJySo35UzNwHodlreVMfJuWPwHO1z+zkcbFfSYU3Avf+sN4n16LJPBb97SBockWyJEKx3Xs8q1wCvzejZmwrmAM= X-Received: by 2002:a62:7905:0:b029:197:f300:5a2a with SMTP id u5-20020a6279050000b0290197f3005a2amr2898775pfc.30.1606307078380; Wed, 25 Nov 2020 04:24:38 -0800 (PST) MIME-Version: 1.0 References: <202011201129.B13FDB3C@keescook> <20201120115142.292999b2@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> <202011220816.8B6591A@keescook> <9b57fd4914b46f38d54087d75e072d6e947cb56d.camel@HansenPartnership.com> <0147972a72bc13f3629de8a32dee6f1f308994b5.camel@HansenPartnership.com> <20201123130348.GA3119@embeddedor> <8f5611bb015e044fa1c0a48147293923c2d904e4.camel@HansenPartnership.com> <202011241327.BB28F12F6@keescook> In-Reply-To: Date: Wed, 25 Nov 2020 04:24:27 -0800 Message-ID: Subject: Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang To: James Bottomley Cc: alsa-devel@alsa-project.org, bridge@lists.linux-foundation.org, target-devel@vger.kernel.org, Greg KH , linux-iio@vger.kernel.org, samba-technical@lists.samba.org, Jonathan Cameron , linux-fbdev@vger.kernel.org, dri-devel , "Gustavo A. R. Silva" , linux-ide@vger.kernel.org, dm-devel@redhat.com, keyrings@vger.kernel.org, linux-mtd@lists.infradead.org, GR-everest-linux-l2@marvell.com, wcn36xx@lists.infradead.org, linux-i3c@lists.infradead.org, linux1394-devel@lists.sourceforge.net, linux-afs@lists.infradead.org, drbd-dev@lists.linbit.com, devel@driverdev.osuosl.org, linux-cifs@vger.kernel.org, rds-devel@oss.oracle.com, linux-scsi@vger.kernel.org, linux-acpi@vger.kernel.org, linux-rdma@vger.kernel.org, oss-drivers@netronome.com, linux-atm-general@lists.sourceforge.net, ceph-devel@vger.kernel.org, amd-gfx list , linux-stm32@st-md-mailman.stormreply.com, cluster-devel@redhat.com, usb-storage@lists.one-eyed-alien.net, linux-mmc@vger.kernel.org, coreteam@netfilter.org, intel-wired-lan@lists.osuosl.org, linux-input@vger.kernel.org, Miguel Ojeda , Jakub Kicinski , linux-ext4@vger.kernel.org, virtualization@lists.linux-foundation.org, netfilter-devel@vger.kernel.org, linux-media@vger.kernel.org, Kees Cook , selinux@vger.kernel.org, linux-arm-msm , intel-gfx@lists.freedesktop.org, linux-sctp@vger.kernel.org, reiserfs-devel@vger.kernel.org, linux-geode@lists.infradead.org, linux-block@vger.kernel.org, linux-gpio@vger.kernel.org, op-tee@lists.trustedfirmware.org, linux-mediatek@lists.infradead.org, xen-devel@lists.xenproject.org, nouveau@lists.freedesktop.org, linux-hams@vger.kernel.org, Nathan Chancellor , linux-can@vger.kernel.org, Linux ARM , linux-hwmon@vger.kernel.org, "maintainer:X86 ARCHITECTURE \(32-BIT AND 64-BIT\)" , linux-watchdog@vger.kernel.org, GR-Linux-NIC-Dev@marvell.com, Linux Memory Management List , Network Development , linux-decnet-user@lists.sourceforge.net, linux-wireless , LKML , Linux-Renesas , linux-security-module@vger.kernel.org, linux-usb@vger.kernel.org, tipc-discussion@lists.sourceforge.net, "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , patches@opensource.cirrus.com, Joe Perches , linux-integrity@vger.kernel.org, linux-nfs@vger.kernel.org, linux-hardening@vger.kernel.org X-BeenThere: virtualization@lists.linux-foundation.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Linux virtualization List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Nick Desaulniers via Virtualization Reply-To: Nick Desaulniers Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: virtualization-bounces@lists.linux-foundation.org Sender: "Virtualization" On Tue, Nov 24, 2020 at 11:05 PM James Bottomley wrote: > > On Tue, 2020-11-24 at 13:32 -0800, Kees Cook wrote: > > We already enable -Wimplicit-fallthrough globally, so that's not the > > discussion. The issue is that Clang is (correctly) even more strict > > than GCC for this, so these are the remaining ones to fix for full > > Clang coverage too. > > > > People have spent more time debating this already than it would have > > taken to apply the patches. :) > > You mean we've already spent 90% of the effort to come this far so we > might as well go the remaining 10% because then at least we get some > return? It's certainly a clinching argument in defence procurement ... So developers and distributions using Clang can't have -Wimplicit-fallthrough enabled because GCC is less strict (which has been shown in this thread to lead to bugs)? We'd like to have nice things too, you know. I even agree that most of the churn comes from case 0: ++x; default: break; which I have a patch for: https://reviews.llvm.org/D91895. I agree that can never lead to bugs. But that's not the sole case of this series, just most of them. Though, note how the reviewer (C++ spec editor and clang front end owner) in https://reviews.llvm.org/D91895 even asks in that review how maybe a new flag would be more appropriate for a watered down/stylistic variant of the existing behavior. And if the current wording of Documentation/process/deprecated.rst around "fallthrough" is a straightforward rule of thumb, I kind of agree with him. > > > This is about robustness and language wrangling. It's a big code- > > base, and this is the price of our managing technical debt for > > permanent robustness improvements. (The numbers I ran from Gustavo's > > earlier patches were that about 10% of the places adjusted were > > identified as legitimate bugs being fixed. This final series may be > > lower, but there are still bugs being found from it -- we need to > > finish this and shut the door on it for good.) > > I got my six patches by analyzing the lwn.net report of the fixes that > was cited which had 21 of which 50% didn't actually change the emitted > code, and 25% didn't have a user visible effect. > > But the broader point I'm making is just because the compiler people > come up with a shiny new warning doesn't necessarily mean the problem That's not what this is though; you're attacking a strawman. I'd encourage you to bring that up when that actually occurs, unlike this case since it's actively hindering getting -Wimplicit-fallthrough enabled for Clang. This is not a shiny new warning; it's already on for GCC and has existed in both compilers for multiple releases. And I'll also note that warnings are warnings and not errors because they cannot be proven to be bugs in 100% of cases, but they have led to bugs in the past. They require a human to review their intent and remove ambiguities. If 97% of cases would end in a break ("Expert C Programming: Deep C Secrets" - Peter van der Linden), then it starts to look to me like a language defect; certainly an incorrectly chosen default. But the compiler can't know those 3% were intentional, unless you're explicit for those exceptional cases. > it's detecting is one that causes us actual problems in the code base. > I'd really be happier if we had a theory about what classes of CVE or > bug we could eliminate before we embrace the next new warning. We don't generally file CVEs and waiting for them to occur might be too reactive, but I agree that pointing to some additional documentation in commit messages about how a warning could lead to a bug would make it clearer to reviewers why being able to enable it treewide, even if there's no bug in their particular subsystem, is in the general interest of the commons. On Mon, Nov 23, 2020 at 7:58 AM James Bottomley wrote: > > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 > > The whole crux of your argument seems to be maintainers' time isn't > important so we should accept all trivial patches ... I'm pushing back > on that assumption in two places, firstly the valulessness of the time > and secondly that all trivial patches are valuable. It's critical to the longevity of any open source project that there are not single points of failure. If someone is not expendable or replaceable (or claims to be) then that's a risk to the project and a bottleneck. Not having a replacement in training or some form of redundancy is short sighted. If trivial patches are adding too much to your workload, consider training a co-maintainer or asking for help from one of your reviewers whom you trust. I don't doubt it's hard to find maintainers, but existing maintainers should go out of their way to entrust co-maintainers especially when they find their workload becomes too high. And reviewing/picking up trivial patches is probably a great way to get started. If we allow too much knowledge of any one subsystem to collect with one maintainer, what happens when that maintainer leaves the community (which, given a finite lifespan, is an inevitability)? -- Thanks, ~Nick Desaulniers _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.5 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EB7DEC63777 for ; Thu, 26 Nov 2020 08:24:17 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 81A3B20872 for ; Thu, 26 Nov 2020 08:24:17 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=google.com header.i=@google.com header.b="GyrkIL7r" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 81A3B20872 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id DE6D86E825; Thu, 26 Nov 2020 08:24:09 +0000 (UTC) Received: from mail-pg1-x542.google.com (mail-pg1-x542.google.com [IPv6:2607:f8b0:4864:20::542]) by gabe.freedesktop.org (Postfix) with ESMTPS id 355EF6E98F for ; Wed, 25 Nov 2020 12:24:39 +0000 (UTC) Received: by mail-pg1-x542.google.com with SMTP id t37so2311612pga.7 for ; Wed, 25 Nov 2020 04:24:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=GyrkIL7rJc/Wrkz9wtYqXZYvGBry6qXFkQono0nmrBFDlUCiGmbX9ByD1wUhih87ZW XCd/8etF0h65aGuVNHVvGVnSoIRV2cIFxWeuMsMEKDZ+SIKsK6eM3KIHPaY2Au+pxfCB jFmSmO0a8jtSnIjbAi/709gkMW9hnqxggrhUNIGI/2GrlejcLn7tyz9MFlSEpE31y19Z 9ARaZhNBbaKZzII6ioDoEFmbbi01XI+4/fF65wWR3SGfZuCMoV2cgGUJ8Osa8sFeqdbz KLllcyBsC3gtRPDbq4Yc+z3inKaZT7D05cYTb7CBHNDdR/afLH0A7E5JHrBWilMAefn4 uS6g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=hT8knKfFCc8RlPxB9FKPo5ZC/Cu9MFRonvR4TuWKkdTQ5rAKKaVaxb1T2ez9KldGAD wWKoUTcQT2bkLo6uhIN3MS4aLam6tqosp4tW2RBGUrzThMH3UU3CZR3dDC9Z3wlUeEHc 8IYzqOZgfp2yC1TZEIooqQm2KBr/Kwjfo3QD2xf2sIEuP51IvJXTxcZ98fvE4yrdGyS3 ne7yr7FMo21H1C4pi8S3CAyXWypNznpACIXStlxatrgoa2nkYdd3c+2XJLz6eAeUognx ZZsjW2rOuHubEz7qgOxYjCnY7mz0KScIJwQkZWukxOjjd9M11yCrPdq7CFkub81eirz7 gl1A== X-Gm-Message-State: AOAM533PuLIfB8uVjACxdFPLBf2QHixkUhn/PNbWSHQAO6lI//QHQz6K +zix11cmOWgzKumxhgbFgjxq5j5bWbxFZcIN5+TxBA== X-Google-Smtp-Source: ABdhPJySo35UzNwHodlreVMfJuWPwHO1z+zkcbFfSYU3Avf+sN4n16LJPBb97SBockWyJEKx3Xs8q1wCvzejZmwrmAM= X-Received: by 2002:a62:7905:0:b029:197:f300:5a2a with SMTP id u5-20020a6279050000b0290197f3005a2amr2898775pfc.30.1606307078380; Wed, 25 Nov 2020 04:24:38 -0800 (PST) MIME-Version: 1.0 References: <202011201129.B13FDB3C@keescook> <20201120115142.292999b2@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> <202011220816.8B6591A@keescook> <9b57fd4914b46f38d54087d75e072d6e947cb56d.camel@HansenPartnership.com> <0147972a72bc13f3629de8a32dee6f1f308994b5.camel@HansenPartnership.com> <20201123130348.GA3119@embeddedor> <8f5611bb015e044fa1c0a48147293923c2d904e4.camel@HansenPartnership.com> <202011241327.BB28F12F6@keescook> In-Reply-To: From: Nick Desaulniers Date: Wed, 25 Nov 2020 04:24:27 -0800 Message-ID: Subject: Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang To: James Bottomley X-Mailman-Approved-At: Thu, 26 Nov 2020 08:24:08 +0000 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: alsa-devel@alsa-project.org, bridge@lists.linux-foundation.org, target-devel@vger.kernel.org, Greg KH , linux-iio@vger.kernel.org, samba-technical@lists.samba.org, linux-mmc@vger.kernel.org, linux-fbdev@vger.kernel.org, dri-devel , "Gustavo A. R. Silva" , linux-ide@vger.kernel.org, dm-devel@redhat.com, keyrings@vger.kernel.org, linux-mtd@lists.infradead.org, GR-everest-linux-l2@marvell.com, wcn36xx@lists.infradead.org, linux-i3c@lists.infradead.org, linux1394-devel@lists.sourceforge.net, linux-afs@lists.infradead.org, drbd-dev@lists.linbit.com, devel@driverdev.osuosl.org, linux-cifs@vger.kernel.org, rds-devel@oss.oracle.com, linux-scsi@vger.kernel.org, linux-acpi@vger.kernel.org, linux-rdma@vger.kernel.org, oss-drivers@netronome.com, linux-atm-general@lists.sourceforge.net, ceph-devel@vger.kernel.org, amd-gfx list , linux-stm32@st-md-mailman.stormreply.com, cluster-devel@redhat.com, usb-storage@lists.one-eyed-alien.net, coreteam@netfilter.org, intel-wired-lan@lists.osuosl.org, linux-input@vger.kernel.org, Miguel Ojeda , Jakub Kicinski , linux-ext4@vger.kernel.org, virtualization@lists.linux-foundation.org, netfilter-devel@vger.kernel.org, linux-media@vger.kernel.org, Kees Cook , selinux@vger.kernel.org, linux-arm-msm , intel-gfx@lists.freedesktop.org, linux-sctp@vger.kernel.org, reiserfs-devel@vger.kernel.org, linux-geode@lists.infradead.org, linux-block@vger.kernel.org, linux-gpio@vger.kernel.org, op-tee@lists.trustedfirmware.org, linux-mediatek@lists.infradead.org, xen-devel@lists.xenproject.org, nouveau@lists.freedesktop.org, linux-hams@vger.kernel.org, Nathan Chancellor , linux-can@vger.kernel.org, Linux ARM , linux-hwmon@vger.kernel.org, "maintainer:X86 ARCHITECTURE \(32-BIT AND 64-BIT\)" , linux-watchdog@vger.kernel.org, GR-Linux-NIC-Dev@marvell.com, Linux Memory Management List , Network Development , linux-decnet-user@lists.sourceforge.net, linux-wireless , LKML , Linux-Renesas , linux-security-module@vger.kernel.org, linux-usb@vger.kernel.org, tipc-discussion@lists.sourceforge.net, "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , Jonathan Cameron , patches@opensource.cirrus.com, Joe Perches , linux-integrity@vger.kernel.org, linux-nfs@vger.kernel.org, linux-hardening@vger.kernel.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Tue, Nov 24, 2020 at 11:05 PM James Bottomley wrote: > > On Tue, 2020-11-24 at 13:32 -0800, Kees Cook wrote: > > We already enable -Wimplicit-fallthrough globally, so that's not the > > discussion. The issue is that Clang is (correctly) even more strict > > than GCC for this, so these are the remaining ones to fix for full > > Clang coverage too. > > > > People have spent more time debating this already than it would have > > taken to apply the patches. :) > > You mean we've already spent 90% of the effort to come this far so we > might as well go the remaining 10% because then at least we get some > return? It's certainly a clinching argument in defence procurement ... So developers and distributions using Clang can't have -Wimplicit-fallthrough enabled because GCC is less strict (which has been shown in this thread to lead to bugs)? We'd like to have nice things too, you know. I even agree that most of the churn comes from case 0: ++x; default: break; which I have a patch for: https://reviews.llvm.org/D91895. I agree that can never lead to bugs. But that's not the sole case of this series, just most of them. Though, note how the reviewer (C++ spec editor and clang front end owner) in https://reviews.llvm.org/D91895 even asks in that review how maybe a new flag would be more appropriate for a watered down/stylistic variant of the existing behavior. And if the current wording of Documentation/process/deprecated.rst around "fallthrough" is a straightforward rule of thumb, I kind of agree with him. > > > This is about robustness and language wrangling. It's a big code- > > base, and this is the price of our managing technical debt for > > permanent robustness improvements. (The numbers I ran from Gustavo's > > earlier patches were that about 10% of the places adjusted were > > identified as legitimate bugs being fixed. This final series may be > > lower, but there are still bugs being found from it -- we need to > > finish this and shut the door on it for good.) > > I got my six patches by analyzing the lwn.net report of the fixes that > was cited which had 21 of which 50% didn't actually change the emitted > code, and 25% didn't have a user visible effect. > > But the broader point I'm making is just because the compiler people > come up with a shiny new warning doesn't necessarily mean the problem That's not what this is though; you're attacking a strawman. I'd encourage you to bring that up when that actually occurs, unlike this case since it's actively hindering getting -Wimplicit-fallthrough enabled for Clang. This is not a shiny new warning; it's already on for GCC and has existed in both compilers for multiple releases. And I'll also note that warnings are warnings and not errors because they cannot be proven to be bugs in 100% of cases, but they have led to bugs in the past. They require a human to review their intent and remove ambiguities. If 97% of cases would end in a break ("Expert C Programming: Deep C Secrets" - Peter van der Linden), then it starts to look to me like a language defect; certainly an incorrectly chosen default. But the compiler can't know those 3% were intentional, unless you're explicit for those exceptional cases. > it's detecting is one that causes us actual problems in the code base. > I'd really be happier if we had a theory about what classes of CVE or > bug we could eliminate before we embrace the next new warning. We don't generally file CVEs and waiting for them to occur might be too reactive, but I agree that pointing to some additional documentation in commit messages about how a warning could lead to a bug would make it clearer to reviewers why being able to enable it treewide, even if there's no bug in their particular subsystem, is in the general interest of the commons. On Mon, Nov 23, 2020 at 7:58 AM James Bottomley wrote: > > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 > > The whole crux of your argument seems to be maintainers' time isn't > important so we should accept all trivial patches ... I'm pushing back > on that assumption in two places, firstly the valulessness of the time > and secondly that all trivial patches are valuable. It's critical to the longevity of any open source project that there are not single points of failure. If someone is not expendable or replaceable (or claims to be) then that's a risk to the project and a bottleneck. Not having a replacement in training or some form of redundancy is short sighted. If trivial patches are adding too much to your workload, consider training a co-maintainer or asking for help from one of your reviewers whom you trust. I don't doubt it's hard to find maintainers, but existing maintainers should go out of their way to entrust co-maintainers especially when they find their workload becomes too high. And reviewing/picking up trivial patches is probably a great way to get started. If we allow too much knowledge of any one subsystem to collect with one maintainer, what happens when that maintainer leaves the community (which, given a finite lifespan, is an inevitability)? -- Thanks, ~Nick Desaulniers _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.7 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 853F4C6379D for ; Thu, 26 Nov 2020 08:36:45 +0000 (UTC) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id CF7E520B80 for ; Thu, 26 Nov 2020 08:36:44 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CF7E520B80 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=tempfail smtp.mailfrom=dm-devel-bounces@redhat.com Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-100-GWstatg4MDCDb2VRneAB-Q-1; Thu, 26 Nov 2020 03:36:33 -0500 X-MC-Unique: GWstatg4MDCDb2VRneAB-Q-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 0F0DD107AFA8; Thu, 26 Nov 2020 08:36:29 +0000 (UTC) Received: from colo-mx.corp.redhat.com (colo-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by smtp.corp.redhat.com (Postfix) with ESMTPS id DE2C619C78; Thu, 26 Nov 2020 08:36:28 +0000 (UTC) Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by colo-mx.corp.redhat.com (Postfix) with ESMTP id B31BF50030; Thu, 26 Nov 2020 08:36:28 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id 0APCOiMu030916 for ; Wed, 25 Nov 2020 07:24:45 -0500 Received: by smtp.corp.redhat.com (Postfix) id CC6D22166B29; Wed, 25 Nov 2020 12:24:44 +0000 (UTC) Received: from mimecast-mx02.redhat.com (mimecast03.extmail.prod.ext.rdu2.redhat.com [10.11.55.19]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C76E62166B27 for ; Wed, 25 Nov 2020 12:24:42 +0000 (UTC) Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 6AD1C811E78 for ; Wed, 25 Nov 2020 12:24:42 +0000 (UTC) Received: from mail-pf1-f194.google.com (mail-pf1-f194.google.com [209.85.210.194]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-100-xoFCqS6gPs2BOWzGCJlDKA-1; Wed, 25 Nov 2020 07:24:39 -0500 X-MC-Unique: xoFCqS6gPs2BOWzGCJlDKA-1 Received: by mail-pf1-f194.google.com with SMTP id x24so2169406pfn.6 for ; Wed, 25 Nov 2020 04:24:39 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=t5YfqFXwJ42mwCn5cos2Gzjgz++Ppf/dY4qD1CZMwEcWwWrH1upX7g2WHLKMzHkd6P f2nAyWegK0trujHO8nptAdZP7uWNiMY0lCD89apCbqj4kkP+Wusz8HhlVM11FHEDSmMq WIRW9B9QHM0TXYciwI6vK/xoWE8t8kofidBPCWScDqU/noBithjderzLgQbHixEzp5Lj yRE+5gY8tLIIjkzJfJTGyoXSrHCiFEdBOCbVpHoHqIMpAYr02fT5gT12dw9I6Pa5MBtN sZjVMoPxIAFoXoG1UOXx+gbkc8/SLUe1qpNX9fF6BvseqJAeM/MtJS0QT6w7YE/GczEv uj3g== X-Gm-Message-State: AOAM533nljEHM+C7eGG7ZTsGksGZD/CTjUfSwzjzkqXRSiwIMxEshst9 ZA3uWFW0GxA1v8M3B0AaF1WYw3dwsFH6Y8U76Rd0zA== X-Google-Smtp-Source: ABdhPJySo35UzNwHodlreVMfJuWPwHO1z+zkcbFfSYU3Avf+sN4n16LJPBb97SBockWyJEKx3Xs8q1wCvzejZmwrmAM= X-Received: by 2002:a62:7905:0:b029:197:f300:5a2a with SMTP id u5-20020a6279050000b0290197f3005a2amr2898775pfc.30.1606307078380; Wed, 25 Nov 2020 04:24:38 -0800 (PST) MIME-Version: 1.0 References: <202011201129.B13FDB3C@keescook> <20201120115142.292999b2@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> <202011220816.8B6591A@keescook> <9b57fd4914b46f38d54087d75e072d6e947cb56d.camel@HansenPartnership.com> <0147972a72bc13f3629de8a32dee6f1f308994b5.camel@HansenPartnership.com> <20201123130348.GA3119@embeddedor> <8f5611bb015e044fa1c0a48147293923c2d904e4.camel@HansenPartnership.com> <202011241327.BB28F12F6@keescook> In-Reply-To: From: Nick Desaulniers Date: Wed, 25 Nov 2020 04:24:27 -0800 Message-ID: To: James Bottomley X-Mimecast-Impersonation-Protect: Policy=CLT - Impersonation Protection Definition; Similar Internal Domain=false; Similar Monitored External Domain=false; Custom External Domain=false; Mimecast External Domain=false; Newly Observed Domain=false; Internal User Name=false; Custom Display Name List=false; Reply-to Address Mismatch=false; Targeted Threat Dictionary=false; Mimecast Threat Dictionary=false; Custom Threat Dictionary=false X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-loop: dm-devel@redhat.com X-Mailman-Approved-At: Thu, 26 Nov 2020 03:35:50 -0500 Cc: alsa-devel@alsa-project.org, bridge@lists.linux-foundation.org, target-devel@vger.kernel.org, Greg KH , linux-iio@vger.kernel.org, samba-technical@lists.samba.org, linux-mmc@vger.kernel.org, linux-fbdev@vger.kernel.org, dri-devel , "Gustavo A. R. Silva" , Linux Memory Management List , linux-ide@vger.kernel.org, dm-devel@redhat.com, keyrings@vger.kernel.org, linux-mtd@lists.infradead.org, GR-everest-linux-l2@marvell.com, wcn36xx@lists.infradead.org, linux-i3c@lists.infradead.org, linux1394-devel@lists.sourceforge.net, linux-afs@lists.infradead.org, linux-watchdog@vger.kernel.org, devel@driverdev.osuosl.org, linux-cifs@vger.kernel.org, rds-devel@oss.oracle.com, linux-scsi@vger.kernel.org, linux-acpi@vger.kernel.org, linux-rdma@vger.kernel.org, oss-drivers@netronome.com, linux-atm-general@lists.sourceforge.net, ceph-devel@vger.kernel.org, amd-gfx list , linux-stm32@st-md-mailman.stormreply.com, cluster-devel@redhat.com, usb-storage@lists.one-eyed-alien.net, coreteam@netfilter.org, intel-wired-lan@lists.osuosl.org, linux-input@vger.kernel.org, Miguel Ojeda , Jakub Kicinski , linux-ext4@vger.kernel.org, virtualization@lists.linux-foundation.org, netfilter-devel@vger.kernel.org, linux-media@vger.kernel.org, Kees Cook , selinux@vger.kernel.org, linux-arm-msm , intel-gfx@lists.freedesktop.org, linux-sctp@vger.kernel.org, reiserfs-devel@vger.kernel.org, linux-geode@lists.infradead.org, linux-block@vger.kernel.org, linux-gpio@vger.kernel.org, op-tee@lists.trustedfirmware.org, linux-mediatek@lists.infradead.org, xen-devel@lists.xenproject.org, drbd-dev@tron.linbit.com, linux-hams@vger.kernel.org, Nathan Chancellor , linux-can@vger.kernel.org, Linux ARM , linux-hwmon@vger.kernel.org, "maintainer:X86 ARCHITECTURE \(32-BIT AND 64-BIT\)" , linux-nfs@vger.kernel.org, GR-Linux-NIC-Dev@marvell.com, nouveau@lists.freedesktop.org, Network Development , linux-decnet-user@lists.sourceforge.net, linux-wireless , LKML , Linux-Renesas , linux-security-module@vger.kernel.org, linux-usb@vger.kernel.org, tipc-discussion@lists.sourceforge.net, "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , Jonathan Cameron , patches@opensource.cirrus.com, Joe Perches , linux-integrity@vger.kernel.org, linux-hardening@vger.kernel.org Subject: Re: [dm-devel] [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang X-BeenThere: dm-devel@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: device-mapper development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dm-devel-bounces@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Tue, Nov 24, 2020 at 11:05 PM James Bottomley wrote: > > On Tue, 2020-11-24 at 13:32 -0800, Kees Cook wrote: > > We already enable -Wimplicit-fallthrough globally, so that's not the > > discussion. The issue is that Clang is (correctly) even more strict > > than GCC for this, so these are the remaining ones to fix for full > > Clang coverage too. > > > > People have spent more time debating this already than it would have > > taken to apply the patches. :) > > You mean we've already spent 90% of the effort to come this far so we > might as well go the remaining 10% because then at least we get some > return? It's certainly a clinching argument in defence procurement ... So developers and distributions using Clang can't have -Wimplicit-fallthrough enabled because GCC is less strict (which has been shown in this thread to lead to bugs)? We'd like to have nice things too, you know. I even agree that most of the churn comes from case 0: ++x; default: break; which I have a patch for: https://reviews.llvm.org/D91895. I agree that can never lead to bugs. But that's not the sole case of this series, just most of them. Though, note how the reviewer (C++ spec editor and clang front end owner) in https://reviews.llvm.org/D91895 even asks in that review how maybe a new flag would be more appropriate for a watered down/stylistic variant of the existing behavior. And if the current wording of Documentation/process/deprecated.rst around "fallthrough" is a straightforward rule of thumb, I kind of agree with him. > > > This is about robustness and language wrangling. It's a big code- > > base, and this is the price of our managing technical debt for > > permanent robustness improvements. (The numbers I ran from Gustavo's > > earlier patches were that about 10% of the places adjusted were > > identified as legitimate bugs being fixed. This final series may be > > lower, but there are still bugs being found from it -- we need to > > finish this and shut the door on it for good.) > > I got my six patches by analyzing the lwn.net report of the fixes that > was cited which had 21 of which 50% didn't actually change the emitted > code, and 25% didn't have a user visible effect. > > But the broader point I'm making is just because the compiler people > come up with a shiny new warning doesn't necessarily mean the problem That's not what this is though; you're attacking a strawman. I'd encourage you to bring that up when that actually occurs, unlike this case since it's actively hindering getting -Wimplicit-fallthrough enabled for Clang. This is not a shiny new warning; it's already on for GCC and has existed in both compilers for multiple releases. And I'll also note that warnings are warnings and not errors because they cannot be proven to be bugs in 100% of cases, but they have led to bugs in the past. They require a human to review their intent and remove ambiguities. If 97% of cases would end in a break ("Expert C Programming: Deep C Secrets" - Peter van der Linden), then it starts to look to me like a language defect; certainly an incorrectly chosen default. But the compiler can't know those 3% were intentional, unless you're explicit for those exceptional cases. > it's detecting is one that causes us actual problems in the code base. > I'd really be happier if we had a theory about what classes of CVE or > bug we could eliminate before we embrace the next new warning. We don't generally file CVEs and waiting for them to occur might be too reactive, but I agree that pointing to some additional documentation in commit messages about how a warning could lead to a bug would make it clearer to reviewers why being able to enable it treewide, even if there's no bug in their particular subsystem, is in the general interest of the commons. On Mon, Nov 23, 2020 at 7:58 AM James Bottomley wrote: > > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 > > The whole crux of your argument seems to be maintainers' time isn't > important so we should accept all trivial patches ... I'm pushing back > on that assumption in two places, firstly the valulessness of the time > and secondly that all trivial patches are valuable. It's critical to the longevity of any open source project that there are not single points of failure. If someone is not expendable or replaceable (or claims to be) then that's a risk to the project and a bottleneck. Not having a replacement in training or some form of redundancy is short sighted. If trivial patches are adding too much to your workload, consider training a co-maintainer or asking for help from one of your reviewers whom you trust. I don't doubt it's hard to find maintainers, but existing maintainers should go out of their way to entrust co-maintainers especially when they find their workload becomes too high. And reviewing/picking up trivial patches is probably a great way to get started. If we allow too much knowledge of any one subsystem to collect with one maintainer, what happens when that maintainer leaves the community (which, given a finite lifespan, is an inevitability)? -- Thanks, ~Nick Desaulniers -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.5 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 02907C63777 for ; Wed, 25 Nov 2020 12:24:42 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 8BC4020BED for ; Wed, 25 Nov 2020 12:24:41 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=google.com header.i=@google.com header.b="GyrkIL7r" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8BC4020BED Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id B39E66E8AF; Wed, 25 Nov 2020 12:24:40 +0000 (UTC) Received: from mail-pf1-x441.google.com (mail-pf1-x441.google.com [IPv6:2607:f8b0:4864:20::441]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2653A6E8AF for ; Wed, 25 Nov 2020 12:24:39 +0000 (UTC) Received: by mail-pf1-x441.google.com with SMTP id w6so2188673pfu.1 for ; Wed, 25 Nov 2020 04:24:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=GyrkIL7rJc/Wrkz9wtYqXZYvGBry6qXFkQono0nmrBFDlUCiGmbX9ByD1wUhih87ZW XCd/8etF0h65aGuVNHVvGVnSoIRV2cIFxWeuMsMEKDZ+SIKsK6eM3KIHPaY2Au+pxfCB jFmSmO0a8jtSnIjbAi/709gkMW9hnqxggrhUNIGI/2GrlejcLn7tyz9MFlSEpE31y19Z 9ARaZhNBbaKZzII6ioDoEFmbbi01XI+4/fF65wWR3SGfZuCMoV2cgGUJ8Osa8sFeqdbz KLllcyBsC3gtRPDbq4Yc+z3inKaZT7D05cYTb7CBHNDdR/afLH0A7E5JHrBWilMAefn4 uS6g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=A87ouYXAJH+UL7ulUGA4wToMbLTD3AuLYzFDjhfUdMJKzhE6NZKWu5IuMrxwWlOjEE QP2aiyG5be4ptNW/6Oz3fTOeS8ujnN/jn1sm0sRPnCI1TTFdQm3VzRiXVe+Z53DKfWrM vwmeYN+OidrCF9DFQb5RGBjn4qnQlofDnaxPooibLq4Ephw9/yxNbn6uahNjw3V+/aG6 w8c8m3UQr0vkiJw9ZMv0ozTFcPCna+efidBgScJtHTukdj1o6CJ59fI2+LoslQ2Fu987 pBujEzTGblm5ZeVJLACVYWjijwT9VublMv4251VEDdnTzkOMtWQJGJCpsZsR7UwzanU0 VcDw== X-Gm-Message-State: AOAM5327TNPHmYbTc/dgS1YBVU4FAg1boQanikN5ZO3FqdRsfbiuPC5G WTWoiyOxxW5IY/sJ1eGN1of9v7ipS6w0ohiZ2OQBoA== X-Google-Smtp-Source: ABdhPJySo35UzNwHodlreVMfJuWPwHO1z+zkcbFfSYU3Avf+sN4n16LJPBb97SBockWyJEKx3Xs8q1wCvzejZmwrmAM= X-Received: by 2002:a62:7905:0:b029:197:f300:5a2a with SMTP id u5-20020a6279050000b0290197f3005a2amr2898775pfc.30.1606307078380; Wed, 25 Nov 2020 04:24:38 -0800 (PST) MIME-Version: 1.0 References: <202011201129.B13FDB3C@keescook> <20201120115142.292999b2@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> <202011220816.8B6591A@keescook> <9b57fd4914b46f38d54087d75e072d6e947cb56d.camel@HansenPartnership.com> <0147972a72bc13f3629de8a32dee6f1f308994b5.camel@HansenPartnership.com> <20201123130348.GA3119@embeddedor> <8f5611bb015e044fa1c0a48147293923c2d904e4.camel@HansenPartnership.com> <202011241327.BB28F12F6@keescook> In-Reply-To: From: Nick Desaulniers Date: Wed, 25 Nov 2020 04:24:27 -0800 Message-ID: To: James Bottomley Subject: Re: [Intel-gfx] [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: alsa-devel@alsa-project.org, bridge@lists.linux-foundation.org, target-devel@vger.kernel.org, Greg KH , linux-iio@vger.kernel.org, samba-technical@lists.samba.org, linux-mmc@vger.kernel.org, linux-fbdev@vger.kernel.org, dri-devel , "Gustavo A. R. Silva" , linux-ide@vger.kernel.org, dm-devel@redhat.com, keyrings@vger.kernel.org, linux-mtd@lists.infradead.org, GR-everest-linux-l2@marvell.com, wcn36xx@lists.infradead.org, linux-i3c@lists.infradead.org, linux1394-devel@lists.sourceforge.net, linux-afs@lists.infradead.org, drbd-dev@lists.linbit.com, devel@driverdev.osuosl.org, linux-cifs@vger.kernel.org, rds-devel@oss.oracle.com, linux-scsi@vger.kernel.org, linux-acpi@vger.kernel.org, linux-rdma@vger.kernel.org, oss-drivers@netronome.com, linux-atm-general@lists.sourceforge.net, ceph-devel@vger.kernel.org, amd-gfx list , linux-stm32@st-md-mailman.stormreply.com, cluster-devel@redhat.com, usb-storage@lists.one-eyed-alien.net, coreteam@netfilter.org, intel-wired-lan@lists.osuosl.org, linux-input@vger.kernel.org, Miguel Ojeda , Jakub Kicinski , linux-ext4@vger.kernel.org, virtualization@lists.linux-foundation.org, netfilter-devel@vger.kernel.org, linux-media@vger.kernel.org, Kees Cook , selinux@vger.kernel.org, linux-arm-msm , intel-gfx@lists.freedesktop.org, linux-sctp@vger.kernel.org, reiserfs-devel@vger.kernel.org, linux-geode@lists.infradead.org, linux-block@vger.kernel.org, linux-gpio@vger.kernel.org, op-tee@lists.trustedfirmware.org, linux-mediatek@lists.infradead.org, xen-devel@lists.xenproject.org, nouveau@lists.freedesktop.org, linux-hams@vger.kernel.org, Nathan Chancellor , linux-can@vger.kernel.org, Linux ARM , linux-hwmon@vger.kernel.org, "maintainer:X86 ARCHITECTURE \(32-BIT AND 64-BIT\)" , linux-watchdog@vger.kernel.org, GR-Linux-NIC-Dev@marvell.com, Linux Memory Management List , Network Development , linux-decnet-user@lists.sourceforge.net, linux-wireless , LKML , Linux-Renesas , linux-security-module@vger.kernel.org, linux-usb@vger.kernel.org, tipc-discussion@lists.sourceforge.net, "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , Jonathan Cameron , patches@opensource.cirrus.com, Joe Perches , linux-integrity@vger.kernel.org, linux-nfs@vger.kernel.org, linux-hardening@vger.kernel.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Tue, Nov 24, 2020 at 11:05 PM James Bottomley wrote: > > On Tue, 2020-11-24 at 13:32 -0800, Kees Cook wrote: > > We already enable -Wimplicit-fallthrough globally, so that's not the > > discussion. The issue is that Clang is (correctly) even more strict > > than GCC for this, so these are the remaining ones to fix for full > > Clang coverage too. > > > > People have spent more time debating this already than it would have > > taken to apply the patches. :) > > You mean we've already spent 90% of the effort to come this far so we > might as well go the remaining 10% because then at least we get some > return? It's certainly a clinching argument in defence procurement ... So developers and distributions using Clang can't have -Wimplicit-fallthrough enabled because GCC is less strict (which has been shown in this thread to lead to bugs)? We'd like to have nice things too, you know. I even agree that most of the churn comes from case 0: ++x; default: break; which I have a patch for: https://reviews.llvm.org/D91895. I agree that can never lead to bugs. But that's not the sole case of this series, just most of them. Though, note how the reviewer (C++ spec editor and clang front end owner) in https://reviews.llvm.org/D91895 even asks in that review how maybe a new flag would be more appropriate for a watered down/stylistic variant of the existing behavior. And if the current wording of Documentation/process/deprecated.rst around "fallthrough" is a straightforward rule of thumb, I kind of agree with him. > > > This is about robustness and language wrangling. It's a big code- > > base, and this is the price of our managing technical debt for > > permanent robustness improvements. (The numbers I ran from Gustavo's > > earlier patches were that about 10% of the places adjusted were > > identified as legitimate bugs being fixed. This final series may be > > lower, but there are still bugs being found from it -- we need to > > finish this and shut the door on it for good.) > > I got my six patches by analyzing the lwn.net report of the fixes that > was cited which had 21 of which 50% didn't actually change the emitted > code, and 25% didn't have a user visible effect. > > But the broader point I'm making is just because the compiler people > come up with a shiny new warning doesn't necessarily mean the problem That's not what this is though; you're attacking a strawman. I'd encourage you to bring that up when that actually occurs, unlike this case since it's actively hindering getting -Wimplicit-fallthrough enabled for Clang. This is not a shiny new warning; it's already on for GCC and has existed in both compilers for multiple releases. And I'll also note that warnings are warnings and not errors because they cannot be proven to be bugs in 100% of cases, but they have led to bugs in the past. They require a human to review their intent and remove ambiguities. If 97% of cases would end in a break ("Expert C Programming: Deep C Secrets" - Peter van der Linden), then it starts to look to me like a language defect; certainly an incorrectly chosen default. But the compiler can't know those 3% were intentional, unless you're explicit for those exceptional cases. > it's detecting is one that causes us actual problems in the code base. > I'd really be happier if we had a theory about what classes of CVE or > bug we could eliminate before we embrace the next new warning. We don't generally file CVEs and waiting for them to occur might be too reactive, but I agree that pointing to some additional documentation in commit messages about how a warning could lead to a bug would make it clearer to reviewers why being able to enable it treewide, even if there's no bug in their particular subsystem, is in the general interest of the commons. On Mon, Nov 23, 2020 at 7:58 AM James Bottomley wrote: > > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 > > The whole crux of your argument seems to be maintainers' time isn't > important so we should accept all trivial patches ... I'm pushing back > on that assumption in two places, firstly the valulessness of the time > and secondly that all trivial patches are valuable. It's critical to the longevity of any open source project that there are not single points of failure. If someone is not expendable or replaceable (or claims to be) then that's a risk to the project and a bottleneck. Not having a replacement in training or some form of redundancy is short sighted. If trivial patches are adding too much to your workload, consider training a co-maintainer or asking for help from one of your reviewers whom you trust. I don't doubt it's hard to find maintainers, but existing maintainers should go out of their way to entrust co-maintainers especially when they find their workload becomes too high. And reviewing/picking up trivial patches is probably a great way to get started. If we allow too much knowledge of any one subsystem to collect with one maintainer, what happens when that maintainer leaves the community (which, given a finite lifespan, is an inevitability)? -- Thanks, ~Nick Desaulniers _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.5 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 56F78C56202 for ; Wed, 25 Nov 2020 13:51:05 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 97D0320656 for ; Wed, 25 Nov 2020 13:51:04 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=google.com header.i=@google.com header.b="GyrkIL7r" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 97D0320656 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=amd-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 719206E9A7; Wed, 25 Nov 2020 13:51:03 +0000 (UTC) Received: from mail-pf1-x441.google.com (mail-pf1-x441.google.com [IPv6:2607:f8b0:4864:20::441]) by gabe.freedesktop.org (Postfix) with ESMTPS id 26AA56E977 for ; Wed, 25 Nov 2020 12:24:39 +0000 (UTC) Received: by mail-pf1-x441.google.com with SMTP id w187so2171518pfd.5 for ; Wed, 25 Nov 2020 04:24:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=GyrkIL7rJc/Wrkz9wtYqXZYvGBry6qXFkQono0nmrBFDlUCiGmbX9ByD1wUhih87ZW XCd/8etF0h65aGuVNHVvGVnSoIRV2cIFxWeuMsMEKDZ+SIKsK6eM3KIHPaY2Au+pxfCB jFmSmO0a8jtSnIjbAi/709gkMW9hnqxggrhUNIGI/2GrlejcLn7tyz9MFlSEpE31y19Z 9ARaZhNBbaKZzII6ioDoEFmbbi01XI+4/fF65wWR3SGfZuCMoV2cgGUJ8Osa8sFeqdbz KLllcyBsC3gtRPDbq4Yc+z3inKaZT7D05cYTb7CBHNDdR/afLH0A7E5JHrBWilMAefn4 uS6g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=T6619q+h4WQPUYleKWIBwUDaBArLFJ0JyPdhpY1DAmME00ds22ZKql2ivEBDEhZle/ vPONMZO4LGWhVhuDEdnj891JC0e4jp0JUL2eBV3D4GdT89LAklpzMKv/Lu0zCmdOO/GR uiOT21/FTmARzxpBT7jS1pjQEo4lX8WoECAkxwbAHiuKMK5X98aMphGXmUoLzHYkzJ98 uyRVZDIsY1LQbwAM3dlE602h36NCLoIAyvR4KQGbdZtqrS402sid49Oy4kSfSASmKUsT PkkkAfsy4AKgYNYrBG/U6TslKqMqKqm7Ju+Deq3B9CPe4QdyZicfDdwVY0R/MOsf2/80 vNmA== X-Gm-Message-State: AOAM531Jjo/4e81C7KbyW9J1Pu9ZKFc00FmLoNNB5FLwj6w0yY3JHwBo er1U4Vlh9w9Ku2MsQ08j/R5cC56WOHElVcZNNN1CWg== X-Google-Smtp-Source: ABdhPJySo35UzNwHodlreVMfJuWPwHO1z+zkcbFfSYU3Avf+sN4n16LJPBb97SBockWyJEKx3Xs8q1wCvzejZmwrmAM= X-Received: by 2002:a62:7905:0:b029:197:f300:5a2a with SMTP id u5-20020a6279050000b0290197f3005a2amr2898775pfc.30.1606307078380; Wed, 25 Nov 2020 04:24:38 -0800 (PST) MIME-Version: 1.0 References: <202011201129.B13FDB3C@keescook> <20201120115142.292999b2@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> <202011220816.8B6591A@keescook> <9b57fd4914b46f38d54087d75e072d6e947cb56d.camel@HansenPartnership.com> <0147972a72bc13f3629de8a32dee6f1f308994b5.camel@HansenPartnership.com> <20201123130348.GA3119@embeddedor> <8f5611bb015e044fa1c0a48147293923c2d904e4.camel@HansenPartnership.com> <202011241327.BB28F12F6@keescook> In-Reply-To: From: Nick Desaulniers Date: Wed, 25 Nov 2020 04:24:27 -0800 Message-ID: Subject: Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang To: James Bottomley X-Mailman-Approved-At: Wed, 25 Nov 2020 13:51:01 +0000 X-BeenThere: amd-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: alsa-devel@alsa-project.org, bridge@lists.linux-foundation.org, target-devel@vger.kernel.org, Greg KH , linux-iio@vger.kernel.org, samba-technical@lists.samba.org, linux-mmc@vger.kernel.org, linux-fbdev@vger.kernel.org, dri-devel , "Gustavo A. R. Silva" , linux-ide@vger.kernel.org, dm-devel@redhat.com, keyrings@vger.kernel.org, linux-mtd@lists.infradead.org, GR-everest-linux-l2@marvell.com, wcn36xx@lists.infradead.org, linux-i3c@lists.infradead.org, linux1394-devel@lists.sourceforge.net, linux-afs@lists.infradead.org, drbd-dev@lists.linbit.com, devel@driverdev.osuosl.org, linux-cifs@vger.kernel.org, rds-devel@oss.oracle.com, linux-scsi@vger.kernel.org, linux-acpi@vger.kernel.org, linux-rdma@vger.kernel.org, oss-drivers@netronome.com, linux-atm-general@lists.sourceforge.net, ceph-devel@vger.kernel.org, amd-gfx list , linux-stm32@st-md-mailman.stormreply.com, cluster-devel@redhat.com, usb-storage@lists.one-eyed-alien.net, coreteam@netfilter.org, intel-wired-lan@lists.osuosl.org, linux-input@vger.kernel.org, Miguel Ojeda , Jakub Kicinski , linux-ext4@vger.kernel.org, virtualization@lists.linux-foundation.org, netfilter-devel@vger.kernel.org, linux-media@vger.kernel.org, Kees Cook , selinux@vger.kernel.org, linux-arm-msm , intel-gfx@lists.freedesktop.org, linux-sctp@vger.kernel.org, reiserfs-devel@vger.kernel.org, linux-geode@lists.infradead.org, linux-block@vger.kernel.org, linux-gpio@vger.kernel.org, op-tee@lists.trustedfirmware.org, linux-mediatek@lists.infradead.org, xen-devel@lists.xenproject.org, nouveau@lists.freedesktop.org, linux-hams@vger.kernel.org, Nathan Chancellor , linux-can@vger.kernel.org, Linux ARM , linux-hwmon@vger.kernel.org, "maintainer:X86 ARCHITECTURE \(32-BIT AND 64-BIT\)" , linux-watchdog@vger.kernel.org, GR-Linux-NIC-Dev@marvell.com, Linux Memory Management List , Network Development , linux-decnet-user@lists.sourceforge.net, linux-wireless , LKML , Linux-Renesas , linux-security-module@vger.kernel.org, linux-usb@vger.kernel.org, tipc-discussion@lists.sourceforge.net, "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , Jonathan Cameron , patches@opensource.cirrus.com, Joe Perches , linux-integrity@vger.kernel.org, linux-nfs@vger.kernel.org, linux-hardening@vger.kernel.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: amd-gfx-bounces@lists.freedesktop.org Sender: "amd-gfx" On Tue, Nov 24, 2020 at 11:05 PM James Bottomley wrote: > > On Tue, 2020-11-24 at 13:32 -0800, Kees Cook wrote: > > We already enable -Wimplicit-fallthrough globally, so that's not the > > discussion. The issue is that Clang is (correctly) even more strict > > than GCC for this, so these are the remaining ones to fix for full > > Clang coverage too. > > > > People have spent more time debating this already than it would have > > taken to apply the patches. :) > > You mean we've already spent 90% of the effort to come this far so we > might as well go the remaining 10% because then at least we get some > return? It's certainly a clinching argument in defence procurement ... So developers and distributions using Clang can't have -Wimplicit-fallthrough enabled because GCC is less strict (which has been shown in this thread to lead to bugs)? We'd like to have nice things too, you know. I even agree that most of the churn comes from case 0: ++x; default: break; which I have a patch for: https://reviews.llvm.org/D91895. I agree that can never lead to bugs. But that's not the sole case of this series, just most of them. Though, note how the reviewer (C++ spec editor and clang front end owner) in https://reviews.llvm.org/D91895 even asks in that review how maybe a new flag would be more appropriate for a watered down/stylistic variant of the existing behavior. And if the current wording of Documentation/process/deprecated.rst around "fallthrough" is a straightforward rule of thumb, I kind of agree with him. > > > This is about robustness and language wrangling. It's a big code- > > base, and this is the price of our managing technical debt for > > permanent robustness improvements. (The numbers I ran from Gustavo's > > earlier patches were that about 10% of the places adjusted were > > identified as legitimate bugs being fixed. This final series may be > > lower, but there are still bugs being found from it -- we need to > > finish this and shut the door on it for good.) > > I got my six patches by analyzing the lwn.net report of the fixes that > was cited which had 21 of which 50% didn't actually change the emitted > code, and 25% didn't have a user visible effect. > > But the broader point I'm making is just because the compiler people > come up with a shiny new warning doesn't necessarily mean the problem That's not what this is though; you're attacking a strawman. I'd encourage you to bring that up when that actually occurs, unlike this case since it's actively hindering getting -Wimplicit-fallthrough enabled for Clang. This is not a shiny new warning; it's already on for GCC and has existed in both compilers for multiple releases. And I'll also note that warnings are warnings and not errors because they cannot be proven to be bugs in 100% of cases, but they have led to bugs in the past. They require a human to review their intent and remove ambiguities. If 97% of cases would end in a break ("Expert C Programming: Deep C Secrets" - Peter van der Linden), then it starts to look to me like a language defect; certainly an incorrectly chosen default. But the compiler can't know those 3% were intentional, unless you're explicit for those exceptional cases. > it's detecting is one that causes us actual problems in the code base. > I'd really be happier if we had a theory about what classes of CVE or > bug we could eliminate before we embrace the next new warning. We don't generally file CVEs and waiting for them to occur might be too reactive, but I agree that pointing to some additional documentation in commit messages about how a warning could lead to a bug would make it clearer to reviewers why being able to enable it treewide, even if there's no bug in their particular subsystem, is in the general interest of the commons. On Mon, Nov 23, 2020 at 7:58 AM James Bottomley wrote: > > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 > > The whole crux of your argument seems to be maintainers' time isn't > important so we should accept all trivial patches ... I'm pushing back > on that assumption in two places, firstly the valulessness of the time > and secondly that all trivial patches are valuable. It's critical to the longevity of any open source project that there are not single points of failure. If someone is not expendable or replaceable (or claims to be) then that's a risk to the project and a bottleneck. Not having a replacement in training or some form of redundancy is short sighted. If trivial patches are adding too much to your workload, consider training a co-maintainer or asking for help from one of your reviewers whom you trust. I don't doubt it's hard to find maintainers, but existing maintainers should go out of their way to entrust co-maintainers especially when they find their workload becomes too high. And reviewing/picking up trivial patches is probably a great way to get started. If we allow too much knowledge of any one subsystem to collect with one maintainer, what happens when that maintainer leaves the community (which, given a finite lifespan, is an inevitability)? -- Thanks, ~Nick Desaulniers _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E0991C64E7B for ; Tue, 1 Dec 2020 07:39:27 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 43B7E2085B for ; Tue, 1 Dec 2020 07:39:27 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="on7kNOOS"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=google.com header.i=@google.com header.b="GyrkIL7r" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 43B7E2085B Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-i3c-bounces+linux-i3c=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=3S941vxmLH5Lhq/rWnQIrafuxKSXr9gj3CZHfjhOD/w=; b=on7kNOOSdO+E5OYqDvPT7DUUh yq8mHM++k/EifkShV6goKu6BtkWEe51qfME3+1UQvHBM4NW7EemIzdSAztcsPODYxsswH7pBs2lYh D7n3qxedBfzXFQzsR1X6d8L0Cri+9DgqO2Jtf905o91SZKm95wjkkz52OPShBUqN7ibV1jb0VJ4SF QsxIso7cwRm1RER4cDMWFD4u6tr3/WZ/lujnj1wVUPIKcjT5vTwNv43CxcKWjvbyKH/998J9viwXw AahfneJV54NrOf8ZgU+NoOU8Cc1w4YhoDyIqetqyEXQhVvSHoUmmpd10Lwe7TMuRnlFu9yB5MW+Kw k5L2BV0Tg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kk0Fy-0007Ra-6R; Tue, 01 Dec 2020 07:39:26 +0000 Received: from mail-pg1-x544.google.com ([2607:f8b0:4864:20::544]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1khtqj-0008WI-1e for linux-i3c@lists.infradead.org; Wed, 25 Nov 2020 12:24:42 +0000 Received: by mail-pg1-x544.google.com with SMTP id t21so2330397pgl.3 for ; Wed, 25 Nov 2020 04:24:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=GyrkIL7rJc/Wrkz9wtYqXZYvGBry6qXFkQono0nmrBFDlUCiGmbX9ByD1wUhih87ZW XCd/8etF0h65aGuVNHVvGVnSoIRV2cIFxWeuMsMEKDZ+SIKsK6eM3KIHPaY2Au+pxfCB jFmSmO0a8jtSnIjbAi/709gkMW9hnqxggrhUNIGI/2GrlejcLn7tyz9MFlSEpE31y19Z 9ARaZhNBbaKZzII6ioDoEFmbbi01XI+4/fF65wWR3SGfZuCMoV2cgGUJ8Osa8sFeqdbz KLllcyBsC3gtRPDbq4Yc+z3inKaZT7D05cYTb7CBHNDdR/afLH0A7E5JHrBWilMAefn4 uS6g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=tWeVZ4pcQl/8/RHU9z5hNTvKRVD30kabz+UqKDj86J+yKPpzxXsE2Fj0PfnPJQk43P TXpz4cqUUHKnxq4ntbellPUlemi/wLkBz733kQcJNfgDpw7QW7ae1MQYmI65V/WHAiqF G39ZJ0GM3ps5u83xpByL5EbkYbL4BrrXUc4XH/UEGAgXuduDJ4ARSEXeRpkGCvhUZV7v LDLhSMIxSD1dKCLJEoC+UGIgPyYuVGNdhMvSZta5iTp6I8EBiaP6mylpOYWVf+WvwPSr k7MfjAljUK4MdWRRaJ51SQJe+5xVw6k8VwixoCq3k7jjMNCIIJ+mm/7f9DXKRrkNPjpR i7Hw== X-Gm-Message-State: AOAM530mmn0ABoCyVnGOA9+2IWxcUTokXbazL+wbVL2zvDcMdZ7iPIH2 ejDUUJE2vGlXHIpqrvkwILL5NIR0hqsh6WaneP7eTw== X-Google-Smtp-Source: ABdhPJySo35UzNwHodlreVMfJuWPwHO1z+zkcbFfSYU3Avf+sN4n16LJPBb97SBockWyJEKx3Xs8q1wCvzejZmwrmAM= X-Received: by 2002:a62:7905:0:b029:197:f300:5a2a with SMTP id u5-20020a6279050000b0290197f3005a2amr2898775pfc.30.1606307078380; Wed, 25 Nov 2020 04:24:38 -0800 (PST) MIME-Version: 1.0 References: <202011201129.B13FDB3C@keescook> <20201120115142.292999b2@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> <202011220816.8B6591A@keescook> <9b57fd4914b46f38d54087d75e072d6e947cb56d.camel@HansenPartnership.com> <0147972a72bc13f3629de8a32dee6f1f308994b5.camel@HansenPartnership.com> <20201123130348.GA3119@embeddedor> <8f5611bb015e044fa1c0a48147293923c2d904e4.camel@HansenPartnership.com> <202011241327.BB28F12F6@keescook> In-Reply-To: From: Nick Desaulniers Date: Wed, 25 Nov 2020 04:24:27 -0800 Message-ID: Subject: Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang To: James Bottomley X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201125_072441_132372_4742F8B7 X-CRM114-Status: GOOD ( 35.78 ) X-Mailman-Approved-At: Tue, 01 Dec 2020 02:39:24 -0500 X-BeenThere: linux-i3c@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: alsa-devel@alsa-project.org, bridge@lists.linux-foundation.org, target-devel@vger.kernel.org, Greg KH , linux-iio@vger.kernel.org, samba-technical@lists.samba.org, Jonathan Cameron , linux-fbdev@vger.kernel.org, dri-devel , "Gustavo A. R. Silva" , linux-ide@vger.kernel.org, dm-devel@redhat.com, keyrings@vger.kernel.org, linux-mtd@lists.infradead.org, GR-everest-linux-l2@marvell.com, wcn36xx@lists.infradead.org, linux-i3c@lists.infradead.org, linux1394-devel@lists.sourceforge.net, linux-afs@lists.infradead.org, drbd-dev@lists.linbit.com, devel@driverdev.osuosl.org, linux-cifs@vger.kernel.org, rds-devel@oss.oracle.com, linux-scsi@vger.kernel.org, linux-acpi@vger.kernel.org, linux-rdma@vger.kernel.org, oss-drivers@netronome.com, linux-atm-general@lists.sourceforge.net, ceph-devel@vger.kernel.org, amd-gfx list , linux-stm32@st-md-mailman.stormreply.com, cluster-devel@redhat.com, usb-storage@lists.one-eyed-alien.net, linux-mmc@vger.kernel.org, coreteam@netfilter.org, intel-wired-lan@lists.osuosl.org, linux-input@vger.kernel.org, Miguel Ojeda , Jakub Kicinski , linux-ext4@vger.kernel.org, virtualization@lists.linux-foundation.org, netfilter-devel@vger.kernel.org, linux-media@vger.kernel.org, Kees Cook , selinux@vger.kernel.org, linux-arm-msm , intel-gfx@lists.freedesktop.org, linux-sctp@vger.kernel.org, reiserfs-devel@vger.kernel.org, linux-geode@lists.infradead.org, linux-block@vger.kernel.org, linux-gpio@vger.kernel.org, op-tee@lists.trustedfirmware.org, linux-mediatek@lists.infradead.org, xen-devel@lists.xenproject.org, nouveau@lists.freedesktop.org, linux-hams@vger.kernel.org, Nathan Chancellor , linux-can@vger.kernel.org, Linux ARM , linux-hwmon@vger.kernel.org, "maintainer:X86 ARCHITECTURE \(32-BIT AND 64-BIT\)" , linux-watchdog@vger.kernel.org, GR-Linux-NIC-Dev@marvell.com, Linux Memory Management List , Network Development , linux-decnet-user@lists.sourceforge.net, linux-wireless , LKML , Linux-Renesas , linux-security-module@vger.kernel.org, linux-usb@vger.kernel.org, tipc-discussion@lists.sourceforge.net, "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , patches@opensource.cirrus.com, Joe Perches , linux-integrity@vger.kernel.org, linux-nfs@vger.kernel.org, linux-hardening@vger.kernel.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-i3c" Errors-To: linux-i3c-bounces+linux-i3c=archiver.kernel.org@lists.infradead.org On Tue, Nov 24, 2020 at 11:05 PM James Bottomley wrote: > > On Tue, 2020-11-24 at 13:32 -0800, Kees Cook wrote: > > We already enable -Wimplicit-fallthrough globally, so that's not the > > discussion. The issue is that Clang is (correctly) even more strict > > than GCC for this, so these are the remaining ones to fix for full > > Clang coverage too. > > > > People have spent more time debating this already than it would have > > taken to apply the patches. :) > > You mean we've already spent 90% of the effort to come this far so we > might as well go the remaining 10% because then at least we get some > return? It's certainly a clinching argument in defence procurement ... So developers and distributions using Clang can't have -Wimplicit-fallthrough enabled because GCC is less strict (which has been shown in this thread to lead to bugs)? We'd like to have nice things too, you know. I even agree that most of the churn comes from case 0: ++x; default: break; which I have a patch for: https://reviews.llvm.org/D91895. I agree that can never lead to bugs. But that's not the sole case of this series, just most of them. Though, note how the reviewer (C++ spec editor and clang front end owner) in https://reviews.llvm.org/D91895 even asks in that review how maybe a new flag would be more appropriate for a watered down/stylistic variant of the existing behavior. And if the current wording of Documentation/process/deprecated.rst around "fallthrough" is a straightforward rule of thumb, I kind of agree with him. > > > This is about robustness and language wrangling. It's a big code- > > base, and this is the price of our managing technical debt for > > permanent robustness improvements. (The numbers I ran from Gustavo's > > earlier patches were that about 10% of the places adjusted were > > identified as legitimate bugs being fixed. This final series may be > > lower, but there are still bugs being found from it -- we need to > > finish this and shut the door on it for good.) > > I got my six patches by analyzing the lwn.net report of the fixes that > was cited which had 21 of which 50% didn't actually change the emitted > code, and 25% didn't have a user visible effect. > > But the broader point I'm making is just because the compiler people > come up with a shiny new warning doesn't necessarily mean the problem That's not what this is though; you're attacking a strawman. I'd encourage you to bring that up when that actually occurs, unlike this case since it's actively hindering getting -Wimplicit-fallthrough enabled for Clang. This is not a shiny new warning; it's already on for GCC and has existed in both compilers for multiple releases. And I'll also note that warnings are warnings and not errors because they cannot be proven to be bugs in 100% of cases, but they have led to bugs in the past. They require a human to review their intent and remove ambiguities. If 97% of cases would end in a break ("Expert C Programming: Deep C Secrets" - Peter van der Linden), then it starts to look to me like a language defect; certainly an incorrectly chosen default. But the compiler can't know those 3% were intentional, unless you're explicit for those exceptional cases. > it's detecting is one that causes us actual problems in the code base. > I'd really be happier if we had a theory about what classes of CVE or > bug we could eliminate before we embrace the next new warning. We don't generally file CVEs and waiting for them to occur might be too reactive, but I agree that pointing to some additional documentation in commit messages about how a warning could lead to a bug would make it clearer to reviewers why being able to enable it treewide, even if there's no bug in their particular subsystem, is in the general interest of the commons. On Mon, Nov 23, 2020 at 7:58 AM James Bottomley wrote: > > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 > > The whole crux of your argument seems to be maintainers' time isn't > important so we should accept all trivial patches ... I'm pushing back > on that assumption in two places, firstly the valulessness of the time > and secondly that all trivial patches are valuable. It's critical to the longevity of any open source project that there are not single points of failure. If someone is not expendable or replaceable (or claims to be) then that's a risk to the project and a bottleneck. Not having a replacement in training or some form of redundancy is short sighted. If trivial patches are adding too much to your workload, consider training a co-maintainer or asking for help from one of your reviewers whom you trust. I don't doubt it's hard to find maintainers, but existing maintainers should go out of their way to entrust co-maintainers especially when they find their workload becomes too high. And reviewing/picking up trivial patches is probably a great way to get started. If we allow too much knowledge of any one subsystem to collect with one maintainer, what happens when that maintainer leaves the community (which, given a finite lifespan, is an inevitability)? -- Thanks, ~Nick Desaulniers -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nick Desaulniers Date: Wed, 25 Nov 2020 04:24:27 -0800 Subject: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang In-Reply-To: References: <202011201129.B13FDB3C@keescook> <20201120115142.292999b2@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> <202011220816.8B6591A@keescook> <9b57fd4914b46f38d54087d75e072d6e947cb56d.camel@HansenPartnership.com> <0147972a72bc13f3629de8a32dee6f1f308994b5.camel@HansenPartnership.com> <20201123130348.GA3119@embeddedor> <8f5611bb015e044fa1c0a48147293923c2d904e4.camel@HansenPartnership.com> <202011241327.BB28F12F6@keescook> Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: intel-wired-lan@osuosl.org List-ID: On Tue, Nov 24, 2020 at 11:05 PM James Bottomley wrote: > > On Tue, 2020-11-24 at 13:32 -0800, Kees Cook wrote: > > We already enable -Wimplicit-fallthrough globally, so that's not the > > discussion. The issue is that Clang is (correctly) even more strict > > than GCC for this, so these are the remaining ones to fix for full > > Clang coverage too. > > > > People have spent more time debating this already than it would have > > taken to apply the patches. :) > > You mean we've already spent 90% of the effort to come this far so we > might as well go the remaining 10% because then at least we get some > return? It's certainly a clinching argument in defence procurement ... So developers and distributions using Clang can't have -Wimplicit-fallthrough enabled because GCC is less strict (which has been shown in this thread to lead to bugs)? We'd like to have nice things too, you know. I even agree that most of the churn comes from case 0: ++x; default: break; which I have a patch for: https://reviews.llvm.org/D91895. I agree that can never lead to bugs. But that's not the sole case of this series, just most of them. Though, note how the reviewer (C++ spec editor and clang front end owner) in https://reviews.llvm.org/D91895 even asks in that review how maybe a new flag would be more appropriate for a watered down/stylistic variant of the existing behavior. And if the current wording of Documentation/process/deprecated.rst around "fallthrough" is a straightforward rule of thumb, I kind of agree with him. > > > This is about robustness and language wrangling. It's a big code- > > base, and this is the price of our managing technical debt for > > permanent robustness improvements. (The numbers I ran from Gustavo's > > earlier patches were that about 10% of the places adjusted were > > identified as legitimate bugs being fixed. This final series may be > > lower, but there are still bugs being found from it -- we need to > > finish this and shut the door on it for good.) > > I got my six patches by analyzing the lwn.net report of the fixes that > was cited which had 21 of which 50% didn't actually change the emitted > code, and 25% didn't have a user visible effect. > > But the broader point I'm making is just because the compiler people > come up with a shiny new warning doesn't necessarily mean the problem That's not what this is though; you're attacking a strawman. I'd encourage you to bring that up when that actually occurs, unlike this case since it's actively hindering getting -Wimplicit-fallthrough enabled for Clang. This is not a shiny new warning; it's already on for GCC and has existed in both compilers for multiple releases. And I'll also note that warnings are warnings and not errors because they cannot be proven to be bugs in 100% of cases, but they have led to bugs in the past. They require a human to review their intent and remove ambiguities. If 97% of cases would end in a break ("Expert C Programming: Deep C Secrets" - Peter van der Linden), then it starts to look to me like a language defect; certainly an incorrectly chosen default. But the compiler can't know those 3% were intentional, unless you're explicit for those exceptional cases. > it's detecting is one that causes us actual problems in the code base. > I'd really be happier if we had a theory about what classes of CVE or > bug we could eliminate before we embrace the next new warning. We don't generally file CVEs and waiting for them to occur might be too reactive, but I agree that pointing to some additional documentation in commit messages about how a warning could lead to a bug would make it clearer to reviewers why being able to enable it treewide, even if there's no bug in their particular subsystem, is in the general interest of the commons. On Mon, Nov 23, 2020 at 7:58 AM James Bottomley wrote: > > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 > > The whole crux of your argument seems to be maintainers' time isn't > important so we should accept all trivial patches ... I'm pushing back > on that assumption in two places, firstly the valulessness of the time > and secondly that all trivial patches are valuable. It's critical to the longevity of any open source project that there are not single points of failure. If someone is not expendable or replaceable (or claims to be) then that's a risk to the project and a bottleneck. Not having a replacement in training or some form of redundancy is short sighted. If trivial patches are adding too much to your workload, consider training a co-maintainer or asking for help from one of your reviewers whom you trust. I don't doubt it's hard to find maintainers, but existing maintainers should go out of their way to entrust co-maintainers especially when they find their workload becomes too high. And reviewing/picking up trivial patches is probably a great way to get started. If we allow too much knowledge of any one subsystem to collect with one maintainer, what happens when that maintainer leaves the community (which, given a finite lifespan, is an inevitability)? -- Thanks, ~Nick Desaulniers From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nick Desaulniers Date: Wed, 25 Nov 2020 04:24:27 -0800 Subject: [Cluster-devel] [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang In-Reply-To: References: <202011201129.B13FDB3C@keescook> <20201120115142.292999b2@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> <202011220816.8B6591A@keescook> <9b57fd4914b46f38d54087d75e072d6e947cb56d.camel@HansenPartnership.com> <0147972a72bc13f3629de8a32dee6f1f308994b5.camel@HansenPartnership.com> <20201123130348.GA3119@embeddedor> <8f5611bb015e044fa1c0a48147293923c2d904e4.camel@HansenPartnership.com> <202011241327.BB28F12F6@keescook> Message-ID: List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Tue, Nov 24, 2020 at 11:05 PM James Bottomley wrote: > > On Tue, 2020-11-24 at 13:32 -0800, Kees Cook wrote: > > We already enable -Wimplicit-fallthrough globally, so that's not the > > discussion. The issue is that Clang is (correctly) even more strict > > than GCC for this, so these are the remaining ones to fix for full > > Clang coverage too. > > > > People have spent more time debating this already than it would have > > taken to apply the patches. :) > > You mean we've already spent 90% of the effort to come this far so we > might as well go the remaining 10% because then at least we get some > return? It's certainly a clinching argument in defence procurement ... So developers and distributions using Clang can't have -Wimplicit-fallthrough enabled because GCC is less strict (which has been shown in this thread to lead to bugs)? We'd like to have nice things too, you know. I even agree that most of the churn comes from case 0: ++x; default: break; which I have a patch for: https://reviews.llvm.org/D91895. I agree that can never lead to bugs. But that's not the sole case of this series, just most of them. Though, note how the reviewer (C++ spec editor and clang front end owner) in https://reviews.llvm.org/D91895 even asks in that review how maybe a new flag would be more appropriate for a watered down/stylistic variant of the existing behavior. And if the current wording of Documentation/process/deprecated.rst around "fallthrough" is a straightforward rule of thumb, I kind of agree with him. > > > This is about robustness and language wrangling. It's a big code- > > base, and this is the price of our managing technical debt for > > permanent robustness improvements. (The numbers I ran from Gustavo's > > earlier patches were that about 10% of the places adjusted were > > identified as legitimate bugs being fixed. This final series may be > > lower, but there are still bugs being found from it -- we need to > > finish this and shut the door on it for good.) > > I got my six patches by analyzing the lwn.net report of the fixes that > was cited which had 21 of which 50% didn't actually change the emitted > code, and 25% didn't have a user visible effect. > > But the broader point I'm making is just because the compiler people > come up with a shiny new warning doesn't necessarily mean the problem That's not what this is though; you're attacking a strawman. I'd encourage you to bring that up when that actually occurs, unlike this case since it's actively hindering getting -Wimplicit-fallthrough enabled for Clang. This is not a shiny new warning; it's already on for GCC and has existed in both compilers for multiple releases. And I'll also note that warnings are warnings and not errors because they cannot be proven to be bugs in 100% of cases, but they have led to bugs in the past. They require a human to review their intent and remove ambiguities. If 97% of cases would end in a break ("Expert C Programming: Deep C Secrets" - Peter van der Linden), then it starts to look to me like a language defect; certainly an incorrectly chosen default. But the compiler can't know those 3% were intentional, unless you're explicit for those exceptional cases. > it's detecting is one that causes us actual problems in the code base. > I'd really be happier if we had a theory about what classes of CVE or > bug we could eliminate before we embrace the next new warning. We don't generally file CVEs and waiting for them to occur might be too reactive, but I agree that pointing to some additional documentation in commit messages about how a warning could lead to a bug would make it clearer to reviewers why being able to enable it treewide, even if there's no bug in their particular subsystem, is in the general interest of the commons. On Mon, Nov 23, 2020 at 7:58 AM James Bottomley wrote: > > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 > > The whole crux of your argument seems to be maintainers' time isn't > important so we should accept all trivial patches ... I'm pushing back > on that assumption in two places, firstly the valulessness of the time > and secondly that all trivial patches are valuable. It's critical to the longevity of any open source project that there are not single points of failure. If someone is not expendable or replaceable (or claims to be) then that's a risk to the project and a bottleneck. Not having a replacement in training or some form of redundancy is short sighted. If trivial patches are adding too much to your workload, consider training a co-maintainer or asking for help from one of your reviewers whom you trust. I don't doubt it's hard to find maintainers, but existing maintainers should go out of their way to entrust co-maintainers especially when they find their workload becomes too high. And reviewing/picking up trivial patches is probably a great way to get started. If we allow too much knowledge of any one subsystem to collect with one maintainer, what happens when that maintainer leaves the community (which, given a finite lifespan, is an inevitability)? -- Thanks, ~Nick Desaulniers From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9IH0l2L/ELs04A0W/6GC4nhC0e+RvRGWJ1bAzD1+dFc=; b=GyrkIL7rJc/Wrkz9wtYqXZYvGBry6qXFkQono0nmrBFDlUCiGmbX9ByD1wUhih87ZW XCd/8etF0h65aGuVNHVvGVnSoIRV2cIFxWeuMsMEKDZ+SIKsK6eM3KIHPaY2Au+pxfCB jFmSmO0a8jtSnIjbAi/709gkMW9hnqxggrhUNIGI/2GrlejcLn7tyz9MFlSEpE31y19Z 9ARaZhNBbaKZzII6ioDoEFmbbi01XI+4/fF65wWR3SGfZuCMoV2cgGUJ8Osa8sFeqdbz KLllcyBsC3gtRPDbq4Yc+z3inKaZT7D05cYTb7CBHNDdR/afLH0A7E5JHrBWilMAefn4 uS6g== MIME-Version: 1.0 References: <202011201129.B13FDB3C@keescook> <20201120115142.292999b2@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> <202011220816.8B6591A@keescook> <9b57fd4914b46f38d54087d75e072d6e947cb56d.camel@HansenPartnership.com> <0147972a72bc13f3629de8a32dee6f1f308994b5.camel@HansenPartnership.com> <20201123130348.GA3119@embeddedor> <8f5611bb015e044fa1c0a48147293923c2d904e4.camel@HansenPartnership.com> <202011241327.BB28F12F6@keescook> In-Reply-To: From: Nick Desaulniers Date: Wed, 25 Nov 2020 04:24:27 -0800 Message-ID: Content-Type: text/plain; charset="UTF-8" Subject: Re: [Bridge] [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang List-Id: Linux Ethernet Bridging List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: James Bottomley Cc: alsa-devel@alsa-project.org, bridge@lists.linux-foundation.org, target-devel@vger.kernel.org, Greg KH , linux-iio@vger.kernel.org, samba-technical@lists.samba.org, Jonathan Cameron , linux-fbdev@vger.kernel.org, dri-devel , "Gustavo A. R. Silva" , linux-ide@vger.kernel.org, dm-devel@redhat.com, keyrings@vger.kernel.org, linux-mtd@lists.infradead.org, GR-everest-linux-l2@marvell.com, wcn36xx@lists.infradead.org, linux-i3c@lists.infradead.org, linux1394-devel@lists.sourceforge.net, linux-afs@lists.infradead.org, drbd-dev@lists.linbit.com, devel@driverdev.osuosl.org, linux-cifs@vger.kernel.org, rds-devel@oss.oracle.com, linux-scsi@vger.kernel.org, linux-acpi@vger.kernel.org, linux-rdma@vger.kernel.org, oss-drivers@netronome.com, linux-atm-general@lists.sourceforge.net, ceph-devel@vger.kernel.org, amd-gfx list , linux-stm32@st-md-mailman.stormreply.com, cluster-devel@redhat.com, usb-storage@lists.one-eyed-alien.net, linux-mmc@vger.kernel.org, coreteam@netfilter.org, intel-wired-lan@lists.osuosl.org, linux-input@vger.kernel.org, Miguel Ojeda , Jakub Kicinski , linux-ext4@vger.kernel.org, virtualization@lists.linux-foundation.org, netfilter-devel@vger.kernel.org, linux-media@vger.kernel.org, Kees Cook , selinux@vger.kernel.org, linux-arm-msm , intel-gfx@lists.freedesktop.org, linux-sctp@vger.kernel.org, reiserfs-devel@vger.kernel.org, linux-geode@lists.infradead.org, linux-block@vger.kernel.org, linux-gpio@vger.kernel.org, op-tee@lists.trustedfirmware.org, linux-mediatek@lists.infradead.org, xen-devel@lists.xenproject.org, nouveau@lists.freedesktop.org, linux-hams@vger.kernel.org, Nathan Chancellor , linux-can@vger.kernel.org, Linux ARM , linux-hwmon@vger.kernel.org, "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" , linux-watchdog@vger.kernel.org, GR-Linux-NIC-Dev@marvell.com, Linux Memory Management List , Network Development , linux-decnet-user@lists.sourceforge.net, linux-wireless , LKML , Linux-Renesas , linux-security-module@vger.kernel.org, linux-usb@vger.kernel.org, tipc-discussion@lists.sourceforge.net, "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" , patches@opensource.cirrus.com, Joe Perches , linux-integrity@vger.kernel.org, linux-nfs@vger.kernel.org, linux-hardening@vger.kernel.org On Tue, Nov 24, 2020 at 11:05 PM James Bottomley wrote: > > On Tue, 2020-11-24 at 13:32 -0800, Kees Cook wrote: > > We already enable -Wimplicit-fallthrough globally, so that's not the > > discussion. The issue is that Clang is (correctly) even more strict > > than GCC for this, so these are the remaining ones to fix for full > > Clang coverage too. > > > > People have spent more time debating this already than it would have > > taken to apply the patches. :) > > You mean we've already spent 90% of the effort to come this far so we > might as well go the remaining 10% because then at least we get some > return? It's certainly a clinching argument in defence procurement ... So developers and distributions using Clang can't have -Wimplicit-fallthrough enabled because GCC is less strict (which has been shown in this thread to lead to bugs)? We'd like to have nice things too, you know. I even agree that most of the churn comes from case 0: ++x; default: break; which I have a patch for: https://reviews.llvm.org/D91895. I agree that can never lead to bugs. But that's not the sole case of this series, just most of them. Though, note how the reviewer (C++ spec editor and clang front end owner) in https://reviews.llvm.org/D91895 even asks in that review how maybe a new flag would be more appropriate for a watered down/stylistic variant of the existing behavior. And if the current wording of Documentation/process/deprecated.rst around "fallthrough" is a straightforward rule of thumb, I kind of agree with him. > > > This is about robustness and language wrangling. It's a big code- > > base, and this is the price of our managing technical debt for > > permanent robustness improvements. (The numbers I ran from Gustavo's > > earlier patches were that about 10% of the places adjusted were > > identified as legitimate bugs being fixed. This final series may be > > lower, but there are still bugs being found from it -- we need to > > finish this and shut the door on it for good.) > > I got my six patches by analyzing the lwn.net report of the fixes that > was cited which had 21 of which 50% didn't actually change the emitted > code, and 25% didn't have a user visible effect. > > But the broader point I'm making is just because the compiler people > come up with a shiny new warning doesn't necessarily mean the problem That's not what this is though; you're attacking a strawman. I'd encourage you to bring that up when that actually occurs, unlike this case since it's actively hindering getting -Wimplicit-fallthrough enabled for Clang. This is not a shiny new warning; it's already on for GCC and has existed in both compilers for multiple releases. And I'll also note that warnings are warnings and not errors because they cannot be proven to be bugs in 100% of cases, but they have led to bugs in the past. They require a human to review their intent and remove ambiguities. If 97% of cases would end in a break ("Expert C Programming: Deep C Secrets" - Peter van der Linden), then it starts to look to me like a language defect; certainly an incorrectly chosen default. But the compiler can't know those 3% were intentional, unless you're explicit for those exceptional cases. > it's detecting is one that causes us actual problems in the code base. > I'd really be happier if we had a theory about what classes of CVE or > bug we could eliminate before we embrace the next new warning. We don't generally file CVEs and waiting for them to occur might be too reactive, but I agree that pointing to some additional documentation in commit messages about how a warning could lead to a bug would make it clearer to reviewers why being able to enable it treewide, even if there's no bug in their particular subsystem, is in the general interest of the commons. On Mon, Nov 23, 2020 at 7:58 AM James Bottomley wrote: > > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 > > The whole crux of your argument seems to be maintainers' time isn't > important so we should accept all trivial patches ... I'm pushing back > on that assumption in two places, firstly the valulessness of the time > and secondly that all trivial patches are valuable. It's critical to the longevity of any open source project that there are not single points of failure. If someone is not expendable or replaceable (or claims to be) then that's a risk to the project and a bottleneck. Not having a replacement in training or some form of redundancy is short sighted. If trivial patches are adding too much to your workload, consider training a co-maintainer or asking for help from one of your reviewers whom you trust. I don't doubt it's hard to find maintainers, but existing maintainers should go out of their way to entrust co-maintainers especially when they find their workload becomes too high. And reviewing/picking up trivial patches is probably a great way to get started. If we allow too much knowledge of any one subsystem to collect with one maintainer, what happens when that maintainer leaves the community (which, given a finite lifespan, is an inevitability)? -- Thanks, ~Nick Desaulniers