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

/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

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

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

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

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

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