Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set
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.
Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set
On Mon, Feb 1, 2021 at 2:48 PM Theodore Ts'o 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
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
On Mon, Feb 1, 2021 at 6:41 PM Nick Desaulniers wrote: > > On Mon, Feb 1, 2021 at 1:38 PM Theodore Ts'o 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
On Mon, Feb 1, 2021 at 1:38 PM Theodore Ts'o 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
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
On Mon, Feb 1, 2021 at 1:09 PM Theodore Ts'o 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
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
On Feb 1, 2021, at 11:41 AM, Vinicius Tinti wrote: > > On Mon, Feb 1, 2021 at 2:13 PM Theodore Ts'o 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 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 signature.asc Description: Message signed with OpenPGP
Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set
On Mon, Feb 1, 2021 at 2:13 PM Theodore Ts'o 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 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
On Mon, Feb 01, 2021 at 01:15:29PM -0300, Vinicius Tinti wrote: > On Mon, Feb 1, 2021 at 9:49 AM 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. > > 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
On Mon, Feb 01, 2021 at 12:49:24PM +, 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
Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set
On Mon, Feb 1, 2021 at 9:49 AM 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. 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
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
On Mon, Feb 01, 2021 at 12:31:25AM +, 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 > --- > 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