linux-kernel - Re: [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */ comments to fallthrough;

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 09 Mar 2020 12:55:14 -0700
From:   Joe Perches <joe@...ches.com>
To:     Nick Desaulniers <ndesaulniers@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        clang-built-linux <clang-built-linux@...glegroups.com>
Subject: Re: [PATCH] cvt_fallthrough: A tool to convert /* fallthrough */
 comments to fallthrough;

On Mon, 2020-03-09 at 12:36 -0700, Nick Desaulniers wrote:
> On Thu, Feb 20, 2020 at 4:21 PM Andrew Morton <akpm@...ux-foundation.org> wrote:
> > On Thu, 20 Feb 2020 12:30:21 -0800 Joe Perches <joe@...ches.com> wrote:
> > 
> > > Convert /* fallthrough */ style comments to the pseudo-keyword fallthrough
> > > to allow clang 10 and higher to work at finding missing fallthroughs too.
> > > 
> > > Requires a git repository and overwrites the input files.
> > > 
> > > Typical command use:
> > >     ./scripts/cvt_fallthrough.pl <path|file>
> > > 
> > > i.e.:
> > > 
> > >    $ ./scripts/cvt_fallthrough.pl block
> > >      converts all files in block and its subdirectories
> > >    $ ./scripts/cvt_fallthrough.pl drivers/net/wireless/zydas/zd1201.c
> > >      converts a single file
> > > 
> > > A fallthrough comment with additional comments is converted akin to:
> > > 
> > > -             /* fall through - maybe userspace knows this conn_id. */
> > > +             fallthrough;    /* maybe userspace knows this conn_id */
> > > 
> > > A fallthrough comment or fallthrough; between successive case statements
> > > is deleted.
> > > 
> > > e.g.:
> > > 
> > >     case FOO:
> > >       /* fallthrough */ (or fallthrough;)
> > >     case BAR:
> > > 
> > > is converted to:
> > > 
> > >     case FOO:
> > >     case BAR:
> > > 
> > > Signed-off-by: Joe Perches <joe@...ches.com>
> > > ---
> > >  scripts/cvt_fallthrough.pl | 215 +++++++++++++++++++++++++++++++++++++
> > 
> > Do we need this in the tree long-term?  Or is it a matters of "hey
> > Linus, please run this" then something like add a checkpatch rule to
> > catch future slipups?
> 
> Just for some added context, please see
> https://reviews.llvm.org/D73852, where support for parsing some forms
> of fallthrough statements was added to Clang in a broken state by a
> contributor, but then ripped out by the code owner (of the clang front
> end to LLVM, and also happens to be the C++ ISO spec editor).  He
> provides further clarification
> https://bugs.llvm.org/show_bug.cgi?id=43465#c37.
> 
> I'm inclined to agree with him; to implement this, we need to keep
> around comments for semantic analyses, a later phase of compilation
> than preprocessing.  It feels like a layering violation to either not
> discard comments as soon as possible, or emit diagnostics from the
> preprocessor.  And as Joe's data shows, there's the classic issue
> faced when using regexes to solve a problem; suddenly you now have two
> problems.
> https://xkcd.com/1171/
> 
> I would like to see this patch landed, though I am curious as toward's
> Andrew's question ('Or is it a matters of "hey Linus, please run
> this"') of what's the imagined workflow here, since it seems like the
> script needs to be run per file. I suppose you could still do that
> treewide, but is that the intention, or is it to do so on a per
> subsystem level?

A single treewide run of a script like this really could make
it quite hard to later backport various fixes to stable trees.

A depth-first per-maintained subsystem run of the script with
commits could be useful and would much more easily allow backports.

Unfortunately there's no tool to apply such a script to the tree
per subsystem as far as I know.

Such a depth-first apply and commit tool could really be quite
useful though.