[PATCH] lib/test_printf.c: fix clang -Wformat warnings

[PATCH] lib/test_printf.c: fix clang -Wformat warnings

[Justin Stitt]

see warnings:
| lib/test_printf.c:157:52: error: format specifies type 'unsigned char'
| but the argument has type 'int' [-Werror,-Wformat] test("0|1|1|128|255",
| "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
-
| lib/test_printf.c:158:55: error: format specifies type 'char' but the
| argument has type 'int' [-Werror,-Wformat] test("0|1|1|-128|-1",
| "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
-
| lib/test_printf.c:159:41: error: format specifies type 'unsigned short'
| but the argument has type 'int' [-Werror,-Wformat]
| test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);

There's an ongoing movement to eventually enable the -Wformat flag for
clang. Previous patches have targeted incorrect usage of
format specifiers. In this case, however, the "incorrect" format
specifiers are intrinsically part of the test cases. Hence, fixing them
would be misaligned with their intended purpose. My proposed fix is to
simply disable the warnings so that one day a clean build of the kernel
with clang (and -Wformat enabled) would be possible. It would also keep
us in the green for alot of the CI bots.

Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
 lib/test_printf.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 07309c45f327..748591a0c55c 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -4,6 +4,12 @@
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define DO_PRAGMA(x) _Pragma(#x)
+#define NOWARN(warnoption, ...)
+	    DO_PRAGMA(GCC diagnostic push)
+		    DO_PRAGMA(GCC diagnostic ignored #warnoption)
+			    __VA_ARGS__
+				    DO_PRAGMA(GCC diagnostic pop)
 
 #include <linux/init.h>
 #include <linux/kernel.h>
@@ -154,9 +160,13 @@ test_number(void)
 	test("0x1234abcd  ", "%#-12x", 0x1234abcd);
 	test("  0x1234abcd", "%#12x", 0x1234abcd);
 	test("0|001| 12|+123| 1234|-123|-1234", "%d|%03d|%3d|%+d|% d|%+d|% d", 0, 1, 12, 123, 1234, -123, -1234);
+	/* disable -Wformat for this chunk */
+	NOWARN(-Wformat, 
 	test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
 	test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
 	test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
+	)
+	/* end chunk */
 	/*
 	 * POSIX/C99: »The result of converting zero with an explicit
 	 * precision of zero shall be no characters.« Hence the output
-- 
2.37.0.rc0.161.g10f37bed90-goog

Re: [PATCH] lib/test_printf.c: fix clang -Wformat warnings

[Andy Shevchenko]

On Thu, Jun 30, 2022 at 2:11 AM Justin Stitt <justinstitt@google.com> wrote:
>
> see warnings:
> | lib/test_printf.c:157:52: error: format specifies type 'unsigned char'
> | but the argument has type 'int' [-Werror,-Wformat] test("0|1|1|128|255",
> | "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
> -
> | lib/test_printf.c:158:55: error: format specifies type 'char' but the
> | argument has type 'int' [-Werror,-Wformat] test("0|1|1|-128|-1",
> | "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
> -
> | lib/test_printf.c:159:41: error: format specifies type 'unsigned short'
> | but the argument has type 'int' [-Werror,-Wformat]
> | test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
>
> There's an ongoing movement to eventually enable the -Wformat flag for
> clang. Previous patches have targeted incorrect usage of
> format specifiers. In this case, however, the "incorrect" format
> specifiers are intrinsically part of the test cases. Hence, fixing them
> would be misaligned with their intended purpose. My proposed fix is to
> simply disable the warnings so that one day a clean build of the kernel
> with clang (and -Wformat enabled) would be possible. It would also keep
> us in the green for alot of the CI bots.

a lot

...

>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

+ blank line

> +#define DO_PRAGMA(x) _Pragma(#x)
> +#define NOWARN(warnoption, ...)
> +           DO_PRAGMA(GCC diagnostic push)
> +                   DO_PRAGMA(GCC diagnostic ignored #warnoption)
> +                           __VA_ARGS__
> +                                   DO_PRAGMA(GCC diagnostic pop)

Is it deliberately a broken indentation here?

...

> +       /* disable -Wformat for this chunk */
> +       NOWARN(-Wformat,
>         test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
>         test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
>         test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);

Perhaps shift right the lines as well?

> +       )
> +       /* end chunk */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] lib/test_printf.c: fix clang -Wformat warnings

[Nick Desaulniers]

On Thu, Jun 30, 2022 at 1:14 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Jun 30, 2022 at 2:11 AM Justin Stitt <justinstitt@google.com> wrote:
> >
> > +       /* disable -Wformat for this chunk */
> > +       NOWARN(-Wformat,
> >         test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
> >         test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
> >         test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
>
> Perhaps shift right the lines as well?

Along these lines, I think it would look nicer to pass a block
statement (a group of statements) to the macro rather than use
__VA_ARGS__.  Here's an example:
https://godbolt.org/z/fsYcGGEMb

You have to be careful with control flow out of blocks like this
sometimes, but for these simple localized cases it looks like that
should be fine.

As Nathan mentions, you can probably re-use the existing infra in your
definition of NOWARN.  I do prefer some macro to make it appear that
the pragma is scoped to a block statement, rather than multiple lines
for the diag push + pop inline.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] lib/test_printf.c: fix clang -Wformat warnings

[Nathan Chancellor]

Hi Justin,

On Wed, Jun 29, 2022 at 04:53:26PM -0700, Justin Stitt wrote:
> see warnings:
> | lib/test_printf.c:157:52: error: format specifies type 'unsigned char'
> | but the argument has type 'int' [-Werror,-Wformat] test("0|1|1|128|255",
> | "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
> -
> | lib/test_printf.c:158:55: error: format specifies type 'char' but the
> | argument has type 'int' [-Werror,-Wformat] test("0|1|1|-128|-1",
> | "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
> -
> | lib/test_printf.c:159:41: error: format specifies type 'unsigned short'
> | but the argument has type 'int' [-Werror,-Wformat]
> | test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
> 
> There's an ongoing movement to eventually enable the -Wformat flag for
> clang. Previous patches have targeted incorrect usage of
> format specifiers. In this case, however, the "incorrect" format
> specifiers are intrinsically part of the test cases. Hence, fixing them
> would be misaligned with their intended purpose. My proposed fix is to
> simply disable the warnings so that one day a clean build of the kernel
> with clang (and -Wformat enabled) would be possible. It would also keep
> us in the green for alot of the CI bots.
> 
> Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
>  lib/test_printf.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 07309c45f327..748591a0c55c 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -4,6 +4,12 @@
>   */
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +#define DO_PRAGMA(x) _Pragma(#x)
> +#define NOWARN(warnoption, ...)
> +	    DO_PRAGMA(GCC diagnostic push)
> +		    DO_PRAGMA(GCC diagnostic ignored #warnoption)
> +			    __VA_ARGS__
> +				    DO_PRAGMA(GCC diagnostic pop)

This is basically a duplicate of the __diag infrastructure that exists
in include/linux/compiler_types.h and include/linux/compiler-clang.h, I
think we should just reuse that here. It eliminates this hunk and the
hunk below...

>  #include <linux/init.h>
>  #include <linux/kernel.h>
> @@ -154,9 +160,13 @@ test_number(void)
>  	test("0x1234abcd  ", "%#-12x", 0x1234abcd);
>  	test("  0x1234abcd", "%#12x", 0x1234abcd);
>  	test("0|001| 12|+123| 1234|-123|-1234", "%d|%03d|%3d|%+d|% d|%+d|% d", 0, 1, 12, 123, 1234, -123, -1234);
> +	/* disable -Wformat for this chunk */
> +	NOWARN(-Wformat, 
>  	test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
>  	test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
>  	test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
> +	)
> +	/* end chunk */
>  	/*
>  	 * POSIX/C99: »The result of converting zero with an explicit
>  	 * precision of zero shall be no characters.« Hence the output

...becomes a lot simpler in my opinion (feel free to reword the comment
however you want).

Cheers,
Nathan

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 8010de49b6c5..86e088ad9e48 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -154,9 +154,12 @@ test_number(void)
 	test("0x1234abcd  ", "%#-12x", 0x1234abcd);
 	test("  0x1234abcd", "%#12x", 0x1234abcd);
 	test("0|001| 12|+123| 1234|-123|-1234", "%d|%03d|%3d|%+d|% d|%+d|% d", 0, 1, 12, 123, 1234, -123, -1234);
+	__diag_push();
+	__diag_ignore(clang, 11, "-Wformat", "These trigger clang's -Wformat and the specifiers should not be changed.");
 	test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
 	test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
 	test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
+	__diag_pop();
 	/*
 	 * POSIX/C99: »The result of converting zero with an explicit
 	 * precision of zero shall be no characters.« Hence the output

[PATCH v2] lib/test_printf.c: fix clang -Wformat warnings

[Justin Stitt]

changes from v1:
* moved NOWARN macro definition to a more appropriate location
* using __diag_ignore_all (thanks Nathan)
* using local scoping for code blocks instead of __VA_ARGS__ (thanks Nick)
* indented affected test cases (thanks Andy)

Suggested-by: Andy Shevchenko <andy@kernel.org>
Suggested-by: Nathan Chancellor <nathan@kernel.org>
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
 lib/test_printf.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 07309c45f327..1b1755ce9fa7 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -30,6 +30,9 @@
 #define PAD_SIZE 16
 #define FILL_CHAR '$'
 
+#define NOWARN(option, comment, block) \
+		__diag_push() __diag_ignore_all(#option, comment) block __diag_pop()
+
 KSTM_MODULE_GLOBALS();
 
 static char *test_buffer __initdata;
@@ -154,9 +157,11 @@ test_number(void)
 	test("0x1234abcd  ", "%#-12x", 0x1234abcd);
 	test("  0x1234abcd", "%#12x", 0x1234abcd);
 	test("0|001| 12|+123| 1234|-123|-1234", "%d|%03d|%3d|%+d|% d|%+d|% d", 0, 1, 12, 123, 1234, -123, -1234);
-	test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
-	test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
-	test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
+	NOWARN(-Wformat, "Disables clang -Wformat warning", {
+		test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
+		test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
+		test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
+	})
 	/*
 	 * POSIX/C99: »The result of converting zero with an explicit
 	 * precision of zero shall be no characters.« Hence the output
-- 
2.37.0.rc0.161.g10f37bed90-goog

Re: [PATCH v2] lib/test_printf.c: fix clang -Wformat warnings

[Andy Shevchenko]

On Thu, Jun 30, 2022 at 9:59 PM Justin Stitt <justinstitt@google.com> wrote:
>
> changes from v1:
> * moved NOWARN macro definition to a more appropriate location
> * using __diag_ignore_all (thanks Nathan)
> * using local scoping for code blocks instead of __VA_ARGS__ (thanks Nick)
> * indented affected test cases (thanks Andy)
>
> Suggested-by: Andy Shevchenko <andy@kernel.org>
> Suggested-by: Nathan Chancellor <nathan@kernel.org>
> Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Justin Stitt <justinstitt@google.com>

Somehow you replaced the commit message with a changelog. On top of
that, I didn't suggest anything important here, so to me it is
considered as a credit in the changelog (see previous sentence as
well).

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3] lib/test_printf.c: fix clang -Wformat warnings

[Justin Stitt]

see warnings:
| lib/test_printf.c:157:52: error: format specifies type 'unsigned char'
| but the argument has type 'int' [-Werror,-Wformat]
test("0|1|1|128|255",
| "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
-
| lib/test_printf.c:158:55: error: format specifies type 'char' but the
| argument has type 'int' [-Werror,-Wformat] test("0|1|1|-128|-1",
| "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
-
| lib/test_printf.c:159:41: error: format specifies type 'unsigned
short'
| but the argument has type 'int' [-Werror,-Wformat]
| test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);

There's an ongoing movement to eventually enable the -Wformat flag for
clang. Previous patches have targeted incorrect usage of
format specifiers. In this case, however, the "incorrect" format
specifiers are intrinsically part of the test cases. Hence, fixing them
would be misaligned with their intended purpose. My proposed fix is to
simply disable the warnings so that one day a clean build of the kernel
with clang (and -Wformat enabled) would be possible. It would also keep
us in the green for alot of the CI bots.

Link: https://github.com/ClangBuiltLinux/linux/issues/378
Suggested-by: Nathan Chancellor <nathan@kernel.org>
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
changes from v1 -> v2:
* moved NOWARN macro definition to a more appropriate location
* using __diag_ignore_all (thanks Nathan)
* using local scoping for code blocks instead of __VA_ARGS__ (thanks Nick)
* indented affected test cases (thanks Andy)

changes from v2 -> v3:
* reinserted commit message
* remove Andy's Suggested-by tag
* add issue tracker link

 lib/test_printf.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 07309c45f327..1b1755ce9fa7 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -30,6 +30,9 @@
 #define PAD_SIZE 16
 #define FILL_CHAR '$'
 
+#define NOWARN(option, comment, block) \
+		__diag_push() __diag_ignore_all(#option, comment) block __diag_pop()
+
 KSTM_MODULE_GLOBALS();
 
 static char *test_buffer __initdata;
@@ -154,9 +157,11 @@ test_number(void)
 	test("0x1234abcd  ", "%#-12x", 0x1234abcd);
 	test("  0x1234abcd", "%#12x", 0x1234abcd);
 	test("0|001| 12|+123| 1234|-123|-1234", "%d|%03d|%3d|%+d|% d|%+d|% d", 0, 1, 12, 123, 1234, -123, -1234);
-	test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
-	test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
-	test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
+	NOWARN(-Wformat, "Disables clang -Wformat warning", {
+		test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
+		test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
+		test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
+	})
 	/*
 	 * POSIX/C99: »The result of converting zero with an explicit
 	 * precision of zero shall be no characters.« Hence the output
-- 
2.37.0.rc0.161.g10f37bed90-goog

Re: [PATCH v3] lib/test_printf.c: fix clang -Wformat warnings

[Nick Desaulniers]

/On Mon, Jul 18, 2022 at 10:29 AM Justin Stitt <justinstitt@google.com> wrote:
>
> see warnings:
> | lib/test_printf.c:157:52: error: format specifies type 'unsigned char'
> | but the argument has type 'int' [-Werror,-Wformat]
> test("0|1|1|128|255",
> | "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
> -
> | lib/test_printf.c:158:55: error: format specifies type 'char' but the
> | argument has type 'int' [-Werror,-Wformat] test("0|1|1|-128|-1",
> | "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
> -
> | lib/test_printf.c:159:41: error: format specifies type 'unsigned
> short'
> | but the argument has type 'int' [-Werror,-Wformat]
> | test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
>
> There's an ongoing movement to eventually enable the -Wformat flag for
> clang. Previous patches have targeted incorrect usage of
> format specifiers. In this case, however, the "incorrect" format
> specifiers are intrinsically part of the test cases. Hence, fixing them
> would be misaligned with their intended purpose. My proposed fix is to
> simply disable the warnings so that one day a clean build of the kernel
> with clang (and -Wformat enabled) would be possible. It would also keep
> us in the green for alot of the CI bots.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/378
> Suggested-by: Nathan Chancellor <nathan@kernel.org>
> Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> changes from v1 -> v2:
> * moved NOWARN macro definition to a more appropriate location
> * using __diag_ignore_all (thanks Nathan)
> * using local scoping for code blocks instead of __VA_ARGS__ (thanks Nick)
> * indented affected test cases (thanks Andy)
>
> changes from v2 -> v3:
> * reinserted commit message
> * remove Andy's Suggested-by tag
> * add issue tracker link
>
>  lib/test_printf.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 07309c45f327..1b1755ce9fa7 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -30,6 +30,9 @@
>  #define PAD_SIZE 16
>  #define FILL_CHAR '$'
>
> +#define NOWARN(option, comment, block) \
> +               __diag_push() __diag_ignore_all(#option, comment) block __diag_pop()

Do you mind putting these 4 on distinct lines with \ at the end of the
lines, and terminate the statements with an additional ; if that
doesn't produce new warnings about empty statements? Maybe something
like:

+#define NOWARN(option, comment, block) \
+  __diag_push(); \
+  __diag_ignore_all(#option, comment); \
+ block \
+ __diag_pop();

(sorry for not formatting that myself, remember to use tabs vs
spaces).  To me, it seems more readable from a quick glance to have
these be distinct lines.  You'll find open coded uses of __diag_push
and friends add the semicolon, even if not strictly necessary.

> +
>  KSTM_MODULE_GLOBALS();
>
>  static char *test_buffer __initdata;
> @@ -154,9 +157,11 @@ test_number(void)
>         test("0x1234abcd  ", "%#-12x", 0x1234abcd);
>         test("  0x1234abcd", "%#12x", 0x1234abcd);
>         test("0|001| 12|+123| 1234|-123|-1234", "%d|%03d|%3d|%+d|% d|%+d|% d", 0, 1, 12, 123, 1234, -123, -1234);
> -       test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
> -       test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
> -       test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
> +       NOWARN(-Wformat, "Disables clang -Wformat warning", {

Technically, this is disabling -Wformat for all compilers.  How about
a comment string like "Intentionally test narrowing conversion
specifiers"?

> +               test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
> +               test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
> +               test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
> +       })
>         /*
>          * POSIX/C99: »The result of converting zero with an explicit
>          * precision of zero shall be no characters.« Hence the output
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>


-- 
Thanks,
~Nick Desaulniers

[PATCH v4] lib/test_printf.c: fix clang -Wformat warnings

[Justin Stitt]

see warnings:
| lib/test_printf.c:157:52: error: format specifies type 'unsigned char'
| but the argument has type 'int' [-Werror,-Wformat]
test("0|1|1|128|255",
| "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
-
| lib/test_printf.c:158:55: error: format specifies type 'char' but the
| argument has type 'int' [-Werror,-Wformat] test("0|1|1|-128|-1",
| "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
-
| lib/test_printf.c:159:41: error: format specifies type 'unsigned
short'
| but the argument has type 'int' [-Werror,-Wformat]
| test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);

There's an ongoing movement to eventually enable the -Wformat flag for
clang. Previous patches have targeted incorrect usage of
format specifiers. In this case, however, the "incorrect" format
specifiers are intrinsically part of the test cases. Hence, fixing them
would be misaligned with their intended purpose. My proposed fix is to
simply disable the warnings so that one day a clean build of the kernel
with clang (and -Wformat enabled) would be possible. It would also keep
us in the green for alot of the CI bots.

Link: https://github.com/ClangBuiltLinux/linux/issues/378
Suggested-by: Nathan Chancellor <nathan@kernel.org>
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
changes from v1 -> v2:
* moved NOWARN macro definition to a more appropriate location
* using __diag_ignore_all (thanks Nathan)
* using local scoping for code blocks instead of __VA_ARGS__ (thanks
* Nick)
* indented affected test cases (thanks Andy)

changes from v2 -> v3:
* reinserted commit message
* remove Andy's Suggested-by tag
* add issue tracker link

changes from v3 -> v4:
* better macro indentation and usage string (thanks Nick)

 lib/test_printf.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 07309c45f327..f78044c1efaa 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -30,6 +30,12 @@
 #define PAD_SIZE 16
 #define FILL_CHAR '$'
 
+#define NOWARN(option, comment, block) \
+	__diag_push(); \
+	__diag_ignore_all(#option, comment); \
+	block \
+	__diag_pop();
+
 KSTM_MODULE_GLOBALS();
 
 static char *test_buffer __initdata;
@@ -154,9 +160,11 @@ test_number(void)
 	test("0x1234abcd  ", "%#-12x", 0x1234abcd);
 	test("  0x1234abcd", "%#12x", 0x1234abcd);
 	test("0|001| 12|+123| 1234|-123|-1234", "%d|%03d|%3d|%+d|% d|%+d|% d", 0, 1, 12, 123, 1234, -123, -1234);
-	test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
-	test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
-	test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
+	NOWARN(-Wformat, "Intentionally test narrowing conversion specifiers.", {
+		test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
+		test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
+		test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
+	})
 	/*
 	 * POSIX/C99: »The result of converting zero with an explicit
 	 * precision of zero shall be no characters.« Hence the output
-- 
2.37.0.170.g444d1eabd0-goog

Re: [PATCH v4] lib/test_printf.c: fix clang -Wformat warnings

[Petr Mladek]

On Mon 2022-07-18 16:06:26, Justin Stitt wrote:
> see warnings:
> | lib/test_printf.c:157:52: error: format specifies type 'unsigned char'
> | but the argument has type 'int' [-Werror,-Wformat]
> test("0|1|1|128|255",
> | "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
> -
> | lib/test_printf.c:158:55: error: format specifies type 'char' but the
> | argument has type 'int' [-Werror,-Wformat] test("0|1|1|-128|-1",
> | "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
> -
> | lib/test_printf.c:159:41: error: format specifies type 'unsigned
> short'
> | but the argument has type 'int' [-Werror,-Wformat]
> | test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
> 
> There's an ongoing movement to eventually enable the -Wformat flag for
> clang. Previous patches have targeted incorrect usage of
> format specifiers. In this case, however, the "incorrect" format
> specifiers are intrinsically part of the test cases. Hence, fixing them
> would be misaligned with their intended purpose. My proposed fix is to
> simply disable the warnings so that one day a clean build of the kernel
> with clang (and -Wformat enabled) would be possible. It would also keep
> us in the green for alot of the CI bots.
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/378
> Suggested-by: Nathan Chancellor <nathan@kernel.org>
> Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Justin Stitt <justinstitt@google.com>

Looks good to me:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v4] lib/test_printf.c: fix clang -Wformat warnings

[Nathan Chancellor]

Hi Petr,

On Tue, Jul 19, 2022 at 02:17:47PM +0200, Petr Mladek wrote:
> On Mon 2022-07-18 16:06:26, Justin Stitt wrote:
> > see warnings:
> > | lib/test_printf.c:157:52: error: format specifies type 'unsigned char'
> > | but the argument has type 'int' [-Werror,-Wformat]
> > test("0|1|1|128|255",
> > | "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
> > -
> > | lib/test_printf.c:158:55: error: format specifies type 'char' but the
> > | argument has type 'int' [-Werror,-Wformat] test("0|1|1|-128|-1",
> > | "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
> > -
> > | lib/test_printf.c:159:41: error: format specifies type 'unsigned
> > short'
> > | but the argument has type 'int' [-Werror,-Wformat]
> > | test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
> > 
> > There's an ongoing movement to eventually enable the -Wformat flag for
> > clang. Previous patches have targeted incorrect usage of
> > format specifiers. In this case, however, the "incorrect" format
> > specifiers are intrinsically part of the test cases. Hence, fixing them
> > would be misaligned with their intended purpose. My proposed fix is to
> > simply disable the warnings so that one day a clean build of the kernel
> > with clang (and -Wformat enabled) would be possible. It would also keep
> > us in the green for alot of the CI bots.
> > 
> > Link: https://github.com/ClangBuiltLinux/linux/issues/378
> > Suggested-by: Nathan Chancellor <nathan@kernel.org>
> > Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> > Signed-off-by: Justin Stitt <justinstitt@google.com>
> 
> Looks good to me:
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>

Would you be able to take this for 5.20 or should we ask Andrew to pick
it up? It seems you two seem to split applying patches to this file and
we are trying to get -Wformat enabled for clang in 5.20.

Cheers,
Nathan

Re: [PATCH v4] lib/test_printf.c: fix clang -Wformat warnings

[Petr Mladek]

On Wed 2022-07-27 12:39:32, Nathan Chancellor wrote:
> Hi Petr,
> 
> On Tue, Jul 19, 2022 at 02:17:47PM +0200, Petr Mladek wrote:
> > On Mon 2022-07-18 16:06:26, Justin Stitt wrote:
> > > see warnings:
> > > | lib/test_printf.c:157:52: error: format specifies type 'unsigned char'
> > > | but the argument has type 'int' [-Werror,-Wformat]
> > > test("0|1|1|128|255",
> > > | "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
> > > -
> > > | lib/test_printf.c:158:55: error: format specifies type 'char' but the
> > > | argument has type 'int' [-Werror,-Wformat] test("0|1|1|-128|-1",
> > > | "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
> > > -
> > > | lib/test_printf.c:159:41: error: format specifies type 'unsigned
> > > short'
> > > | but the argument has type 'int' [-Werror,-Wformat]
> > > | test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
> > > 
> > > There's an ongoing movement to eventually enable the -Wformat flag for
> > > clang. Previous patches have targeted incorrect usage of
> > > format specifiers. In this case, however, the "incorrect" format
> > > specifiers are intrinsically part of the test cases. Hence, fixing them
> > > would be misaligned with their intended purpose. My proposed fix is to
> > > simply disable the warnings so that one day a clean build of the kernel
> > > with clang (and -Wformat enabled) would be possible. It would also keep
> > > us in the green for alot of the CI bots.
> > > 
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/378
> > > Suggested-by: Nathan Chancellor <nathan@kernel.org>
> > > Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> > > Signed-off-by: Justin Stitt <justinstitt@google.com>
> > 
> > Looks good to me:
> > 
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> Would you be able to take this for 5.20 or should we ask Andrew to pick
> it up? It seems you two seem to split applying patches to this file and
> we are trying to get -Wformat enabled for clang in 5.20.

I take most vsprintf-related patches via the printk git tree
last few years.

Anyway, I have just committed the patch into printk/linux.git,
branch for-5.20.

Best Regards,
Petr

Re: [PATCH v4] lib/test_printf.c: fix clang -Wformat warnings

[Nick Desaulniers]

On Mon, Jul 18, 2022 at 4:06 PM Justin Stitt <justinstitt@google.com> wrote:
>
> see warnings:
> | lib/test_printf.c:157:52: error: format specifies type 'unsigned char'
> | but the argument has type 'int' [-Werror,-Wformat]
> test("0|1|1|128|255",
> | "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
> -
> | lib/test_printf.c:158:55: error: format specifies type 'char' but the
> | argument has type 'int' [-Werror,-Wformat] test("0|1|1|-128|-1",
> | "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
> -
> | lib/test_printf.c:159:41: error: format specifies type 'unsigned
> short'
> | but the argument has type 'int' [-Werror,-Wformat]
> | test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
>
> There's an ongoing movement to eventually enable the -Wformat flag for
> clang. Previous patches have targeted incorrect usage of
> format specifiers. In this case, however, the "incorrect" format
> specifiers are intrinsically part of the test cases. Hence, fixing them
> would be misaligned with their intended purpose. My proposed fix is to
> simply disable the warnings so that one day a clean build of the kernel
> with clang (and -Wformat enabled) would be possible. It would also keep
> us in the green for alot of the CI bots.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/378
> Suggested-by: Nathan Chancellor <nathan@kernel.org>
> Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Justin Stitt <justinstitt@google.com>

Thanks for humoring all of our requests. I'm happy with the result.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
> changes from v1 -> v2:
> * moved NOWARN macro definition to a more appropriate location
> * using __diag_ignore_all (thanks Nathan)
> * using local scoping for code blocks instead of __VA_ARGS__ (thanks
> * Nick)
> * indented affected test cases (thanks Andy)
>
> changes from v2 -> v3:
> * reinserted commit message
> * remove Andy's Suggested-by tag
> * add issue tracker link
>
> changes from v3 -> v4:
> * better macro indentation and usage string (thanks Nick)
>
>  lib/test_printf.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 07309c45f327..f78044c1efaa 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -30,6 +30,12 @@
>  #define PAD_SIZE 16
>  #define FILL_CHAR '$'
>
> +#define NOWARN(option, comment, block) \
> +       __diag_push(); \
> +       __diag_ignore_all(#option, comment); \
> +       block \
> +       __diag_pop();
> +
>  KSTM_MODULE_GLOBALS();
>
>  static char *test_buffer __initdata;
> @@ -154,9 +160,11 @@ test_number(void)
>         test("0x1234abcd  ", "%#-12x", 0x1234abcd);
>         test("  0x1234abcd", "%#12x", 0x1234abcd);
>         test("0|001| 12|+123| 1234|-123|-1234", "%d|%03d|%3d|%+d|% d|%+d|% d", 0, 1, 12, 123, 1234, -123, -1234);
> -       test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
> -       test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
> -       test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
> +       NOWARN(-Wformat, "Intentionally test narrowing conversion specifiers.", {
> +               test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
> +               test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
> +               test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
> +       })
>         /*
>          * POSIX/C99: »The result of converting zero with an explicit
>          * precision of zero shall be no characters.« Hence the output
> --
> 2.37.0.170.g444d1eabd0-goog
>
>


-- 
Thanks,
~Nick Desaulniers