Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set

[Vinicius Tinti]

On Mon, Feb 1, 2021 at 6:41 PM Nick Desaulniers <ndesaulniers@xxxxxxxxxx> wrote:
>
> On Mon, Feb 1, 2021 at 1:38 PM Theodore Ts'o <tytso@xxxxxxx> 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