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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKwvOdkzc3AtpkRcZU06yvAEzp_bjw55HkpGui6RsAcy=FhnJw@mail.gmail.com>
Date:   Mon, 9 Mar 2020 12:36:47 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Joe Perches <joe@...ches.com>, 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 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?

--
Thanks,
~Nick Desaulniers