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 <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
---
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
On Thu, Jun 30, 2022 at 2:11 AM Justin Stitt <[email protected]> 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
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 <[email protected]>
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> 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
On Thu, Jun 30, 2022 at 1:14 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Thu, Jun 30, 2022 at 2:11 AM Justin Stitt <[email protected]> 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
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 <[email protected]>
Suggested-by: Nathan Chancellor <[email protected]>
Suggested-by: Nick Desaulniers <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
---
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 Thu, Jun 30, 2022 at 9:59 PM Justin Stitt <[email protected]> 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 <[email protected]>
> Suggested-by: Nathan Chancellor <[email protected]>
> Suggested-by: Nick Desaulniers <[email protected]>
> Signed-off-by: Justin Stitt <[email protected]>
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
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 <[email protected]>
Suggested-by: Nick Desaulniers <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
---
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 <[email protected]> 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 <[email protected]>
> Suggested-by: Nick Desaulniers <[email protected]>
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> 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 <[email protected]>
Suggested-by: Nick Desaulniers <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
---
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 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 <[email protected]>
> Suggested-by: Nick Desaulniers <[email protected]>
> Signed-off-by: Justin Stitt <[email protected]>
Looks good to me:
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On Mon, Jul 18, 2022 at 4:06 PM Justin Stitt <[email protected]> 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 <[email protected]>
> Suggested-by: Nick Desaulniers <[email protected]>
> Signed-off-by: Justin Stitt <[email protected]>
Thanks for humoring all of our requests. I'm happy with the result.
Reviewed-by: Nick Desaulniers <[email protected]>
> ---
> 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
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 <[email protected]>
> > Suggested-by: Nick Desaulniers <[email protected]>
> > Signed-off-by: Justin Stitt <[email protected]>
>
> Looks good to me:
>
> Reviewed-by: Petr Mladek <[email protected]>
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 <[email protected]>
> > > Suggested-by: Nick Desaulniers <[email protected]>
> > > Signed-off-by: Justin Stitt <[email protected]>
> >
> > Looks good to me:
> >
> > Reviewed-by: Petr Mladek <[email protected]>
>
> 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