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.
Fixes: e8d31c204e36 ("kgdb: add kgdb internal test suite")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/misc/kgdbts.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/misc/kgdbts.c b/drivers/misc/kgdbts.c
index 945701bce553..2e081a58da6c 100644
--- a/drivers/misc/kgdbts.c
+++ b/drivers/misc/kgdbts.c
@@ -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 40
static struct kgdb_io kgdbts_io_ops;
--
2.29.2
On Mon, Mar 22, 2021 at 05:43:03PM +0100, Arnd Bergmann wrote:
> 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.
>
> Fixes: e8d31c204e36 ("kgdb: add kgdb internal test suite")
> Signed-off-by: Arnd Bergmann <[email protected]>
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...
Daniel.
> ---
> drivers/misc/kgdbts.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/misc/kgdbts.c b/drivers/misc/kgdbts.c
> index 945701bce553..2e081a58da6c 100644
> --- a/drivers/misc/kgdbts.c
> +++ b/drivers/misc/kgdbts.c
> @@ -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 40
>
> static struct kgdb_io kgdbts_io_ops;
> --
> 2.29.2
>
Hi,
On Mon, Mar 22, 2021 at 9:43 AM Arnd Bergmann <[email protected]> wrote:
>
> -#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 { \
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).
> + 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
> 1" as the indentation implied. There doesn't feel like a reason to
touch the watchdog if we're not doing anything slow.
-Doug
On Mon, Mar 22, 2021 at 6:03 PM Daniel Thompson
<[email protected]> wrote:
>
>
> 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
On Mon, Mar 22, 2021 at 6:07 PM Doug Anderson <[email protected]> wrote:
> On Mon, Mar 22, 2021 at 9:43 AM Arnd Bergmann <[email protected]> wrote:
> >
> > -#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 { \
>
> 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.
>
> > + 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
> > 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
Hi,
On Mon, Mar 22, 2021 at 11:19 AM Arnd Bergmann <[email protected]> wrote:
>
> On Mon, Mar 22, 2021 at 6:07 PM Doug Anderson <[email protected]> wrote:
> > On Mon, Mar 22, 2021 at 9:43 AM Arnd Bergmann <[email protected]> wrote:
> > >
> > > -#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 { \
> >
> > 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.
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.
> > > + 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
> > > 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
On Mon, Mar 22, 2021 at 10:04:57AM -0700, Doug Anderson wrote:
> > + 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
> > 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.
On 3/22/21 2:22 PM, Doug Anderson wrote:
> Hi,
>
> On Mon, Mar 22, 2021 at 11:19 AM Arnd Bergmann <[email protected]> wrote:
>>
>> On Mon, Mar 22, 2021 at 6:07 PM Doug Anderson <[email protected]> wrote:
>>> On Mon, Mar 22, 2021 at 9:43 AM Arnd Bergmann <[email protected]> wrote:
>>>>
>>>> -#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 { \
>>>
>>> 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.
>
> 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.
>
>
>>>> + 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
>>>> 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.
>
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.
On Mon, Mar 22, 2021 at 9:14 PM Jason Wessel <[email protected]> wrote:
>
> 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)
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.
> 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
On Mon, Mar 22, 2021 at 07:03:45PM +0100, Arnd Bergmann wrote:
> On Mon, Mar 22, 2021 at 6:03 PM Daniel Thompson
> <[email protected]> wrote:
> >
> >
> > 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