* [PATCH] ext4: Remove unreachable code @ 2021-01-29 18:58 Vinicius Tinti 2021-01-30 1:35 ` Nathan Chancellor 2021-01-30 6:42 ` Andreas Dilger 0 siblings, 2 replies; 22+ messages in thread From: Vinicius Tinti @ 2021-01-29 18:58 UTC (permalink / raw) To: Theodore Ts'o, Andreas Dilger Cc: Nathan Chancellor, Nick Desaulniers, Vinicius Tinti, linux-ext4, linux-kernel, clang-built-linux 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] ext4: Remove unreachable code 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 1 sibling, 0 replies; 22+ messages in thread From: Nathan Chancellor @ 2021-01-30 1:35 UTC (permalink / raw) To: Vinicius Tinti Cc: Theodore Ts'o, Andreas Dilger, Nathan Chancellor, Nick Desaulniers, linux-ext4, linux-kernel, clang-built-linux 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 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ext4: Remove unreachable code 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 1 sibling, 1 reply; 22+ messages in thread From: Andreas Dilger @ 2021-01-30 6:42 UTC (permalink / raw) To: Vinicius Tinti Cc: Theodore Ts'o, Andreas Dilger, Nathan Chancellor, Nick Desaulniers, linux-ext4, linux-kernel, clang-built-linux [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2] ext4: Enable code path when DX_DEBUG is set 2021-01-30 6:42 ` Andreas Dilger @ 2021-02-01 0:31 ` Vinicius Tinti 2021-02-01 0:48 ` Nathan Chancellor 2021-02-01 12:49 ` Christoph Hellwig 0 siblings, 2 replies; 22+ messages in thread From: Vinicius Tinti @ 2021-02-01 0:31 UTC (permalink / raw) To: Andreas Dilger, Nathan Chancellor Cc: Theodore Ts'o, Nick Desaulniers, Vinicius Tinti, linux-ext4, linux-kernel, clang-built-linux 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set 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 1 sibling, 0 replies; 22+ messages in thread From: Nathan Chancellor @ 2021-02-01 0:48 UTC (permalink / raw) To: Vinicius Tinti Cc: Andreas Dilger, Nathan Chancellor, Theodore Ts'o, Nick Desaulniers, linux-ext4, linux-kernel, clang-built-linux 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set 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 16:58 ` [PATCH v2] " Theodore Ts'o 1 sibling, 2 replies; 22+ messages in thread From: Christoph Hellwig @ 2021-02-01 12:49 UTC (permalink / raw) To: Vinicius Tinti Cc: Andreas Dilger, Nathan Chancellor, Theodore Ts'o, Nick Desaulniers, linux-ext4, linux-kernel, clang-built-linux DX_DEBUG is completely dead code, so either kill it off or make it an actual CONFIG_* symbol through Kconfig if it seems useful. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set 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 16:58 ` [PATCH v2] " Theodore Ts'o 1 sibling, 1 reply; 22+ messages in thread From: Vinicius Tinti @ 2021-02-01 16:15 UTC (permalink / raw) To: Christoph Hellwig Cc: Andreas Dilger, Nathan Chancellor, Theodore Ts'o, Nick Desaulniers, linux-ext4, linux-kernel, clang-built-linux 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? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set 2021-02-01 16:15 ` Vinicius Tinti @ 2021-02-01 17:13 ` Theodore Ts'o 2021-02-01 18:41 ` Vinicius Tinti 2021-02-02 8:05 ` Christoph Hellwig 0 siblings, 2 replies; 22+ messages in thread From: Theodore Ts'o @ 2021-02-01 17:13 UTC (permalink / raw) To: Vinicius Tinti Cc: Christoph Hellwig, Andreas Dilger, Nathan Chancellor, Nick Desaulniers, linux-ext4, linux-kernel, clang-built-linux 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set 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-02 8:05 ` Christoph Hellwig 1 sibling, 2 replies; 22+ messages in thread From: Vinicius Tinti @ 2021-02-01 18:41 UTC (permalink / raw) To: Theodore Ts'o Cc: Christoph Hellwig, Andreas Dilger, Nathan Chancellor, Nick Desaulniers, linux-ext4, linux-kernel, clang-built-linux 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set 2021-02-01 18:41 ` Vinicius Tinti @ 2021-02-01 20:45 ` Andreas Dilger 2021-02-01 21:09 ` Theodore Ts'o 1 sibling, 0 replies; 22+ messages in thread From: Andreas Dilger @ 2021-02-01 20:45 UTC (permalink / raw) To: Vinicius Tinti Cc: Theodore Ts'o, Christoph Hellwig, Nathan Chancellor, Nick Desaulniers, Ext4 Developers List, Linux Kernel Mailing List, clang-built-linux [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set 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 1 sibling, 1 reply; 22+ messages in thread From: Theodore Ts'o @ 2021-02-01 21:09 UTC (permalink / raw) To: Vinicius Tinti Cc: Christoph Hellwig, Andreas Dilger, Nathan Chancellor, Nick Desaulniers, linux-ext4, linux-kernel, clang-built-linux 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set 2021-02-01 21:09 ` Theodore Ts'o @ 2021-02-01 21:16 ` Nick Desaulniers 2021-02-01 21:38 ` Theodore Ts'o 0 siblings, 1 reply; 22+ messages in thread From: Nick Desaulniers @ 2021-02-01 21:16 UTC (permalink / raw) To: Vinicius Tinti Cc: Christoph Hellwig, Andreas Dilger, Nathan Chancellor, Ext4 Developers List, LKML, clang-built-linux, Theodore Ts'o 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set 2021-02-01 21:16 ` Nick Desaulniers @ 2021-02-01 21:38 ` Theodore Ts'o 2021-02-01 21:41 ` Nick Desaulniers 0 siblings, 1 reply; 22+ messages in thread From: Theodore Ts'o @ 2021-02-01 21:38 UTC (permalink / raw) To: Nick Desaulniers Cc: Vinicius Tinti, Christoph Hellwig, Andreas Dilger, Nathan Chancellor, Ext4 Developers List, LKML, clang-built-linux 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set 2021-02-01 21:38 ` Theodore Ts'o @ 2021-02-01 21:41 ` Nick Desaulniers 2021-02-01 22:05 ` Vinicius Tinti 0 siblings, 1 reply; 22+ messages in thread From: Nick Desaulniers @ 2021-02-01 21:41 UTC (permalink / raw) To: Theodore Ts'o Cc: Vinicius Tinti, Christoph Hellwig, Andreas Dilger, Nathan Chancellor, Ext4 Developers List, LKML, clang-built-linux 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set 2021-02-01 21:41 ` Nick Desaulniers @ 2021-02-01 22:05 ` Vinicius Tinti 2021-02-01 22:48 ` Theodore Ts'o 0 siblings, 1 reply; 22+ messages in thread From: Vinicius Tinti @ 2021-02-01 22:05 UTC (permalink / raw) To: Nick Desaulniers Cc: Theodore Ts'o, Christoph Hellwig, Andreas Dilger, Nathan Chancellor, Ext4 Developers List, LKML, clang-built-linux 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set 2021-02-01 22:05 ` Vinicius Tinti @ 2021-02-01 22:48 ` Theodore Ts'o 2021-02-01 23:09 ` Nick Desaulniers 0 siblings, 1 reply; 22+ messages in thread From: Theodore Ts'o @ 2021-02-01 22:48 UTC (permalink / raw) To: Vinicius Tinti Cc: Nick Desaulniers, Christoph Hellwig, Andreas Dilger, Nathan Chancellor, Ext4 Developers List, LKML, clang-built-linux 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. :-) ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set 2021-02-01 22:48 ` Theodore Ts'o @ 2021-02-01 23:09 ` Nick Desaulniers 0 siblings, 0 replies; 22+ messages in thread From: Nick Desaulniers @ 2021-02-01 23:09 UTC (permalink / raw) To: Theodore Ts'o Cc: Vinicius Tinti, Christoph Hellwig, Andreas Dilger, Nathan Chancellor, Ext4 Developers List, LKML, clang-built-linux 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set 2021-02-01 17:13 ` Theodore Ts'o 2021-02-01 18:41 ` Vinicius Tinti @ 2021-02-02 8:05 ` Christoph Hellwig 2021-02-02 16:28 ` [PATCH v3] " Vinicius Tinti 1 sibling, 1 reply; 22+ messages in thread From: Christoph Hellwig @ 2021-02-02 8:05 UTC (permalink / raw) To: Theodore Ts'o Cc: Vinicius Tinti, Christoph Hellwig, Andreas Dilger, Nathan Chancellor, Nick Desaulniers, linux-ext4, linux-kernel, clang-built-linux 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3] ext4: Enable code path when DX_DEBUG is set 2021-02-02 8:05 ` Christoph Hellwig @ 2021-02-02 16:28 ` Vinicius Tinti 2021-02-03 5:39 ` Theodore Ts'o 0 siblings, 1 reply; 22+ messages in thread From: Vinicius Tinti @ 2021-02-02 16:28 UTC (permalink / raw) To: Theodore Ts'o, Christoph Hellwig, Andreas Dilger, Nathan Chancellor, Nick Desaulniers Cc: Vinicius Tinti, linux-ext4, linux-kernel, clang-built-linux 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3] ext4: Enable code path when DX_DEBUG is set 2021-02-02 16:28 ` [PATCH v3] " Vinicius Tinti @ 2021-02-03 5:39 ` Theodore Ts'o 2021-02-03 9:51 ` Vinicius Tinti 0 siblings, 1 reply; 22+ messages in thread From: Theodore Ts'o @ 2021-02-03 5:39 UTC (permalink / raw) To: Vinicius Tinti Cc: Christoph Hellwig, Andreas Dilger, Nathan Chancellor, Nick Desaulniers, linux-ext4, linux-kernel, clang-built-linux 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3] ext4: Enable code path when DX_DEBUG is set 2021-02-03 5:39 ` Theodore Ts'o @ 2021-02-03 9:51 ` Vinicius Tinti 0 siblings, 0 replies; 22+ messages in thread From: Vinicius Tinti @ 2021-02-03 9:51 UTC (permalink / raw) To: Theodore Ts'o Cc: Christoph Hellwig, Andreas Dilger, Nathan Chancellor, Nick Desaulniers, Ext4 Developers List, LKML, clang-built-linux 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set 2021-02-01 12:49 ` Christoph Hellwig 2021-02-01 16:15 ` Vinicius Tinti @ 2021-02-01 16:58 ` Theodore Ts'o 1 sibling, 0 replies; 22+ messages in thread From: Theodore Ts'o @ 2021-02-01 16:58 UTC (permalink / raw) To: Christoph Hellwig Cc: Vinicius Tinti, Andreas Dilger, Nathan Chancellor, Nick Desaulniers, linux-ext4, linux-kernel, clang-built-linux 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2021-02-03 9:52 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
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).