kgdb: fix gcc-11 warning on indentation
Subsystems (2)
PATCH 1/1 kgdb: fix gcc-11 warning on indentation
From: Arnd Bergmann <[email protected]>
gcc-11 starts warning about misleading indentation inside of macros:
drivers/misc/kgdbts.c: In function ‘kgdbts_break_test’:drivers/misc/kgdbts.c:103:9: error: this ‘if’ clause does not guard... [-Werror=misleading-indentation]
103 | if (verbose > 1) \
| ^~
drivers/misc/kgdbts.c:200:9: note: in expansion of macro ‘v2printk’
200 | v2printk("kgdbts: breakpoint complete\n");
| ^~~~~~~~
drivers/misc/kgdbts.c:105:17: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’
105 | touch_nmi_watchdog(); \
| ^~~~~~~~~~~~~~~~~~The code looks correct to me, so just reindent it for readability.
--- drivers/misc/kgdbts.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
drivers/misc/kgdbts.c +13 -13
@@ -95,19 +95,19 @@#include <asm/sections.h> -#define v1printk(a...) do { \ - if (verbose) \ - printk(KERN_INFO a); \ - } while (0) -#define v2printk(a...) do { \ - if (verbose > 1) \ - printk(KERN_INFO a); \ - touch_nmi_watchdog(); \ - } while (0) -#define eprintk(a...) do { \ - printk(KERN_ERR a); \ - WARN_ON(1); \ - } while (0) +#define v1printk(a...) do { \ + if (verbose) \ + printk(KERN_INFO a); \ +} while (0) +#define v2printk(a...) do { \ + if (verbose > 1) \ + printk(KERN_INFO a); \ + touch_nmi_watchdog(); \ +} while (0) +#define eprintk(a...) do { \ + printk(KERN_ERR a); \ + WARN_ON(1); \ +} while (0)#define MAX_CONFIG_LEN 40static struct kgdb_io kgdbts_io_ops;
Signature · 1 line
2.29.2
Quoted 15 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
gcc-11 starts warning about misleading indentation inside of macros:
drivers/misc/kgdbts.c: In function ‘kgdbts_break_test’:drivers/misc/kgdbts.c:103:9: error: this ‘if’ clause does not guard... [-Werror=misleading-indentation]103 | if (verbose > 1) \ | ^~ drivers/misc/kgdbts.c:200:9: note: in expansion of macro ‘v2printk’ 200 | v2printk("kgdbts: breakpoint complete\n"); | ^~~~~~~~ drivers/misc/kgdbts.c:105:17: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’ 105 | touch_nmi_watchdog(); \ | ^~~~~~~~~~~~~~~~~~
Quoted 4 lines
The code looks correct to me, so just reindent it for readability.
Fixes e8d31c204e36 ("kgdb: add kgdb internal test suite")Signed-off-by Arnd Bergmann <[email protected]>
Which tree do you want to merge this one though? I've got nothing else pending for this file so I am very relaxed about the route...
Daniel.
Quoted 44 lines
--- drivers/misc/kgdbts.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)drivers/misc/kgdbts.c +13
-13@@ -95,19 +95,19 @@#include <asm/sections.h> -#define v1printk(a...) do { \ - if (verbose) \ - printk(KERN_INFO a); \ - } while (0) -#define v2printk(a...) do { \ - if (verbose > 1) \ - printk(KERN_INFO a); \ - touch_nmi_watchdog(); \ - } while (0) -#define eprintk(a...) do { \ - printk(KERN_ERR a); \ - WARN_ON(1); \ - } while (0) +#define v1printk(a...) do { \ + if (verbose) \ + printk(KERN_INFO a); \ +} while (0) +#define v2printk(a...) do { \ + if (verbose > 1) \ + printk(KERN_INFO a); \ + touch_nmi_watchdog(); \ +} while (0) +#define eprintk(a...) do { \ + printk(KERN_ERR a); \ + WARN_ON(1); \ +} while (0)#define MAX_CONFIG_LEN 40static struct kgdb_io kgdbts_io_ops;Signature · 1 line
2.29.2
Quoted 6 lines · Daniel Thompson
Acked-by Daniel Thompson <[email protected]>Which tree do you want to merge this one though? I've got nothing else pending for this file so I am very relaxed about the route...
I don't plan to merge any of the build fixes through my own tree. If you don't have anything else, maybe Greg can pick it up in the char-misc tree.
Arnd
Greg Kroah-Hartman
maintainerQuoted 4 lines · Arnd Bergmann
Quoted 2 lines · Daniel Thompson
Quoted 8 lines
Quoted 4 lines
Acked-by Daniel Thompson <[email protected]>Which tree do you want to merge this one though? I've got nothing else pending for this file so I am very relaxed about the route...
I don't plan to merge any of the build fixes through my own tree. If you don't have anything else, maybe Greg can pick it up in the char-misc tree.
Yes, I can take it, thanks.
greg k-h
Doug Anderson
reviewerHi,
Quoted 5 lines · Arnd Bergmann
drivers/misc/kgdbts.c +0
-4-#define v1printk(a...) do { \ - if (verbose) \ - printk(KERN_INFO a); \ - } while (0)
drivers/misc/kgdbts.c line 98
Quoted 10 lines
drivers/misc/kgdbts.c +1
-9-#define v2printk(a...) do { \ - if (verbose > 1) \ - printk(KERN_INFO a); \ - touch_nmi_watchdog(); \ - } while (0) -#define eprintk(a...) do { \ - printk(KERN_ERR a); \ - WARN_ON(1); \ - } while (0) +#define v1printk(a...) do { \
nit: In addition to the indentation change you're also lining up the backslashes. Is that just personal preference, or is there some official recommendation in the kernel? I don't really have a strong opinion either way (IMO each style has its advantages).
drivers/misc/kgdbts.c line 105
Quoted 7 lines
drivers/misc/kgdbts.c +7
-0+if (verbose) \ + printk(KERN_INFO a); \ +} while (0) +#define v2printk(a...) do { \ + if (verbose > 1) \ + printk(KERN_INFO a); \ + touch_nmi_watchdog(); \
This touch_nmi_watchdog() is pretty wonky. I guess maybe the assumption is that the "verbose level 2" prints are so chatty that the printing might prevent us from touching the NMI watchdog in the way that we normally do and thus we need an extra one here?
...but, in that case, I think the old code was _wrong_ and that the intention was that the touch_nmi_watchdog() should only be if "verose
Quoted 1 line
1" as the indentation implied. There doesn't feel like a reason to
touch the watchdog if we're not doing anything slow.
-Doug
Quoted 13 lines · Doug Anderson
Quoted 12 lines · Arnd Bergmann
drivers/misc/kgdbts.c +0
-11-#define v1printk(a...) do { \ - if (verbose) \ - printk(KERN_INFO a); \ - } while (0) -#define v2printk(a...) do { \ - if (verbose > 1) \ - printk(KERN_INFO a); \ - touch_nmi_watchdog(); \ - } while (0) -#define eprintk(a...) do { \ - printk(KERN_ERR a); \
Quoted 8 lines
drivers/misc/kgdbts.c line 98
Quoted 3 lines
drivers/misc/kgdbts.c +1
-2-WARN_ON (1 ); \ - }while (0 ) +#define v1printk(a...) do { \nit: In addition to the indentation change you're also lining up the backslashes. Is that just personal preference, or is there some official recommendation in the kernel? I don't really have a strong opinion either way (IMO each style has its advantages).
I don't think there is an official recommendation, I just think the style is more common, and it helped my figure out what the indentation should look like in this case.
Quoted 14 lines
drivers/misc/kgdbts.c line 105
Quoted 8 lines
drivers/misc/kgdbts.c +7
-0+if (verbose) \ + printk(KERN_INFO a); \ +} while (0) +#define v2printk(a...) do { \ + if (verbose > 1) \ + printk(KERN_INFO a); \ + touch_nmi_watchdog(); \This
touch_nmi_watchdog()is pretty wonky. I guess maybe the assumption is that the "verbose level 2" prints are so chatty that the printing might prevent us from touching the NMI watchdog in the way that we normally do and thus we need an extra one here?
Quoted 4 lines
...but, in that case, I think the old code was _wrong_ and that the intention was that the
touch_nmi_watchdog()should only be if "veroseQuoted 1 line
1" as the indentation implied. There doesn't feel like a reason to
touch the watchdog if we're not doing anything slow.
No idea. It was like this in Jason's original version from 2008.
Arnd
Doug Anderson
reviewerHi,
Quoted 24 lines · Arnd Bergmann
Quoted 13 lines · Doug Anderson
Quoted 12 lines · Arnd Bergmann
drivers/misc/kgdbts.c +0
-11-#define v1printk(a...) do { \ - if (verbose) \ - printk(KERN_INFO a); \ - } while (0) -#define v2printk(a...) do { \ - if (verbose > 1) \ - printk(KERN_INFO a); \ - touch_nmi_watchdog(); \ - } while (0) -#define eprintk(a...) do { \ - printk(KERN_ERR a); \Quoted 8 lines
drivers/misc/kgdbts.c line 98
Quoted 3 lines
drivers/misc/kgdbts.c +1
-2-WARN_ON (1 ); \ - }while (0 ) +#define v1printk(a...) do { \nit: In addition to the indentation change you're also lining up the backslashes. Is that just personal preference, or is there some official recommendation in the kernel? I don't really have a strong opinion either way (IMO each style has its advantages).
Quoted 3 lines
I don't think there is an official recommendation, I just think the style is more common, and it helped my figure out what the indentation should look like in this case.
OK, makes sense. I just wasn't sure if there was some standard that I wasn't aware of. Given that you have to touch all these lines anyway then making them all pretty like this seems fine to me.
Quoted 13 lines
Quoted 13 lines
drivers/misc/kgdbts.c line 105
Quoted 7 lines
drivers/misc/kgdbts.c +7
-0+if (verbose) \ + printk(KERN_INFO a); \ +} while (0) +#define v2printk(a...) do { \ + if (verbose > 1) \ + printk(KERN_INFO a); \ + touch_nmi_watchdog(); \This
touch_nmi_watchdog()is pretty wonky. I guess maybe the assumption is that the "verbose level 2" prints are so chatty that the printing might prevent us from touching the NMI watchdog in the way that we normally do and thus we need an extra one here?
Quoted 6 lines
Quoted 4 lines
...but, in that case, I think the old code was _wrong_ and that the intention was that the
touch_nmi_watchdog()should only be if "veroseQuoted 1 line
1" as the indentation implied. There doesn't feel like a reason to
touch the watchdog if we're not doing anything slow.
No idea. It was like this in Jason's original version from 2008.
Yeah, I noticed the same. I'd be curious what Daneil (or Jason if he's reading) says. I suppose i could always wait until your patch lands and then send a new patch that puts it inside the "if" statement and we can debate it then.
-Doug
Jason Wessel
maintainerQuoted 52 lines · Doug Anderson
Hi,
Quoted 24 lines · Arnd Bergmann
Quoted 13 lines · Doug Anderson
Quoted 12 lines · Arnd Bergmann
drivers/misc/kgdbts.c +0
-11-#define v1printk(a...) do { \ - if (verbose) \ - printk(KERN_INFO a); \ - } while (0) -#define v2printk(a...) do { \ - if (verbose > 1) \ - printk(KERN_INFO a); \ - touch_nmi_watchdog(); \ - } while (0) -#define eprintk(a...) do { \ - printk(KERN_ERR a); \Quoted 8 lines
drivers/misc/kgdbts.c line 98
Quoted 3 lines
drivers/misc/kgdbts.c +1
-2-WARN_ON (1 ); \ - }while (0 ) +#define v1printk(a...) do { \nit: In addition to the indentation change you're also lining up the backslashes. Is that just personal preference, or is there some official recommendation in the kernel? I don't really have a strong opinion either way (IMO each style has its advantages).
Quoted 3 lines
I don't think there is an official recommendation, I just think the style is more common, and it helped my figure out what the indentation should look like in this case.
OK, makes sense. I just wasn't sure if there was some standard that I wasn't aware of. Given that you have to touch all these lines anyway then making them all pretty like this seems fine to me.
Quoted 16 lines
Quoted 16 lines
drivers/misc/kgdbts.c line 105
Quoted 7 lines
drivers/misc/kgdbts.c +7
-0+if (verbose) \ + printk(KERN_INFO a); \ +} while (0) +#define v2printk(a...) do { \ + if (verbose > 1) \ + printk(KERN_INFO a); \ + touch_nmi_watchdog(); \This
touch_nmi_watchdog()is pretty wonky. I guess maybe the assumption is that the "verbose level 2" prints are so chatty that the printing might prevent us from touching the NMI watchdog in the way that we normally do and thus we need an extra one here?...but, in that case, I think the old code was _wrong_ and that the intention was that the
touch_nmi_watchdog()should only be if "veroseQuoted 1 line
1" as the indentation implied. There doesn't feel like a reason to
Quoted 9 lines
Quoted 3 lines
Quoted 1 line
touch the watchdog if we're not doing anything slow.
No idea. It was like this in Jason's original version from 2008.
Yeah, I noticed the same. I'd be curious what Daneil (or Jason if he's reading) says. I suppose i could always wait until your patch lands and then send a new patch that puts it inside the "if" statement and we can debate it then.
The original board this was developed with was a 32bit eeepc.
The intent was that when v2printk() was called for a verbose > 1
condition the touch_nmi_watchdog() was called. The test case
where a whole lot of single steps are executed sequentially was not letting the watchdog get reset by the normal kernel routine. The serial port was so slow it was pretty easy to hit this problem and it would just power cycle itself.
The original intent would have bee:
#define v2printk(a...) do { \
if (verbose > 1) { \
printk(KERN_INFO a); \
touch_nmi_watchdog(); \
} \
} while (0)I'd guess this probably not the first time gcc-11 is finding brace imbalances.
Cheers,
Jason.
Quoted 10 lines · Jason Wessel
The original board this was developed with was a 32bit eeepc.
The intent was that when
v2printk()was called for a verbose > 1
condition thetouch_nmi_watchdog()was called. The test case
where a whole lot of single steps are executed sequentially was not letting the watchdog get reset by the normal kernel routine. The serial port was so slow it was pretty easy to hit this problem and it would just power cycle itself.
Quoted 8 lines
The original intent would have bee:
#define v2printk(a...) do { \ if (verbose > 1) { \ printk(KERN_INFO a); \ touch_nmi_watchdog(); \ } \} while (0)
Ok, thanks for sharing how the code was intended to work. I'll let you all come up with a decision on what should be done about it now, I'm happy to send one or two patches to address both the compiler warning, and the original mistake.
My feeling is it would be best to address the warning first, pretty much with the patch I sent here, and to change the behavior as a second patch.
That way the gcc-11 warning can be silenced in stable kernels by backporting the first patch, while the second patch can be tried out in new kernels first and might not get backported because the existing behavior is not harmful.
Quoted 2 lines
I'd guess this probably not the first time gcc-11 is finding brace imbalances.
There were only a handful of new -Wmisleading-indentation warnings for gcc-11, the older compilers already caught every instance in normal functions, while gcc-11 improved on finding them in macros as well.
Arnd
Quoted 13 lines · Doug Anderson
drivers/misc/kgdbts.c line 105
Quoted 7 lines
drivers/misc/kgdbts.c +7
-0+if (verbose) \ + printk(KERN_INFO a); \ +} while (0) +#define v2printk(a...) do { \ + if (verbose > 1) \ + printk(KERN_INFO a); \ + touch_nmi_watchdog(); \This
touch_nmi_watchdog()is pretty wonky. I guess maybe the assumption is that the "verbose level 2" prints are so chatty that the printing might prevent us from touching the NMI watchdog in the way that we normally do and thus we need an extra one here?
Quoted 4 lines
...but, in that case, I think the old code was _wrong_ and that the intention was that the
touch_nmi_watchdog()should only be if "veroseQuoted 1 line
1" as the indentation implied. There doesn't feel like a reason to
touch the watchdog if we're not doing anything slow.
I'm not entirely sure I'd like to second guess the intent here. This macro has been there since this file was introduced but several callers have been added since then. We have to guess their intent too!
So, whilst I think you are probably right, v2printk() does appears in places such as the single step test loop which makes it pretty difficult to decide by inspection whether or not touching the watchdog is useful.
It's something that could be further examined... but I'd be a little reluctant to combine it directly with a whitespace change!
Daniel.