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

[Theodore Ts'o]

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
> 
> 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.  

Hence, this is not a warning that we will *ever* be able to enable
unconditionally --- 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
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.

Cheers,


						- Ted