[PATCH] ext4: Remove unreachable code

[PATCH] ext4: Remove unreachable code

[Vinicius Tinti]

By enabling -Wunreachable-code-aggressive on Clang the following code
paths are unreachable.

Commit dd73b5d5cb67 ("ext4: convert dx_probe() to use the ERR_PTR
convention")
Commit ac27a0ec112a ("[PATCH] ext4: initial copy of files from ext3")

Clang warns:

fs/ext4/namei.c:831:17: warning: code will never be executed
[-Wunreachable-code]
                        unsigned n = count - 1;
                                     ^~~~~
fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as
explicitly dead
                if (0) { // linear search cross check
                    ^
                    /* DISABLES CODE */ ( )

Signed-off-by: Vinicius Tinti <viniciustinti@gmail.com>
---
 fs/ext4/namei.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index cf652ba3e74d..1f64dbd7237b 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -827,21 +827,6 @@ dx_probe(struct ext4_filename *fname, struct inode *dir,
 				p = m + 1;
 		}
 
-		if (0) { // linear search cross check
-			unsigned n = count - 1;
-			at = entries;
-			while (n--)
-			{
-				dxtrace(printk(KERN_CONT ","));
-				if (dx_get_hash(++at) > hash)
-				{
-					at--;
-					break;
-				}
-			}
-			ASSERT(at == p - 1);
-		}
-
 		at = p - 1;
 		dxtrace(printk(KERN_CONT " %x->%u\n",
 			       at == entries ? 0 : dx_get_hash(at),
-- 
2.25.1

Re: [PATCH] ext4: Remove unreachable code

[Nathan Chancellor]

On Fri, Jan 29, 2021 at 06:58:56PM +0000, Vinicius Tinti wrote:
> By enabling -Wunreachable-code-aggressive on Clang the following code
> paths are unreachable.
> 
> Commit dd73b5d5cb67 ("ext4: convert dx_probe() to use the ERR_PTR
> convention")
> Commit ac27a0ec112a ("[PATCH] ext4: initial copy of files from ext3")
> 
> Clang warns:
> 
> fs/ext4/namei.c:831:17: warning: code will never be executed
> [-Wunreachable-code]
>                         unsigned n = count - 1;
>                                      ^~~~~
> fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as
> explicitly dead
>                 if (0) { // linear search cross check
>                     ^
>                     /* DISABLES CODE */ ( )

The commit message might be a little clearer if it were restructured a
bit, maybe something like so?

By enabling -Wunreachable-code-aggressive on Clang, the following code
paths are unreachable:

fs/ext4/namei.c:831:17: warning: code will never be executed
[-Wunreachable-code]
                        unsigned n = count - 1;
                                     ^~~~~
fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as
explicitly dead
                if (0) { // linear search cross check
                    ^
                    /* DISABLES CODE */ ( )

This has been present since commit ac27a0ec112a ("[PATCH] ext4: initial
copy of files from ext3") and fs/ext3 had it present at the beginning of
git history so it is safe to remove.

> Signed-off-by: Vinicius Tinti <viniciustinti@gmail.com>

Regardless of the commit message, I believe this is the right way to get
rid of the warning:

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  fs/ext4/namei.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index cf652ba3e74d..1f64dbd7237b 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -827,21 +827,6 @@ dx_probe(struct ext4_filename *fname, struct inode *dir,
>  				p = m + 1;
>  		}
>  
> -		if (0) { // linear search cross check
> -			unsigned n = count - 1;
> -			at = entries;
> -			while (n--)
> -			{
> -				dxtrace(printk(KERN_CONT ","));
> -				if (dx_get_hash(++at) > hash)
> -				{
> -					at--;
> -					break;
> -				}
> -			}
> -			ASSERT(at == p - 1);
> -		}
> -
>  		at = p - 1;
>  		dxtrace(printk(KERN_CONT " %x->%u\n",
>  			       at == entries ? 0 : dx_get_hash(at),
> -- 
> 2.25.1
> 

Re: [PATCH] ext4: Remove unreachable code

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1786 bytes --]

On Jan 29, 2021, at 11:58 AM, Vinicius Tinti <viniciustinti@gmail.com> wrote:
> 
> By enabling -Wunreachable-code-aggressive on Clang the following code
> paths are unreachable.
> 
> Commit dd73b5d5cb67 ("ext4: convert dx_probe() to use the ERR_PTR
> convention")
> Commit ac27a0ec112a ("[PATCH] ext4: initial copy of files from ext3")
> 
> Clang warns:
> 
> fs/ext4/namei.c:831:17: warning: code will never be executed
> [-Wunreachable-code]
>                        unsigned n = count - 1;
>                                     ^~~~~
> fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as
> explicitly dead
>                if (0) { // linear search cross check
>                    ^
>                    /* DISABLES CODE */ ( )
> 
> Signed-off-by: Vinicius Tinti <viniciustinti@gmail.com>
> ---
> fs/ext4/namei.c | 15 ---------------
> 1 file changed, 15 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index cf652ba3e74d..1f64dbd7237b 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -827,21 +827,6 @@ dx_probe(struct ext4_filename *fname, struct inode *dir,
> 				p = m + 1;
> 		}
> 
> -		if (0) { // linear search cross check

I would rather put this block under "#ifdef DX_DEBUG" so that it is available
in the future for debugging problems with hashed directories.

> -			unsigned n = count - 1;
> -			at = entries;
> -			while (n--)
> -			{
> -				dxtrace(printk(KERN_CONT ","));
> -				if (dx_get_hash(++at) > hash)
> -				{
> -					at--;
> -					break;
> -				}
> -			}
> -			ASSERT(at == p - 1);
> -		}
> -
> 		at = p - 1;
> 		dxtrace(printk(KERN_CONT " %x->%u\n",
> 			       at == entries ? 0 : dx_get_hash(at),
> --
> 2.25.1
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

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

[Vinicius Tinti]

By enabling -Wunreachable-code-aggressive on Clang the following code
paths are unreachable.

This has been present since commit ac27a0ec112a ("[PATCH] ext4: initial
copy of files from ext3") and fs/ext3 had it present at the beginning of
git history. It has not been changed since.

Clang warns:

fs/ext4/namei.c:831:17: warning: code will never be executed
[-Wunreachable-code]
                        unsigned n = count - 1;
                                     ^~~~~
fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as
explicitly dead
                if (0) { // linear search cross check
                    ^
                    /* DISABLES CODE */ ( )

Signed-off-by: Vinicius Tinti <viniciustinti@gmail.com>
---
 fs/ext4/namei.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index cf652ba3e74d..46ae6a4e4be5 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -827,20 +827,21 @@ dx_probe(struct ext4_filename *fname, struct inode *dir,
 				p = m + 1;
 		}
 
-		if (0) { // linear search cross check
-			unsigned n = count - 1;
-			at = entries;
-			while (n--)
+#ifdef DX_DEBUG
+		// linear search cross check
+		unsigned n = count - 1;
+		at = entries;
+		while (n--)
+		{
+			dxtrace(printk(KERN_CONT ","));
+			if (dx_get_hash(++at) > hash)
 			{
-				dxtrace(printk(KERN_CONT ","));
-				if (dx_get_hash(++at) > hash)
-				{
-					at--;
-					break;
-				}
+				at--;
+				break;
 			}
-			ASSERT(at == p - 1);
 		}
+		ASSERT(at == p - 1);
+#endif
 
 		at = p - 1;
 		dxtrace(printk(KERN_CONT " %x->%u\n",
-- 
2.25.1

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

[Nathan Chancellor]

On Mon, Feb 01, 2021 at 12:31:25AM +0000, Vinicius Tinti wrote:
> By enabling -Wunreachable-code-aggressive on Clang the following code
> paths are unreachable.
> 
> This has been present since commit ac27a0ec112a ("[PATCH] ext4: initial
> copy of files from ext3") and fs/ext3 had it present at the beginning of
> git history. It has not been changed since.
> 
> Clang warns:
> 
> fs/ext4/namei.c:831:17: warning: code will never be executed
> [-Wunreachable-code]
>                         unsigned n = count - 1;
>                                      ^~~~~
> fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as
> explicitly dead
>                 if (0) { // linear search cross check
>                     ^
>                     /* DISABLES CODE */ ( )
> 
> Signed-off-by: Vinicius Tinti <viniciustinti@gmail.com>
> ---
>  fs/ext4/namei.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index cf652ba3e74d..46ae6a4e4be5 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -827,20 +827,21 @@ dx_probe(struct ext4_filename *fname, struct inode *dir,
>  				p = m + 1;
>  		}
>  
> -		if (0) { // linear search cross check
> -			unsigned n = count - 1;
> -			at = entries;
> -			while (n--)
> +#ifdef DX_DEBUG
> +		// linear search cross check
> +		unsigned n = count - 1;

You are going to introduce an instance of -Wdeclaration-after-statement
here.

fs/ext4/namei.c:834:12: warning: ISO C90 forbids mixing declarations and
code [-Wdeclaration-after-statement]
                unsigned n = count - 1;
                         ^
1 warning generated.

The quick hack would be changing the

if (0) {

to

#ifdef DX_DEBUG
    if (1) {

but that seems kind of ugly.

Other options would be turning DX_DEBUG into a proper Kconfig symbol so
that IS_ENABLED could be used or maybe combine it into
CONFIG_EXT4_DEBUG.

Cheers,
Nathan

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

[Christoph Hellwig]

DX_DEBUG is completely dead code, so either kill it off or make it an
actual CONFIG_* symbol through Kconfig if it seems useful.

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

[Vinicius Tinti]

On Mon, Feb 1, 2021 at 9:49 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> DX_DEBUG is completely dead code, so either kill it off or make it an
> actual CONFIG_* symbol through Kconfig if it seems useful.

About the unreachable code in "if (0)" I think it could be removed.
It seems to be doing an extra check.

About the DX_DEBUG I think I can do another patch adding it to Kconfig
as you and Nathan suggested.

What do you think?

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

[Theodore Ts'o]

On Mon, Feb 01, 2021 at 01:15:29PM -0300, Vinicius Tinti wrote:
> On Mon, Feb 1, 2021 at 9:49 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > DX_DEBUG is completely dead code, so either kill it off or make it an
> > actual CONFIG_* symbol through Kconfig if it seems useful.
> 
> About the unreachable code in "if (0)" I think it could be removed.
> It seems to be doing an extra check.
> 
> About the DX_DEBUG I think I can do another patch adding it to Kconfig
> as you and Nathan suggested.

Yes, it's doing another check which is useful in terms of early
detection of bugs when a developer has the code open for
modifications.  It slows down performance under normal circumstances,
and assuming the code is bug-free(tm), it's entirely unnecessary ---
which is why it's under an "if (0)".

However, if there *is* a bug, having an early detection that the
representation invariant of the data structure has been violated can
be useful in root causing a bug.  This would probably be clearer if
the code was pulled out into a separate function with comments
explaining that this is a rep invariant check.

The main thing about DX_DEBUG right now is that it is **super**
verbose.  Unwary users who enable it.... will be sorry.  If we want to
make it to be a first-class feature enabled via CONFIG_EXT4_DEBUG, we
should convert all of the dx_trace calls to use pr_debug so they are
enabled only if dynamic debug enables those pr_debug() statements.
And this should absolutely be a separate patch.

Cheers,

						- Ted

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

[Vinicius Tinti]

On Mon, Feb 1, 2021 at 2:13 PM Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Mon, Feb 01, 2021 at 01:15:29PM -0300, Vinicius Tinti wrote:
> > On Mon, Feb 1, 2021 at 9:49 AM Christoph Hellwig <hch@infradead.org> wrote:
> > >
> > > DX_DEBUG is completely dead code, so either kill it off or make it an
> > > actual CONFIG_* symbol through Kconfig if it seems useful.
> >
> > About the unreachable code in "if (0)" I think it could be removed.
> > It seems to be doing an extra check.
> >
> > About the DX_DEBUG I think I can do another patch adding it to Kconfig
> > as you and Nathan suggested.
>
> Yes, it's doing another check which is useful in terms of early
> detection of bugs when a developer has the code open for
> modifications.  It slows down performance under normal circumstances,
> and assuming the code is bug-free(tm), it's entirely unnecessary ---
> which is why it's under an "if (0)".

My goal is to avoid having a dead code. Three options come to mind.

The first would be to add another #ifdef SOMETHING (suggest a name).
But this doesn't remove the code and someone could enable it by accident.

The second would be to add the code in a block comment. Then document
that it is for checking an invariant. This will make it harder to cause
problems.

The third would be to remove the code and explain the invariant in a block
comment. Maybe add a pseudo code too in the comment.

What do you think?

> However, if there *is* a bug, having an early detection that the
> representation invariant of the data structure has been violated can
> be useful in root causing a bug.  This would probably be clearer if
> the code was pulled out into a separate function with comments
> explaining that this is a rep invariant check.

Good idea. I will do that too.

> The main thing about DX_DEBUG right now is that it is **super**
> verbose.  Unwary users who enable it.... will be sorry.  If we want to
> make it to be a first-class feature enabled via CONFIG_EXT4_DEBUG, we
> should convert all of the dx_trace calls to use pr_debug so they are
> enabled only if dynamic debug enables those pr_debug() statements.
> And this should absolutely be a separate patch.

Agreed. For now I only want to focus on the "if (0)".

Regards,
Vinicius

> Cheers,
>
>                                                 - Ted

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

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 3442 bytes --]

On Feb 1, 2021, at 11:41 AM, Vinicius Tinti <viniciustinti@gmail.com> wrote:
> 
> On Mon, Feb 1, 2021 at 2:13 PM Theodore Ts'o <tytso@mit.edu> wrote:
>> 
>> On Mon, Feb 01, 2021 at 01:15:29PM -0300, Vinicius Tinti wrote:
>>> On Mon, Feb 1, 2021 at 9:49 AM Christoph Hellwig <hch@infradead.org> wrote:
>>>> 
>>>> DX_DEBUG is completely dead code, so either kill it off or make it an
>>>> actual CONFIG_* symbol through Kconfig if it seems useful.
>>> 
>>> About the unreachable code in "if (0)" I think it could be removed.
>>> It seems to be doing an extra check.
>>> 
>>> About the DX_DEBUG I think I can do another patch adding it to Kconfig
>>> as you and Nathan suggested.
>> 
>> Yes, it's doing another check which is useful in terms of early
>> detection of bugs when a developer has the code open for
>> modifications.  It slows down performance under normal circumstances,
>> and assuming the code is bug-free(tm), it's entirely unnecessary ---
>> which is why it's under an "if (0)".
> 
> My goal is to avoid having a dead code. Three options come to mind.
> 
> The first would be to add another #ifdef SOMETHING (suggest a name).
> But this doesn't remove the code and someone could enable it by accident.

I don't see anything wrong with the original suggestion to use "DX_DEBUG".
If we want to get rid of "if (0)" in the code it could be changed like:

#define DX_DEBUG 0
#if DX_DEBUG
#define dxtrace(command) command
#else
#define dxtrace(command)
#endif

Then in the code check this directly (and fix the //-style comment also):

	if (DX_DEBUG) { /* linear search cross check */
		:
	}

That will hopefully avoid the "dead code" warning, while keeping the code
visible for syntax checking by the compiler (unlike if it was under #ifdef).

Alternately, if we want to keep the "#ifdef DX_DEBUG" check intact:

#ifdef DX_DEBUG
#define dxtrace(command) command
#define DX_DEBUG_CROSSCHECK true
#else
#define dxtrace(command)
#define DX_DEBUG_CROSSCHECK false
#endif

	if (DX_DEBUG_CROSSCHECK) { /* linear search cross check */
		:
	}

Cheers, Andreas

> 
> The second would be to add the code in a block comment. Then document
> that it is for checking an invariant. This will make it harder to cause
> problems.
> 
> The third would be to remove the code and explain the invariant in a block
> comment. Maybe add a pseudo code too in the comment.
> 
> What do you think?
> 
>> However, if there *is* a bug, having an early detection that the
>> representation invariant of the data structure has been violated can
>> be useful in root causing a bug.  This would probably be clearer if
>> the code was pulled out into a separate function with comments
>> explaining that this is a rep invariant check.
> 
> Good idea. I will do that too.
> 
>> The main thing about DX_DEBUG right now is that it is **super**
>> verbose.  Unwary users who enable it.... will be sorry.  If we want to
>> make it to be a first-class feature enabled via CONFIG_EXT4_DEBUG, we
>> should convert all of the dx_trace calls to use pr_debug so they are
>> enabled only if dynamic debug enables those pr_debug() statements.
>> And this should absolutely be a separate patch.
> 
> Agreed. For now I only want to focus on the "if (0)".
> 
> Regards,
> Vinicius
> 
>> Cheers,
>> 
>>                                                - Ted


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

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

[Theodore Ts'o]

On Mon, Feb 01, 2021 at 03:41:50PM -0300, Vinicius Tinti wrote:
> 
> My goal is to avoid having a dead code. Three options come to mind.
> 
> The first would be to add another #ifdef SOMETHING (suggest a name).
> But this doesn't remove the code and someone could enable it by accident.

I *really* don't see the point of having the compiler whine about
"dead code", so I'm not terribly fond of
-Wunreachable-code-aggressive.  There may be times where depending on
how things are compiled, we *want* the compiler to remove code block,
and it makes the code less ugly than having #ifdef ... #endif breaking
up the code.

If turning that on requires uglifying many places in the kernel code,
maybe the right answer is... don't.

That being said, I have no problem of replacing

	if (0) {
		...
	}

with

#ifdef DX_DEBUG
	...
#endif

In this particular place.

But before we go there, I want to register my extreme skepticsm about
-Wunreachable-code-aggressive.  How much other places where it is
***obvious*** that the maintainer really knew what they are doing, and
it's just the compiler whining about a false positive?

> > However, if there *is* a bug, having an early detection that the
> > representation invariant of the data structure has been violated can
> > be useful in root causing a bug.  This would probably be clearer if
> > the code was pulled out into a separate function with comments
> > explaining that this is a rep invariant check.
> 
> Good idea. I will do that too.

If you want to do that, and do something like

#ifdef DX_DEBUG
static inline htree_rep_invariant_Check(...)
{
	...
}
#else
static inline htree_rep_invariant_check(...) { }
#endif

I'm not going to complain.  That's actually a better way to go, since
there may be other places in the code where a developer might want to
introduce a rep invariant check.  So that's actually improving the
code, as opposed to making a pointless change just to suppress a
compiler warning.

Of course, then someone will try enabling a -W flag which causes the
compiler to whine about empty function bodies....

					- Ted

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

[Nick Desaulniers]

On Mon, Feb 1, 2021 at 1:09 PM Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Mon, Feb 01, 2021 at 03:41:50PM -0300, Vinicius Tinti wrote:
> >
> > My goal is to avoid having a dead code. Three options come to mind.
> >
> > The first would be to add another #ifdef SOMETHING (suggest a name).
> > But this doesn't remove the code and someone could enable it by accident.
>
> I *really* don't see the point of having the compiler whine about
> "dead code", so I'm not terribly fond of
> -Wunreachable-code-aggressive.

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).
-- 
Thanks,
~Nick Desaulniers

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

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

[Nick Desaulniers]

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

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

> 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.
-- 
Thanks,
~Nick Desaulniers

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

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

[Theodore Ts'o]

On Mon, Feb 01, 2021 at 07:05:11PM -0300, Vinicius Tinti wrote:
> 
> 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.

The trick was that it was unused code, but it was pretty obviously
deliberate, which should have implied that at some point, it was
considered useful.   :-)

It was the fact that you were so determined to find a way to suppress
the warning, suggesting multiple tactics, which made me wonder.... why
were you going through so much effort to silence the warning if the
goal was *not* to turn it on unconditionally everywhere?

I suspect the much more useful thing to consider is how can we suggest
hueristics to the Clang folks to make the warning more helpful.  For
example, Coverity will warn about the following:

void test_func(unsigned int arg)
{
	if (arg < 0) {
		printf("Hello, world\n")
	}
}

This is an example of dead code that is pretty clearly unintended ---
and it's something that "clang -Wall" or "gcc -Wall" doesn't pick up
on, but Coverity does.

So in cases where the code is explicitly doing "if (0)" or "if
(IS_ENABLED(xxx))" where IS_ENABLED resolves down to zero due to
preprocessor magic, arguably, that's not a useful compiler warning
because it almost *certainly* is intentional.  But in the case of an
unsigned int being compared to see if it is less than, or greater than
or equal to 0, that's almost certainly a bug --- and yes, Coverity has
found a real bug (tm) in my code due to that kind of static code
analysis.  So it would actually be quite nice if there was a compiler
warning (either gcc or clang, I don't really care which) which would
reliably call that out without having the maintainer submit the code
to Coverity for analysis.

Cheers,

							- Ted

P.S.  If anyone wants to file a feature request bug with the Clang
developers, feel free.  :-)

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

[Nick Desaulniers]

On Mon, Feb 1, 2021 at 2:48 PM Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Mon, Feb 01, 2021 at 07:05:11PM -0300, Vinicius Tinti wrote:
> >
> > 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.
>
> The trick was that it was unused code, but it was pretty obviously
> deliberate, which should have implied that at some point, it was
> considered useful.   :-)
>
> It was the fact that you were so determined to find a way to suppress
> the warning, suggesting multiple tactics, which made me wonder.... why
> were you going through so much effort to silence the warning if the
> goal was *not* to turn it on unconditionally everywhere?

Because a maintainer might say "oh, I meant to turn that back on years
ago" or "that should not have been committed!"  Hasn't happened yet,
doesn't mean it's impossible.  Vinicius asked how he can help. I said
"go see if any instances of this warning are that case."

>
> I suspect the much more useful thing to consider is how can we suggest
> hueristics to the Clang folks to make the warning more helpful.  For
> example, Coverity will warn about the following:
>
> void test_func(unsigned int arg)
> {
>         if (arg < 0) {
>                 printf("Hello, world\n")
>         }
> }

Put that code in in godbolt.org (https://godbolt.org/z/E7KK9T) and
you'll see that both compilers already warn here on -Wextra (via
-Wtautological-unsigned-zero-compare in clang or -Wtype-limits in
GCC).
clang:

warning: result of comparison of unsigned expression < 0 is always
false [-Wtautological-unsigned-zero-compare]
        if (arg < 0) {
            ~~~ ^ ~

gcc:

warning: comparison of unsigned expression in '< 0' is always false
[-Wtype-limits]
    3 |         if (arg < 0) {
      |                 ^


>
> P.S.  If anyone wants to file a feature request bug with the Clang
> developers, feel free.  :-)



-- 
Thanks,
~Nick Desaulniers

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

[Christoph Hellwig]

On Mon, Feb 01, 2021 at 12:13:52PM -0500, Theodore Ts'o wrote:
> However, if there *is* a bug, having an early detection that the
> representation invariant of the data structure has been violated can
> be useful in root causing a bug.  This would probably be clearer if
> the code was pulled out into a separate function with comments
> explaining that this is a rep invariant check.
> 
> The main thing about DX_DEBUG right now is that it is **super**
> verbose.  Unwary users who enable it.... will be sorry.  If we want to
> make it to be a first-class feature enabled via CONFIG_EXT4_DEBUG, we
> should convert all of the dx_trace calls to use pr_debug so they are
> enabled only if dynamic debug enables those pr_debug() statements.
> And this should absolutely be a separate patch.

Yes.  The problem with a non-Kconfig ifdef is that is is almost
guaranteed to bitrot very fast, so you might as well just remove the
code.  The "if (0)", while ugly, at least ensures the code still
actually is seen and checked by the compiler.

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

[Vinicius Tinti]

Clang with -Wunreachable-code-aggressive is being used to try to find
unreachable code that could cause potential bugs. There is no plan to
enable it by default.

The following code was detected as unreachable:

fs/ext4/namei.c:831:17: warning: code will never be executed
[-Wunreachable-code]
                        unsigned n = count - 1;
                                     ^~~~~
fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as
explicitly dead
                if (0) { // linear search cross check
                    ^
                    /* DISABLES CODE */ ( )

This has been present since commit ac27a0ec112a ("[PATCH] ext4: initial
copy of files from ext3") and fs/ext3 had it present at the beginning of
git history. It has not been changed since.

This patch moves the code to a new function `htree_rep_invariant_check`
which only performs the check when DX_DEBUG is set. This allows the
function to be used in other parts of the code.

Suggestions from: Andreas, Christoph, Nathan, Nick and Ted.

Signed-off-by: Vinicius Tinti <viniciustinti@gmail.com>
---
 fs/ext4/namei.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index cf652ba3e74d..a6e28b4b5a95 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -731,6 +731,29 @@ struct stats dx_show_entries(struct dx_hash_info *hinfo, struct inode *dir,
 		       (space/bcount)*100/blocksize);
 	return (struct stats) { names, space, bcount};
 }
+
+/*
+ * Linear search cross check
+ */
+static inline void htree_rep_invariant_check(struct dx_entry *at,
+					     struct dx_entry *target,
+					     u32 hash, unsigned int n)
+{
+	while (n--) {
+		dxtrace(printk(KERN_CONT ","));
+		if (dx_get_hash(++at) > hash) {
+			at--;
+			break;
+		}
+	}
+	ASSERT(at == target - 1);
+}
+#else /* DX_DEBUG */
+static inline void htree_rep_invariant_check(struct dx_entry *at,
+					     struct dx_entry *target,
+					     u32 hash, unsigned int n)
+{
+}
 #endif /* DX_DEBUG */
 
 /*
@@ -827,20 +850,7 @@ dx_probe(struct ext4_filename *fname, struct inode *dir,
 				p = m + 1;
 		}
 
-		if (0) { // linear search cross check
-			unsigned n = count - 1;
-			at = entries;
-			while (n--)
-			{
-				dxtrace(printk(KERN_CONT ","));
-				if (dx_get_hash(++at) > hash)
-				{
-					at--;
-					break;
-				}
-			}
-			ASSERT(at == p - 1);
-		}
+		htree_rep_invariant_check(entries, p, hash, count - 1);
 
 		at = p - 1;
 		dxtrace(printk(KERN_CONT " %x->%u\n",
-- 
2.25.1

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

[Theodore Ts'o]

On Tue, Feb 02, 2021 at 04:28:37PM +0000, Vinicius Tinti wrote:
> Clang with -Wunreachable-code-aggressive is being used to try to find
> unreachable code that could cause potential bugs. There is no plan to
> enable it by default.
> 
> The following code was detected as unreachable:
> 
> fs/ext4/namei.c:831:17: warning: code will never be executed
> [-Wunreachable-code]
>                         unsigned n = count - 1;
>                                      ^~~~~
> fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as
> explicitly dead
>                 if (0) { // linear search cross check
>                     ^
>                     /* DISABLES CODE */ ( )
> 
> This has been present since commit ac27a0ec112a ("[PATCH] ext4: initial
> copy of files from ext3") and fs/ext3 had it present at the beginning of
> git history. It has not been changed since.
> 
> This patch moves the code to a new function `htree_rep_invariant_check`
> which only performs the check when DX_DEBUG is set. This allows the
> function to be used in other parts of the code.
> 
> Suggestions from: Andreas, Christoph, Nathan, Nick and Ted.
> 
> Signed-off-by: Vinicius Tinti <viniciustinti@gmail.com>

Thanks, applied, although I rewrote the commit description:

    ext4: factor out htree rep invariant check
    
    This patch moves some debugging code which is used to validate the
    hash tree node when doing a binary search of an htree node into a
    separate function, which is disabled by default (since it is only used
    by developers when they are modifying the htree code paths).
    
    In addition to cleaning up the code to make it more maintainable, it
    silences a Clang compiler warning when -Wunreachable-code-aggressive
    is enabled.  (There is no plan to enable this warning by default, since
    there it has far too many false positives; nevertheless, this commit
    reduces one of the many false positives by one.)

The original description buried the lede, in terms of the primary
reason why I think the change was worthwhile (although I know you have
different priorities than mine :-).

Thanks for working to find a way to improve the code in a way that
makes both of us happy!

					- Ted

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

[Vinicius Tinti]

On Wed, Feb 3, 2021 at 2:39 AM Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Tue, Feb 02, 2021 at 04:28:37PM +0000, Vinicius Tinti wrote:
> > Clang with -Wunreachable-code-aggressive is being used to try to find
> > unreachable code that could cause potential bugs. There is no plan to
> > enable it by default.
> >
> > The following code was detected as unreachable:
> >
> > fs/ext4/namei.c:831:17: warning: code will never be executed
> > [-Wunreachable-code]
> >                         unsigned n = count - 1;
> >                                      ^~~~~
> > fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as
> > explicitly dead
> >                 if (0) { // linear search cross check
> >                     ^
> >                     /* DISABLES CODE */ ( )
> >
> > This has been present since commit ac27a0ec112a ("[PATCH] ext4: initial
> > copy of files from ext3") and fs/ext3 had it present at the beginning of
> > git history. It has not been changed since.
> >
> > This patch moves the code to a new function `htree_rep_invariant_check`
> > which only performs the check when DX_DEBUG is set. This allows the
> > function to be used in other parts of the code.
> >
> > Suggestions from: Andreas, Christoph, Nathan, Nick and Ted.
> >
> > Signed-off-by: Vinicius Tinti <viniciustinti@gmail.com>
>
> Thanks, applied, although I rewrote the commit description:
>
>     ext4: factor out htree rep invariant check
>
>     This patch moves some debugging code which is used to validate the
>     hash tree node when doing a binary search of an htree node into a
>     separate function, which is disabled by default (since it is only used
>     by developers when they are modifying the htree code paths).
>
>     In addition to cleaning up the code to make it more maintainable, it
>     silences a Clang compiler warning when -Wunreachable-code-aggressive
>     is enabled.  (There is no plan to enable this warning by default, since
>     there it has far too many false positives; nevertheless, this commit
>     reduces one of the many false positives by one.)
>
> The original description buried the lede, in terms of the primary
> reason why I think the change was worthwhile (although I know you have
> different priorities than mine :-).
>
> Thanks for working to find a way to improve the code in a way that
> makes both of us happy!

Thanks for the feedback.

And thanks for all the ones who reviewed.

>                                         - Ted

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

[Theodore Ts'o]

On Mon, Feb 01, 2021 at 12:49:24PM +0000, Christoph Hellwig wrote:
> DX_DEBUG is completely dead code, so either kill it off or make it an
> actual CONFIG_* symbol through Kconfig if it seems useful.

I wouldn't call it completely dead code.  If you manually add "#define
DX_DEBUG" fs/ext4/namei.c compiles without any problems.  I believe it
was most recently used when we added large htree support.

It's true that it can only be enabled via manually enabled via manual
editing of the .c file, but it's *really* something that only
developers who are actively involved in modifying the code would want
to use.  Sure, we could add work to add debug levels to all of the
dxtrace() statements, and/or switch it all to dyndebug.  We'd also
have to figure out how to get rid of all of the KERN_CONT printk's in
the ideal world.  The question is whether doing all of this is
worth it if the goal is to shut up some clang warnings.

	      	    	     	   - Ted