From: Vinicius Tinti <viniciustinti@gmail.com>
To: Nick Desaulniers <ndesaulniers@google.com>
Cc: "Theodore Ts'o" <tytso@mit.edu>,
Christoph Hellwig <hch@infradead.org>,
Andreas Dilger <adilger.kernel@dilger.ca>,
Nathan Chancellor <natechancellor@gmail.com>,
Ext4 Developers List <linux-ext4@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
clang-built-linux <clang-built-linux@googlegroups.com>
Subject: Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set
Date: Mon, 1 Feb 2021 19:05:11 -0300 [thread overview]
Message-ID: <CALD9WKx6HREQeTRXuv81v-=DTVuznXG_56YFm2dM1GOG3s4BRQ@mail.gmail.com> (raw)
In-Reply-To: <CAKwvOdkZRdBzzW19sVAs+pX-7wWwN6AWrxUkkZwP8L4OT7SLfQ@mail.gmail.com>
On Mon, Feb 1, 2021 at 6:41 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Mon, Feb 1, 2021 at 1:38 PM Theodore Ts'o <tytso@mit.edu> wrote:
> >
> > On Mon, Feb 01, 2021 at 01:16:19PM -0800, Nick Desaulniers wrote:
> > > I agree; Vinicius, my recommendation for -Wunreachable-* with Clang
> > > was to see whether dead code identified by this more aggressive
> > > diagnostic (than -Wunused-function) was to ask maintainers whether
> > > code identified by it was intentionally dead and if they would
> > > consider removing it. If they say "no," that's fine, and doesn't need
> > > to be pushed. It's not clear to maintainers that:
> > > 1. this warning is not on by default
> > > 2. we're not looking to pursue turning this on by default
Ok. I will make it clear in next commit messages.
> > >
> > > If maintainers want to keep the dead code, that's fine, let them and
> > > move on to the next instance to see if that's interesting (or not).
> >
> > It should be noted that in Documenting/process/coding-style.rst, there
> > is an expicit recommendation to code in a way that will result in dead
> > code warnings:
> >
> > Within code, where possible, use the IS_ENABLED macro to convert a Kconfig
> > symbol into a C boolean expression, and use it in a normal C conditional:
> >
> > .. code-block:: c
> >
> > if (IS_ENABLED(CONFIG_SOMETHING)) {
> > ...
> > }
> >
> > The compiler will constant-fold the conditional away, and include or exclude
> > the block of code just as with an #ifdef, so this will not add any runtime
> > overhead. However, this approach still allows the C compiler to see the code
> > inside the block, and check it for correctness (syntax, types, symbol
> > references, etc). Thus, you still have to use an #ifdef if the code inside the
> > block references symbols that will not exist if the condition is not met.
> >
> > So our process documentation *explicitly* recommends against using
> > #ifdef CONFIG_XXX ... #endif, and instead use something that will
> > -Wunreachable-code-aggressive to cause the compiler to complain.
>
> I agree.
I agree too.
> >
> > Hence, this is not a warning that we will *ever* be able to enable
> > unconditionally ---
>
> I agree.
>
> > so why work hard to remove such warnings from the
> > code? If the goal is to see if we can detect real bugs using this
>
> Because not every instance of -Wunreachable-code-aggressive may be that pattern.
The goal is to try to detect real bugs. In this instance specifically I
suggested to remove the "if (0) {...}" because it sounded like an
unused code.
If it is useful it is fine to keep.
For now I am only looking for dead code that cannot be enabled
by a configuration file or architecture. In fact, there are several
warnings that I am ignoring because they are a dead code in my
build but may not be in another.
> > technique, well and good. If the data shows that this warning
> > actually is useful in finding bugs, then manybe we can figure out a
> > way that we can explicitly hint to the compiler that in *this* case,
> > the maintainer actually knew what they were doing.
> >
> > But if an examination of the warnings shows that
> > -Wunreachable-code-aggressive isn't actually finding any real bugs,
> > then perhaps it's not worth it.
>
> I agree. Hence the examination of instances found by Vinicius.
I liked the idea to create htree_rep_invariant_check. I will be doing
that.
Thanks for the help and suggestions.
> --
> Thanks,
> ~Nick Desaulniers
next prev parent reply other threads:[~2021-02-01 22:08 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-29 18:58 [PATCH] ext4: Remove unreachable code Vinicius Tinti
2021-01-30 1:35 ` Nathan Chancellor
2021-01-30 6:42 ` Andreas Dilger
2021-02-01 0:31 ` [PATCH v2] ext4: Enable code path when DX_DEBUG is set Vinicius Tinti
2021-02-01 0:48 ` Nathan Chancellor
2021-02-01 12:49 ` Christoph Hellwig
2021-02-01 16:15 ` Vinicius Tinti
2021-02-01 17:13 ` Theodore Ts'o
2021-02-01 18:41 ` Vinicius Tinti
2021-02-01 20:45 ` Andreas Dilger
2021-02-01 21:09 ` Theodore Ts'o
2021-02-01 21:16 ` Nick Desaulniers
2021-02-01 21:38 ` Theodore Ts'o
2021-02-01 21:41 ` Nick Desaulniers
2021-02-01 22:05 ` Vinicius Tinti [this message]
2021-02-01 22:48 ` Theodore Ts'o
2021-02-01 23:09 ` Nick Desaulniers
2021-02-02 8:05 ` Christoph Hellwig
2021-02-02 16:28 ` [PATCH v3] " Vinicius Tinti
2021-02-03 5:39 ` Theodore Ts'o
2021-02-03 9:51 ` Vinicius Tinti
2021-02-01 16:58 ` [PATCH v2] " Theodore Ts'o
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='CALD9WKx6HREQeTRXuv81v-=DTVuznXG_56YFm2dM1GOG3s4BRQ@mail.gmail.com' \
--to=viniciustinti@gmail.com \
--cc=adilger.kernel@dilger.ca \
--cc=clang-built-linux@googlegroups.com \
--cc=hch@infradead.org \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=natechancellor@gmail.com \
--cc=ndesaulniers@google.com \
--cc=tytso@mit.edu \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).