Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <AAB32610-D238-4137-96DE-33655AAAB545@dilger.ca>
 <20210201003125.90257-1-viniciustinti@gmail.com> <20210201124924.GA3284018@infradead.org>
 <CALD9WKxc0kMPCHSoikko+qYk2+ZLUy73+ryKGW9qMSpyzAobLA@mail.gmail.com>
 <YBg20AuSC3/9w2zz@mit.edu> <CALD9WKzO53AXQW-qQ82VZ41H5=cGdLTUiEoz3X6BmPkb6XaTag@mail.gmail.com>
 <YBhuHJgZ3QPqHheV@mit.edu> <CAKwvOd=ny2TeYV8SGZMD+aj8Yb6OSYGKAzSb-45r-HKk6WTUCQ@mail.gmail.com>
 <YBh0ywVzkUIR3fXg@mit.edu> <CAKwvOdkZRdBzzW19sVAs+pX-7wWwN6AWrxUkkZwP8L4OT7SLfQ@mail.gmail.com>
In-Reply-To: <CAKwvOdkZRdBzzW19sVAs+pX-7wWwN6AWrxUkkZwP8L4OT7SLfQ@mail.gmail.com>
From:   Vinicius Tinti <viniciustinti@gmail.com>
Date:   Mon, 1 Feb 2021 19:05:11 -0300
Message-ID: <CALD9WKx6HREQeTRXuv81v-=DTVuznXG_56YFm2dM1GOG3s4BRQ@mail.gmail.com>
Subject: Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set
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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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