linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: "Gustavo A. R. Silva" <garsilva@embeddedor.com>,
	Alan Cox <gnomes@lxorguk.ukuu.org.uk>,
	Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
	X86 ML <x86@kernel.org>, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs
Date: Wed, 29 Nov 2017 16:21:20 -0800	[thread overview]
Message-ID: <CAGXu5j+S3brOaWYK1P2Y7=RgZDjr3G1ymHhZ=iSg+FxEJduZeQ@mail.gmail.com> (raw)
In-Reply-To: <alpine.DEB.2.20.1711291612580.1825@nanos>

On Wed, Nov 29, 2017 at 7:14 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 29 Nov 2017, Gustavo A. R. Silva wrote:
>> Quoting Thomas Gleixner <tglx@linutronix.de>:
>>
>> >
>> > So I have to ask WHY this information was not in the changelog of the patch
>> > in question:
>> >
>> >   1) How it works
>> >
>> >   2) Why comments have been chosen over macros
>> >
>>
>> I will add this info and send the patch again.
>>
>> > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>> > > where we are expecting to fall through.
>> >
>> > It's not a reviewers job to chase that information down.
>> >
>> > While I can understand that the comments are intentional due to existing
>> > tools, I still prefer the macro/annotation. But I'm not religious about it
>> > when there is common consensus. :)
>
>                   ^^^^^^^^^^^^^^^^
>
> This is the important point. And there are people aside of me who prefer the
> macro annotation.

Understood. I, too, prefer the macro annotation, but when considering
where the primary benefit comes from, I had to admit that using the
comments was tolerable: the many external tools (e.g. Coverity,
Eclipse, gcc, clang, etc) that already process the comments means we
gain their coverage without any additional work to teach them about
yet another way to mark fall-through. In other words, making a marking
that only works for humans and gcc leaves out the other tools. To me,
"it's ugly" isn't sufficient to limit the wider benefit.

And I do recognize that when we get all fixes landed and we add
-Wimplicit-fallthrough, then we can just ignore the tools that don't
understand the new marking and announce false positives: but it's not
worth everyone else's time to deal with those false positives. There
is already a solution that all "missing break" tools handle, so let's
use it, all false positives vanish, and everyone wins (with a small
"ugliness" cost).

-Kees

-- 
Kees Cook
Pixel Security

  reply	other threads:[~2017-11-30  0:21 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-27 23:52 Gustavo A. R. Silva
2017-11-28 13:49 ` Thomas Gleixner
2017-11-28 18:05   ` Gustavo A. R. Silva
2017-11-28 18:10     ` Thomas Gleixner
2017-11-28 18:17       ` Thomas Gleixner
2017-11-28 18:22         ` Gustavo A. R. Silva
2017-11-28 18:27           ` Thomas Gleixner
2017-11-28 18:35             ` Thomas Gleixner
2017-11-28 18:45               ` Thomas Gleixner
2017-11-28 18:53                 ` Gustavo A. R. Silva
2017-11-28 19:48                   ` Thomas Gleixner
2017-11-28 19:00               ` Alan Cox
2017-11-28 19:10                 ` Linus Torvalds
2017-11-28 19:59                   ` Joe Perches
2017-11-28 20:08                   ` Thomas Gleixner
2017-11-28 20:34                     ` Kees Cook
2017-11-28 20:37                   ` Gustavo A. R. Silva
2017-11-29  1:07                     ` Joe Perches
2017-11-29  8:20                       ` Geert Uytterhoeven
2017-11-28 20:11                 ` Thomas Gleixner
2017-11-28 20:25                   ` Gustavo A. R. Silva
2017-11-28 21:25                     ` Thomas Gleixner
2017-11-29 15:10                       ` Gustavo A. R. Silva
2017-11-29 15:14                         ` Thomas Gleixner
2017-11-30  0:21                           ` Kees Cook [this message]
2019-01-29 23:56 Gustavo A. R. Silva
2019-01-30  0:14 ` Thomas Gleixner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAGXu5j+S3brOaWYK1P2Y7=RgZDjr3G1ymHhZ=iSg+FxEJduZeQ@mail.gmail.com' \
    --to=keescook@chromium.org \
    --cc=garsilva@embeddedor.com \
    --cc=gnomes@lxorguk.ukuu.org.uk \
    --cc=hpa@zytor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH] x86/syscalls: Mark expected switch fall-throughs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).