* [PATCH] [12/11/10] Fix invalid format warnings on Windows @ 2022-01-07 18:33 Tomas Kalibera 2022-01-11 13:37 ` Martin Liška 0 siblings, 1 reply; 10+ messages in thread From: Tomas Kalibera @ 2022-01-07 18:33 UTC (permalink / raw) To: gcc-patches; +Cc: joseph, martin, redi, mliska Mingw32 targets use ms_printf format for printf, but mingw-w64 when configured for UCRT uses gnu_format (via stdio.h). GCC then checks both formats, which means that one cannot print a 64-bit integer without a warning. All these lines issue a warning: printf("Hello %"PRIu64"\n", x); // 1 printf("Hello %I64u\n", x); // 2 printf("Hello %llu\n", x); // 3 because each of them violates one of the formats. This causes trouble particularly for systems that turn warnings into errors or otherwise require no warnings (leading to the use of -Wno-format or of various printf replacements). Also, one gets a warning twice if the format string violates both formats. These issues have been reported as PR 95130 and PR 92292: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95130 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92292 This patch fixes these issues following the suggestion of Joseph Myers, it disables the built in format in case there are additional ones. It applies to GCC 12, 11, 10 and fixes the example above as tested on cross compilers built on Linux. I've also verified that R built using a 10.3 native compiler with the patch applied builds and passes its tests. I've updated the patch based on advice and comments from Martin Liska and Martin Storsjo. Could this or a variant of please be accepted to 12/11/10? Thanks Tomas gcc/c-family/ChangeLog: * c-common.c (check_function_arguments): Pass also function declaration to check_function_format. * c-common.h (check_function_format): Extra argument - function declaration. * c-format.c (check_function_format): For builtin functions with a built in format and at least one more, do not check the first one. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 13341fa315e..be4d8400447 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -6057,7 +6057,7 @@ check_function_arguments (location_t loc, const_tree fndecl, const_tree fntype, /* Check for errors in format strings. */ if (warn_format || warn_suggest_attribute_format) - check_function_format (fntype, TYPE_ATTRIBUTES (fntype), nargs, argarray, + check_function_format (fndecl, fntype, TYPE_ATTRIBUTES (fntype), nargs, argarray, arglocs); if (warn_format) diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index 8b7bf35e888..ee370eafbbc 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -856,7 +856,7 @@ extern void check_function_arguments_recurse (void (*) unsigned HOST_WIDE_INT); extern bool check_builtin_function_arguments (location_t, vec<location_t>, tree, tree, int, tree *); -extern void check_function_format (const_tree, tree, int, tree *, +extern void check_function_format (const_tree, const_tree, tree, int, tree *, vec<location_t> *); extern bool attribute_fallthrough_p (tree); extern tree handle_format_attribute (tree *, tree, tree, int, bool *); diff --git a/gcc/c-family/c-format.c b/gcc/c-family/c-format.c index afa77810a5c..8155ee8c6f2 100644 --- a/gcc/c-family/c-format.c +++ b/gcc/c-family/c-format.c @@ -1160,12 +1160,13 @@ decode_format_type (const char *s, bool *is_raw /* = NULL */) attribute themselves. */ void -check_function_format (const_tree fntype, tree attrs, int nargs, +check_function_format (const_tree fndecl, const_tree fntype, tree attrs, int nargs, tree *argarray, vec<location_t> *arglocs) { - tree a; + tree a, aa; tree atname = get_identifier ("format"); + int skipped_default_format = 0; /* See if this function has any format attributes. */ for (a = attrs; a; a = TREE_CHAIN (a)) @@ -1176,6 +1177,58 @@ check_function_format (const_tree fntype, tree attrs, int nargs, function_format_info info; decode_format_attr (fntype, atname, TREE_VALUE (a), &info, /*validated=*/true); + + /* Mingw32 targets have traditionally used ms_printf format for the + printf function, and this format is built in GCC. But nowadays, + if mingw-w64 is configured to target UCRT, the printf function + uses the gnu_printf format (specified in the stdio.h header). This + causes GCC to check both formats, which means that there is no way + to e.g. print a long long unsigned without a warning (ms_printf + warns for %llu and gnu_printf warns for %I64u). Also, GCC would warn + twice about the same issue when both formats are violated, e.g. + for %lu used to print long long unsigned. + + Hence, if there are multiple format specifiers, we skip the first + one. See PR 95130, PR 92292. */ + + if (!skipped_default_format && fndecl) + { + for(aa = TREE_CHAIN (a); aa; aa = TREE_CHAIN(aa)) + { + if (is_attribute_p ("format", get_attribute_name (aa)) && + fndecl && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)) + { + switch (DECL_FUNCTION_CODE (fndecl)) + { + case BUILT_IN_FSCANF: + case BUILT_IN_PRINTF: + case BUILT_IN_SCANF: + case BUILT_IN_SNPRINTF: + case BUILT_IN_SSCANF: + case BUILT_IN_VFSCANF: + case BUILT_IN_VPRINTF: + case BUILT_IN_VSCANF: + case BUILT_IN_VSNPRINTF: + case BUILT_IN_VSSCANF: + case BUILT_IN_DCGETTEXT: + case BUILT_IN_DGETTEXT: + case BUILT_IN_GETTEXT: + case BUILT_IN_STRFMON: + case BUILT_IN_STRFTIME: + case BUILT_IN_SNPRINTF_CHK: + case BUILT_IN_VSNPRINTF_CHK: + case BUILT_IN_PRINTF_CHK: + case BUILT_IN_VPRINTF_CHK: + skipped_default_format = 1; + break; + default: + break; + } + } + } + if (skipped_default_format) continue; + } + if (warn_format) { /* FIXME: Rewrite all the internal functions in this file ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [12/11/10] Fix invalid format warnings on Windows 2022-01-07 18:33 [PATCH] [12/11/10] Fix invalid format warnings on Windows Tomas Kalibera @ 2022-01-11 13:37 ` Martin Liška 2022-01-12 13:34 ` Tomas Kalibera 0 siblings, 1 reply; 10+ messages in thread From: Martin Liška @ 2022-01-11 13:37 UTC (permalink / raw) To: Tomas Kalibera, gcc-patches; +Cc: joseph, martin, redi Hello. I do support the patch, but I would ... On 1/7/22 19:33, Tomas Kalibera wrote: > + if (is_attribute_p ("format", get_attribute_name (aa)) && > + fndecl && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)) > + { > + switch (DECL_FUNCTION_CODE (fndecl)) > + { > + case BUILT_IN_FSCANF: > + case BUILT_IN_PRINTF: > + case BUILT_IN_SCANF: > + case BUILT_IN_SNPRINTF: > + case BUILT_IN_SSCANF: > + case BUILT_IN_VFSCANF: > + case BUILT_IN_VPRINTF: > + case BUILT_IN_VSCANF: > + case BUILT_IN_VSNPRINTF: > + case BUILT_IN_VSSCANF: > + case BUILT_IN_DCGETTEXT: > + case BUILT_IN_DGETTEXT: > + case BUILT_IN_GETTEXT: > + case BUILT_IN_STRFMON: > + case BUILT_IN_STRFTIME: > + case BUILT_IN_SNPRINTF_CHK: > + case BUILT_IN_VSNPRINTF_CHK: > + case BUILT_IN_PRINTF_CHK: > + case BUILT_IN_VPRINTF_CHK: > + skipped_default_format = 1; > + break; > + default: > + break; > + } > + } ... skip this as the listed functions are only these that have defined ATTR_FORMAT_*: $ grep ATTR_FORMAT gcc/builtins.def DEF_LIB_BUILTIN (BUILT_IN_FSCANF, "fscanf", BT_FN_INT_FILEPTR_CONST_STRING_VAR, ATTR_FORMAT_SCANF_2_3) DEF_LIB_BUILTIN (BUILT_IN_PRINTF, "printf", BT_FN_INT_CONST_STRING_VAR, ATTR_FORMAT_PRINTF_1_2) DEF_LIB_BUILTIN (BUILT_IN_SCANF, "scanf", BT_FN_INT_CONST_STRING_VAR, ATTR_FORMAT_SCANF_1_2) DEF_C99_BUILTIN (BUILT_IN_SNPRINTF, "snprintf", BT_FN_INT_STRING_SIZE_CONST_STRING_VAR, ATTR_FORMAT_PRINTF_NOTHROW_3_4) DEF_LIB_BUILTIN (BUILT_IN_SSCANF, "sscanf", BT_FN_INT_CONST_STRING_CONST_STRING_VAR, ATTR_FORMAT_SCANF_NOTHROW_2_3) DEF_C99_BUILTIN (BUILT_IN_VFSCANF, "vfscanf", BT_FN_INT_FILEPTR_CONST_STRING_VALIST_ARG, ATTR_FORMAT_SCANF_2_0) DEF_LIB_BUILTIN (BUILT_IN_VPRINTF, "vprintf", BT_FN_INT_CONST_STRING_VALIST_ARG, ATTR_FORMAT_PRINTF_1_0) DEF_C99_BUILTIN (BUILT_IN_VSCANF, "vscanf", BT_FN_INT_CONST_STRING_VALIST_ARG, ATTR_FORMAT_SCANF_1_0) DEF_C99_BUILTIN (BUILT_IN_VSNPRINTF, "vsnprintf", BT_FN_INT_STRING_SIZE_CONST_STRING_VALIST_ARG, ATTR_FORMAT_PRINTF_NOTHROW_3_0) DEF_C99_BUILTIN (BUILT_IN_VSSCANF, "vsscanf", BT_FN_INT_CONST_STRING_CONST_STRING_VALIST_ARG, ATTR_FORMAT_SCANF_NOTHROW_2_0) DEF_EXT_LIB_BUILTIN (BUILT_IN_DCGETTEXT, "dcgettext", BT_FN_STRING_CONST_STRING_CONST_STRING_INT, ATTR_FORMAT_ARG_2) DEF_EXT_LIB_BUILTIN (BUILT_IN_DGETTEXT, "dgettext", BT_FN_STRING_CONST_STRING_CONST_STRING, ATTR_FORMAT_ARG_2) DEF_EXT_LIB_BUILTIN (BUILT_IN_GETTEXT, "gettext", BT_FN_STRING_CONST_STRING, ATTR_FORMAT_ARG_1) DEF_EXT_LIB_BUILTIN (BUILT_IN_STRFMON, "strfmon", BT_FN_SSIZE_STRING_SIZE_CONST_STRING_VAR, ATTR_FORMAT_STRFMON_NOTHROW_3_4) DEF_LIB_BUILTIN (BUILT_IN_STRFTIME, "strftime", BT_FN_SIZE_STRING_SIZE_CONST_STRING_CONST_TM_PTR, ATTR_FORMAT_STRFTIME_NOTHROW_3_0) DEF_EXT_LIB_BUILTIN (BUILT_IN_SNPRINTF_CHK, "__snprintf_chk", BT_FN_INT_STRING_SIZE_INT_SIZE_CONST_STRING_VAR, ATTR_FORMAT_PRINTF_NOTHROW_5_6) DEF_EXT_LIB_BUILTIN (BUILT_IN_VSNPRINTF_CHK, "__vsnprintf_chk", BT_FN_INT_STRING_SIZE_INT_SIZE_CONST_STRING_VALIST_ARG, ATTR_FORMAT_PRINTF_NOTHROW_5_0) DEF_EXT_LIB_BUILTIN (BUILT_IN_PRINTF_CHK, "__printf_chk", BT_FN_INT_INT_CONST_STRING_VAR, ATTR_FORMAT_PRINTF_2_3) DEF_EXT_LIB_BUILTIN (BUILT_IN_VPRINTF_CHK, "__vprintf_chk", BT_FN_INT_INT_CONST_STRING_VALIST_ARG, ATTR_FORMAT_PRINTF_2_0) Martin ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [12/11/10] Fix invalid format warnings on Windows 2022-01-11 13:37 ` Martin Liška @ 2022-01-12 13:34 ` Tomas Kalibera 2022-01-13 9:40 ` Martin Liška 0 siblings, 1 reply; 10+ messages in thread From: Tomas Kalibera @ 2022-01-12 13:34 UTC (permalink / raw) To: Martin Liška, gcc-patches; +Cc: joseph, martin, redi [-- Attachment #1: Type: text/plain, Size: 4483 bytes --] On 1/11/22 2:37 PM, Martin Liška wrote: > Hello. > > I do support the patch, but I would ... Thanks, Martin, that makes the patch simpler and easier to maintain. Would the attached version do? Thanks Tomas > > On 1/7/22 19:33, Tomas Kalibera wrote: >> + if (is_attribute_p ("format", get_attribute_name (aa)) && >> + fndecl && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)) >> + { >> + switch (DECL_FUNCTION_CODE (fndecl)) >> + { >> + case BUILT_IN_FSCANF: >> + case BUILT_IN_PRINTF: >> + case BUILT_IN_SCANF: >> + case BUILT_IN_SNPRINTF: >> + case BUILT_IN_SSCANF: >> + case BUILT_IN_VFSCANF: >> + case BUILT_IN_VPRINTF: >> + case BUILT_IN_VSCANF: >> + case BUILT_IN_VSNPRINTF: >> + case BUILT_IN_VSSCANF: >> + case BUILT_IN_DCGETTEXT: >> + case BUILT_IN_DGETTEXT: >> + case BUILT_IN_GETTEXT: >> + case BUILT_IN_STRFMON: >> + case BUILT_IN_STRFTIME: >> + case BUILT_IN_SNPRINTF_CHK: >> + case BUILT_IN_VSNPRINTF_CHK: >> + case BUILT_IN_PRINTF_CHK: >> + case BUILT_IN_VPRINTF_CHK: >> + skipped_default_format = 1; >> + break; >> + default: >> + break; >> + } >> + } > > ... skip this as the listed functions are only these that have defined > ATTR_FORMAT_*: > > $ grep ATTR_FORMAT gcc/builtins.def > DEF_LIB_BUILTIN (BUILT_IN_FSCANF, "fscanf", > BT_FN_INT_FILEPTR_CONST_STRING_VAR, ATTR_FORMAT_SCANF_2_3) > DEF_LIB_BUILTIN (BUILT_IN_PRINTF, "printf", > BT_FN_INT_CONST_STRING_VAR, ATTR_FORMAT_PRINTF_1_2) > DEF_LIB_BUILTIN (BUILT_IN_SCANF, "scanf", > BT_FN_INT_CONST_STRING_VAR, ATTR_FORMAT_SCANF_1_2) > DEF_C99_BUILTIN (BUILT_IN_SNPRINTF, "snprintf", > BT_FN_INT_STRING_SIZE_CONST_STRING_VAR, ATTR_FORMAT_PRINTF_NOTHROW_3_4) > DEF_LIB_BUILTIN (BUILT_IN_SSCANF, "sscanf", > BT_FN_INT_CONST_STRING_CONST_STRING_VAR, ATTR_FORMAT_SCANF_NOTHROW_2_3) > DEF_C99_BUILTIN (BUILT_IN_VFSCANF, "vfscanf", > BT_FN_INT_FILEPTR_CONST_STRING_VALIST_ARG, ATTR_FORMAT_SCANF_2_0) > DEF_LIB_BUILTIN (BUILT_IN_VPRINTF, "vprintf", > BT_FN_INT_CONST_STRING_VALIST_ARG, ATTR_FORMAT_PRINTF_1_0) > DEF_C99_BUILTIN (BUILT_IN_VSCANF, "vscanf", > BT_FN_INT_CONST_STRING_VALIST_ARG, ATTR_FORMAT_SCANF_1_0) > DEF_C99_BUILTIN (BUILT_IN_VSNPRINTF, "vsnprintf", > BT_FN_INT_STRING_SIZE_CONST_STRING_VALIST_ARG, > ATTR_FORMAT_PRINTF_NOTHROW_3_0) > DEF_C99_BUILTIN (BUILT_IN_VSSCANF, "vsscanf", > BT_FN_INT_CONST_STRING_CONST_STRING_VALIST_ARG, > ATTR_FORMAT_SCANF_NOTHROW_2_0) > DEF_EXT_LIB_BUILTIN (BUILT_IN_DCGETTEXT, "dcgettext", > BT_FN_STRING_CONST_STRING_CONST_STRING_INT, ATTR_FORMAT_ARG_2) > DEF_EXT_LIB_BUILTIN (BUILT_IN_DGETTEXT, "dgettext", > BT_FN_STRING_CONST_STRING_CONST_STRING, ATTR_FORMAT_ARG_2) > DEF_EXT_LIB_BUILTIN (BUILT_IN_GETTEXT, "gettext", > BT_FN_STRING_CONST_STRING, ATTR_FORMAT_ARG_1) > DEF_EXT_LIB_BUILTIN (BUILT_IN_STRFMON, "strfmon", > BT_FN_SSIZE_STRING_SIZE_CONST_STRING_VAR, > ATTR_FORMAT_STRFMON_NOTHROW_3_4) > DEF_LIB_BUILTIN (BUILT_IN_STRFTIME, "strftime", > BT_FN_SIZE_STRING_SIZE_CONST_STRING_CONST_TM_PTR, > ATTR_FORMAT_STRFTIME_NOTHROW_3_0) > DEF_EXT_LIB_BUILTIN (BUILT_IN_SNPRINTF_CHK, "__snprintf_chk", > BT_FN_INT_STRING_SIZE_INT_SIZE_CONST_STRING_VAR, > ATTR_FORMAT_PRINTF_NOTHROW_5_6) > DEF_EXT_LIB_BUILTIN (BUILT_IN_VSNPRINTF_CHK, "__vsnprintf_chk", > BT_FN_INT_STRING_SIZE_INT_SIZE_CONST_STRING_VALIST_ARG, > ATTR_FORMAT_PRINTF_NOTHROW_5_0) > DEF_EXT_LIB_BUILTIN (BUILT_IN_PRINTF_CHK, "__printf_chk", > BT_FN_INT_INT_CONST_STRING_VAR, ATTR_FORMAT_PRINTF_2_3) > DEF_EXT_LIB_BUILTIN (BUILT_IN_VPRINTF_CHK, "__vprintf_chk", > BT_FN_INT_INT_CONST_STRING_VALIST_ARG, ATTR_FORMAT_PRINTF_2_0) > > Martin [-- Attachment #2: fix_invalid_format_warnings_on_windows.patch --] [-- Type: text/x-patch, Size: 4642 bytes --] From 82a659c7e5b24bbd39ac567dff3f79cc4c1e083f Mon Sep 17 00:00:00 2001 From: Tomas Kalibera <tomas.kalibera@gmail.com> Date: Wed, 12 Jan 2022 08:17:21 -0500 Subject: [PATCH] Mingw32 targets use ms_printf format for printf, but mingw-w64 when configured for UCRT uses gnu_format (via stdio.h). GCC then checks both formats, which means that one cannot print a 64-bit integer without a warning. All these lines issue a warning: printf("Hello %"PRIu64"\n", x); printf("Hello %I64u\n", x); printf("Hello %llu\n", x); because each of them violates one of the formats. Also, one gets a warning twice if the format string violates both formats. Fixed by disabling the built in format in case there are additional ones. gcc/c-family/ChangeLog: PR c/95130 PR c/92292 * c-common.c (check_function_arguments): Pass also function declaration to check_function_format. * c-common.h (check_function_format): Extra argument - function declaration. * c-format.c (check_function_format): For builtin functions with a built in format and at least one more, do not check the first one. --- gcc/c-family/c-common.c | 2 +- gcc/c-family/c-common.h | 2 +- gcc/c-family/c-format.c | 31 +++++++++++++++++++++++++++++-- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 4a6a4edb763..00fc734d28e 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -6064,7 +6064,7 @@ check_function_arguments (location_t loc, const_tree fndecl, const_tree fntype, /* Check for errors in format strings. */ if (warn_format || warn_suggest_attribute_format) - check_function_format (fntype, TYPE_ATTRIBUTES (fntype), nargs, argarray, + check_function_format (fndecl, fntype, TYPE_ATTRIBUTES (fntype), nargs, argarray, arglocs); if (warn_format) diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index 8b7bf35e888..ee370eafbbc 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -856,7 +856,7 @@ extern void check_function_arguments_recurse (void (*) unsigned HOST_WIDE_INT); extern bool check_builtin_function_arguments (location_t, vec<location_t>, tree, tree, int, tree *); -extern void check_function_format (const_tree, tree, int, tree *, +extern void check_function_format (const_tree, const_tree, tree, int, tree *, vec<location_t> *); extern bool attribute_fallthrough_p (tree); extern tree handle_format_attribute (tree *, tree, tree, int, bool *); diff --git a/gcc/c-family/c-format.c b/gcc/c-family/c-format.c index afa77810a5c..da72d85f66e 100644 --- a/gcc/c-family/c-format.c +++ b/gcc/c-family/c-format.c @@ -1160,12 +1160,13 @@ decode_format_type (const char *s, bool *is_raw /* = NULL */) attribute themselves. */ void -check_function_format (const_tree fntype, tree attrs, int nargs, +check_function_format (const_tree fndecl, const_tree fntype, tree attrs, int nargs, tree *argarray, vec<location_t> *arglocs) { - tree a; + tree a, aa; tree atname = get_identifier ("format"); + int skipped_default_format = 0; /* See if this function has any format attributes. */ for (a = attrs; a; a = TREE_CHAIN (a)) @@ -1176,6 +1177,32 @@ check_function_format (const_tree fntype, tree attrs, int nargs, function_format_info info; decode_format_attr (fntype, atname, TREE_VALUE (a), &info, /*validated=*/true); + + /* Mingw32 targets have traditionally used ms_printf format for the + printf function, and this format is built in GCC. But nowadays, + if mingw-w64 is configured to target UCRT, the printf function + uses the gnu_printf format (specified in the stdio.h header). This + causes GCC to check both formats, which means that there is no way + to e.g. print a long long unsigned without a warning (ms_printf + warns for %llu and gnu_printf warns for %I64u). Also, GCC would warn + twice about the same issue when both formats are violated, e.g. + for %lu used to print long long unsigned. + + Hence, if there are multiple format specifiers, we skip the first + one. See PR 95130, PR 92292. */ + + if (!skipped_default_format && fndecl) + { + for(aa = TREE_CHAIN (a); aa; aa = TREE_CHAIN(aa)) + if (is_attribute_p ("format", get_attribute_name (aa)) && + fndecl && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)) + { + skipped_default_format = 1; + break; + } + if (skipped_default_format) continue; + } + if (warn_format) { /* FIXME: Rewrite all the internal functions in this file -- 2.25.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [12/11/10] Fix invalid format warnings on Windows 2022-01-12 13:34 ` Tomas Kalibera @ 2022-01-13 9:40 ` Martin Liška 2022-01-13 11:00 ` Tomas Kalibera 0 siblings, 1 reply; 10+ messages in thread From: Martin Liška @ 2022-01-13 9:40 UTC (permalink / raw) To: Tomas Kalibera, gcc-patches; +Cc: joseph, martin, redi On 1/12/22 14:34, Tomas Kalibera wrote: > > On 1/11/22 2:37 PM, Martin Liška wrote: >> Hello. >> >> I do support the patch, but I would ... > > Thanks, Martin, that makes the patch simpler and easier to maintain. Would the attached version do? > > Thanks > Tomas > >> >> On 1/7/22 19:33, Tomas Kalibera wrote: >>> + if (is_attribute_p ("format", get_attribute_name (aa)) && >>> + fndecl && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)) >>> + { >>> + switch (DECL_FUNCTION_CODE (fndecl)) >>> + { >>> + case BUILT_IN_FSCANF: >>> + case BUILT_IN_PRINTF: >>> + case BUILT_IN_SCANF: >>> + case BUILT_IN_SNPRINTF: >>> + case BUILT_IN_SSCANF: >>> + case BUILT_IN_VFSCANF: >>> + case BUILT_IN_VPRINTF: >>> + case BUILT_IN_VSCANF: >>> + case BUILT_IN_VSNPRINTF: >>> + case BUILT_IN_VSSCANF: >>> + case BUILT_IN_DCGETTEXT: >>> + case BUILT_IN_DGETTEXT: >>> + case BUILT_IN_GETTEXT: >>> + case BUILT_IN_STRFMON: >>> + case BUILT_IN_STRFTIME: >>> + case BUILT_IN_SNPRINTF_CHK: >>> + case BUILT_IN_VSNPRINTF_CHK: >>> + case BUILT_IN_PRINTF_CHK: >>> + case BUILT_IN_VPRINTF_CHK: >>> + skipped_default_format = 1; >>> + break; >>> + default: >>> + break; >>> + } >>> + } >> >> ... skip this as the listed functions are only these that have defined ATTR_FORMAT_*: >> >> $ grep ATTR_FORMAT gcc/builtins.def >> DEF_LIB_BUILTIN (BUILT_IN_FSCANF, "fscanf", BT_FN_INT_FILEPTR_CONST_STRING_VAR, ATTR_FORMAT_SCANF_2_3) >> DEF_LIB_BUILTIN (BUILT_IN_PRINTF, "printf", BT_FN_INT_CONST_STRING_VAR, ATTR_FORMAT_PRINTF_1_2) >> DEF_LIB_BUILTIN (BUILT_IN_SCANF, "scanf", BT_FN_INT_CONST_STRING_VAR, ATTR_FORMAT_SCANF_1_2) >> DEF_C99_BUILTIN (BUILT_IN_SNPRINTF, "snprintf", BT_FN_INT_STRING_SIZE_CONST_STRING_VAR, ATTR_FORMAT_PRINTF_NOTHROW_3_4) >> DEF_LIB_BUILTIN (BUILT_IN_SSCANF, "sscanf", BT_FN_INT_CONST_STRING_CONST_STRING_VAR, ATTR_FORMAT_SCANF_NOTHROW_2_3) >> DEF_C99_BUILTIN (BUILT_IN_VFSCANF, "vfscanf", BT_FN_INT_FILEPTR_CONST_STRING_VALIST_ARG, ATTR_FORMAT_SCANF_2_0) >> DEF_LIB_BUILTIN (BUILT_IN_VPRINTF, "vprintf", BT_FN_INT_CONST_STRING_VALIST_ARG, ATTR_FORMAT_PRINTF_1_0) >> DEF_C99_BUILTIN (BUILT_IN_VSCANF, "vscanf", BT_FN_INT_CONST_STRING_VALIST_ARG, ATTR_FORMAT_SCANF_1_0) >> DEF_C99_BUILTIN (BUILT_IN_VSNPRINTF, "vsnprintf", BT_FN_INT_STRING_SIZE_CONST_STRING_VALIST_ARG, ATTR_FORMAT_PRINTF_NOTHROW_3_0) >> DEF_C99_BUILTIN (BUILT_IN_VSSCANF, "vsscanf", BT_FN_INT_CONST_STRING_CONST_STRING_VALIST_ARG, ATTR_FORMAT_SCANF_NOTHROW_2_0) >> DEF_EXT_LIB_BUILTIN (BUILT_IN_DCGETTEXT, "dcgettext", BT_FN_STRING_CONST_STRING_CONST_STRING_INT, ATTR_FORMAT_ARG_2) >> DEF_EXT_LIB_BUILTIN (BUILT_IN_DGETTEXT, "dgettext", BT_FN_STRING_CONST_STRING_CONST_STRING, ATTR_FORMAT_ARG_2) >> DEF_EXT_LIB_BUILTIN (BUILT_IN_GETTEXT, "gettext", BT_FN_STRING_CONST_STRING, ATTR_FORMAT_ARG_1) >> DEF_EXT_LIB_BUILTIN (BUILT_IN_STRFMON, "strfmon", BT_FN_SSIZE_STRING_SIZE_CONST_STRING_VAR, ATTR_FORMAT_STRFMON_NOTHROW_3_4) >> DEF_LIB_BUILTIN (BUILT_IN_STRFTIME, "strftime", BT_FN_SIZE_STRING_SIZE_CONST_STRING_CONST_TM_PTR, ATTR_FORMAT_STRFTIME_NOTHROW_3_0) >> DEF_EXT_LIB_BUILTIN (BUILT_IN_SNPRINTF_CHK, "__snprintf_chk", BT_FN_INT_STRING_SIZE_INT_SIZE_CONST_STRING_VAR, ATTR_FORMAT_PRINTF_NOTHROW_5_6) >> DEF_EXT_LIB_BUILTIN (BUILT_IN_VSNPRINTF_CHK, "__vsnprintf_chk", BT_FN_INT_STRING_SIZE_INT_SIZE_CONST_STRING_VALIST_ARG, ATTR_FORMAT_PRINTF_NOTHROW_5_0) >> DEF_EXT_LIB_BUILTIN (BUILT_IN_PRINTF_CHK, "__printf_chk", BT_FN_INT_INT_CONST_STRING_VAR, ATTR_FORMAT_PRINTF_2_3) >> DEF_EXT_LIB_BUILTIN (BUILT_IN_VPRINTF_CHK, "__vprintf_chk", BT_FN_INT_INT_CONST_STRING_VALIST_ARG, ATTR_FORMAT_PRINTF_2_0) >> >> Martin Few inline comments: > From 82a659c7e5b24bbd39ac567dff3f79cc4c1e083f Mon Sep 17 00:00:00 2001 > From: Tomas Kalibera <tomas.kalibera@gmail.com> > Date: Wed, 12 Jan 2022 08:17:21 -0500 > Subject: [PATCH] Mingw32 targets use ms_printf format for printf, but > mingw-w64 when configured for UCRT uses gnu_format (via stdio.h). GCC then > checks both formats, which means that one cannot print a 64-bit integer > without a warning. All these lines issue a warning: Please shorted the commit message's first line and put the rest to next lines. > > printf("Hello %"PRIu64"\n", x); > printf("Hello %I64u\n", x); > printf("Hello %llu\n", x); > > because each of them violates one of the formats. Also, one gets a warning > twice if the format string violates both formats. > > Fixed by disabling the built in format in case there are additional ones. > > gcc/c-family/ChangeLog: > > PR c/95130 > PR c/92292 > > * c-common.c (check_function_arguments): Pass also function > declaration to check_function_format. > > * c-common.h (check_function_format): Extra argument - function > declaration. > > * c-format.c (check_function_format): For builtin functions with a > built in format and at least one more, do not check the first one. > --- > gcc/c-family/c-common.c | 2 +- > gcc/c-family/c-common.h | 2 +- > gcc/c-family/c-format.c | 31 +++++++++++++++++++++++++++++-- > 3 files changed, 31 insertions(+), 4 deletions(-) > > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c > index 4a6a4edb763..00fc734d28e 100644 > --- a/gcc/c-family/c-common.c > +++ b/gcc/c-family/c-common.c > @@ -6064,7 +6064,7 @@ check_function_arguments (location_t loc, const_tree fndecl, const_tree fntype, > /* Check for errors in format strings. */ > > if (warn_format || warn_suggest_attribute_format) > - check_function_format (fntype, TYPE_ATTRIBUTES (fntype), nargs, argarray, > + check_function_format (fndecl, fntype, TYPE_ATTRIBUTES (fntype), nargs, argarray, > arglocs); > > if (warn_format) > diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h > index 8b7bf35e888..ee370eafbbc 100644 > --- a/gcc/c-family/c-common.h > +++ b/gcc/c-family/c-common.h > @@ -856,7 +856,7 @@ extern void check_function_arguments_recurse (void (*) > unsigned HOST_WIDE_INT); > extern bool check_builtin_function_arguments (location_t, vec<location_t>, > tree, tree, int, tree *); > -extern void check_function_format (const_tree, tree, int, tree *, > +extern void check_function_format (const_tree, const_tree, tree, int, tree *, > vec<location_t> *); > extern bool attribute_fallthrough_p (tree); > extern tree handle_format_attribute (tree *, tree, tree, int, bool *); > diff --git a/gcc/c-family/c-format.c b/gcc/c-family/c-format.c > index afa77810a5c..da72d85f66e 100644 > --- a/gcc/c-family/c-format.c > +++ b/gcc/c-family/c-format.c > @@ -1160,12 +1160,13 @@ decode_format_type (const char *s, bool *is_raw /* = NULL */) > attribute themselves. */ > > void > -check_function_format (const_tree fntype, tree attrs, int nargs, > +check_function_format (const_tree fndecl, const_tree fntype, tree attrs, int nargs, > tree *argarray, vec<location_t> *arglocs) > { > - tree a; > + tree a, aa; > > tree atname = get_identifier ("format"); > + int skipped_default_format = 0; Use bool type: bool skipped_default_format = false; > > /* See if this function has any format attributes. */ > for (a = attrs; a; a = TREE_CHAIN (a)) > @@ -1176,6 +1177,32 @@ check_function_format (const_tree fntype, tree attrs, int nargs, > function_format_info info; > decode_format_attr (fntype, atname, TREE_VALUE (a), &info, > /*validated=*/true); > + > + /* Mingw32 targets have traditionally used ms_printf format for the > + printf function, and this format is built in GCC. But nowadays, > + if mingw-w64 is configured to target UCRT, the printf function > + uses the gnu_printf format (specified in the stdio.h header). This > + causes GCC to check both formats, which means that there is no way > + to e.g. print a long long unsigned without a warning (ms_printf > + warns for %llu and gnu_printf warns for %I64u). Also, GCC would warn > + twice about the same issue when both formats are violated, e.g. > + for %lu used to print long long unsigned. > + > + Hence, if there are multiple format specifiers, we skip the first > + one. See PR 95130, PR 92292. */ > + > + if (!skipped_default_format && fndecl) > + { > + for(aa = TREE_CHAIN (a); aa; aa = TREE_CHAIN(aa)) > + if (is_attribute_p ("format", get_attribute_name (aa)) && > + fndecl && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)) > + { > + skipped_default_format = 1; > + break; > + } > + if (skipped_default_format) continue; continue on next line please. Apart from that, I support the patch (I cannot approve it). Note we're now approaching stage4 and this is definitelly a stage1 material (opens after GCC 12.1.0 gets released). Cheers, Martin > + } > + > if (warn_format) > { > /* FIXME: Rewrite all the internal functions in this file > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [12/11/10] Fix invalid format warnings on Windows 2022-01-13 9:40 ` Martin Liška @ 2022-01-13 11:00 ` Tomas Kalibera 2022-05-11 8:21 ` Martin Liška 0 siblings, 1 reply; 10+ messages in thread From: Tomas Kalibera @ 2022-01-13 11:00 UTC (permalink / raw) To: Martin Liška, gcc-patches; +Cc: joseph, martin, redi [-- Attachment #1: Type: text/plain, Size: 342 bytes --] On 1/13/22 10:40 AM, Martin Liška wrote: [...] > Apart from that, I support the patch (I cannot approve it). Note we're > now approaching > stage4 and this is definitelly a stage1 material (opens after GCC > 12.1.0 gets released). Thanks, Martin, I've updated the patch following your suggestions. Cheers Tomas > > Cheers, > Martin > [-- Attachment #2: 0001-c-family-Let-stdio.h-override-built-in-printf-format.patch --] [-- Type: text/x-patch, Size: 4727 bytes --] From 4db4e6b35be5793902d8820d2c8e4d1f1cbba80d Mon Sep 17 00:00:00 2001 From: Tomas Kalibera <tomas.kalibera@gmail.com> Date: Thu, 13 Jan 2022 05:25:32 -0500 Subject: [PATCH] c-family: Let stdio.h override built in printf format [PR95130,PR92292] Mingw32 targets use ms_printf format for printf, but mingw-w64 when configured for UCRT uses gnu_format (via stdio.h). GCC then checks both formats, which means that one cannot print a 64-bit integer without a warning. All these lines issue a warning: printf("Hello %"PRIu64"\n", x); printf("Hello %I64u\n", x); printf("Hello %llu\n", x); because each of them violates one of the formats. Also, one gets a warning twice if the format string violates both formats. Fixed by disabling the built in format in case there are additional ones. gcc/c-family/ChangeLog: PR c/95130 PR c/92292 * c-common.c (check_function_arguments): Pass also function declaration to check_function_format. * c-common.h (check_function_format): Extra argument - function declaration. * c-format.c (check_function_format): For builtin functions with a built in format and at least one more, do not check the first one. --- gcc/c-family/c-common.c | 2 +- gcc/c-family/c-common.h | 2 +- gcc/c-family/c-format.c | 32 ++++++++++++++++++++++++++++++-- 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 4a6a4edb763..00fc734d28e 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -6064,7 +6064,7 @@ check_function_arguments (location_t loc, const_tree fndecl, const_tree fntype, /* Check for errors in format strings. */ if (warn_format || warn_suggest_attribute_format) - check_function_format (fntype, TYPE_ATTRIBUTES (fntype), nargs, argarray, + check_function_format (fndecl, fntype, TYPE_ATTRIBUTES (fntype), nargs, argarray, arglocs); if (warn_format) diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index 8b7bf35e888..ee370eafbbc 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -856,7 +856,7 @@ extern void check_function_arguments_recurse (void (*) unsigned HOST_WIDE_INT); extern bool check_builtin_function_arguments (location_t, vec<location_t>, tree, tree, int, tree *); -extern void check_function_format (const_tree, tree, int, tree *, +extern void check_function_format (const_tree, const_tree, tree, int, tree *, vec<location_t> *); extern bool attribute_fallthrough_p (tree); extern tree handle_format_attribute (tree *, tree, tree, int, bool *); diff --git a/gcc/c-family/c-format.c b/gcc/c-family/c-format.c index afa77810a5c..bc2abee5146 100644 --- a/gcc/c-family/c-format.c +++ b/gcc/c-family/c-format.c @@ -1160,12 +1160,13 @@ decode_format_type (const char *s, bool *is_raw /* = NULL */) attribute themselves. */ void -check_function_format (const_tree fntype, tree attrs, int nargs, +check_function_format (const_tree fndecl, const_tree fntype, tree attrs, int nargs, tree *argarray, vec<location_t> *arglocs) { - tree a; + tree a, aa; tree atname = get_identifier ("format"); + bool skipped_default_format = false; /* See if this function has any format attributes. */ for (a = attrs; a; a = TREE_CHAIN (a)) @@ -1176,6 +1177,33 @@ check_function_format (const_tree fntype, tree attrs, int nargs, function_format_info info; decode_format_attr (fntype, atname, TREE_VALUE (a), &info, /*validated=*/true); + + /* Mingw32 targets have traditionally used ms_printf format for the + printf function, and this format is built in GCC. But nowadays, + if mingw-w64 is configured to target UCRT, the printf function + uses the gnu_printf format (specified in the stdio.h header). This + causes GCC to check both formats, which means that there is no way + to e.g. print a long long unsigned without a warning (ms_printf + warns for %llu and gnu_printf warns for %I64u). Also, GCC would warn + twice about the same issue when both formats are violated, e.g. + for %lu used to print long long unsigned. + + Hence, if there are multiple format specifiers, we skip the first + one. See PR 95130, PR 92292. */ + + if (!skipped_default_format && fndecl) + { + for(aa = TREE_CHAIN (a); aa; aa = TREE_CHAIN(aa)) + if (is_attribute_p ("format", get_attribute_name (aa)) && + fndecl && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)) + { + skipped_default_format = true; + break; + } + if (skipped_default_format) + continue; + } + if (warn_format) { /* FIXME: Rewrite all the internal functions in this file -- 2.25.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [12/11/10] Fix invalid format warnings on Windows 2022-01-13 11:00 ` Tomas Kalibera @ 2022-05-11 8:21 ` Martin Liška 2022-05-11 16:43 ` Joseph Myers 0 siblings, 1 reply; 10+ messages in thread From: Martin Liška @ 2022-05-11 8:21 UTC (permalink / raw) To: Tomas Kalibera, gcc-patches Cc: joseph, martin, redi, Marek Polacek, Jason Merrill On 1/13/22 12:00, Tomas Kalibera wrote: > On 1/13/22 10:40 AM, Martin Liška wrote: > > [...] >> Apart from that, I support the patch (I cannot approve it). Note we're now approaching >> stage4 and this is definitelly a stage1 material (opens after GCC 12.1.0 gets released). > > Thanks, Martin, I've updated the patch following your suggestions. > > Cheers > Tomas > > >> >> Cheers, >> Martin >> May I please ping review for this? Cheers, Martin ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [12/11/10] Fix invalid format warnings on Windows 2022-05-11 8:21 ` Martin Liška @ 2022-05-11 16:43 ` Joseph Myers 2022-05-12 15:19 ` Martin Storsjö 2022-05-16 11:27 ` Tomas Kalibera 0 siblings, 2 replies; 10+ messages in thread From: Joseph Myers @ 2022-05-11 16:43 UTC (permalink / raw) To: Martin Liška; +Cc: Tomas Kalibera, gcc-patches, Marek Polacek, redi On Wed, 11 May 2022, Martin Liška wrote: > May I please ping review for this? There are various coding style issues in the patch; at least missing space before '(' and '&&' at end of line (should be at start of line). It will also need to be updated for .c files having been renamed to .cc in the GCC source tree. I'd also like to check that "if mingw-w64 is configured to target UCRT" is not something that is necessarily known when GCC is built or from the command-line options passed to GCC. Because ideally one might expect the TARGET_OVERRIDES_FORMAT_ATTRIBUTES / TARGET_OVERRIDES_FORMAT_INIT definitions to handle things appropriately conditionally, so that printf attributes are handled as gnu_printf for the "if mingw-w64 is configured to target UCRT" case. Disregarding a built-in format attribute when one is also specified explicitly in the header, even though the two are not exactly equivalent attributes, as in this patch, seems more like the right approach in the case where the attributes in installed header (at the time GCC is run, not the time it is built) *are* the way in which GCC gets the "configured to target UCRT" information - as opposed to it being something available before the header is parsed. -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [12/11/10] Fix invalid format warnings on Windows 2022-05-11 16:43 ` Joseph Myers @ 2022-05-12 15:19 ` Martin Storsjö 2022-05-16 11:27 ` Tomas Kalibera 1 sibling, 0 replies; 10+ messages in thread From: Martin Storsjö @ 2022-05-12 15:19 UTC (permalink / raw) To: Joseph Myers; +Cc: Martin Liška, Marek Polacek, gcc-patches, redi On Wed, 11 May 2022, Joseph Myers wrote: > I'd also like to check that "if mingw-w64 is configured to target UCRT" is > not something that is necessarily known when GCC is built or from the > command-line options passed to GCC. Because ideally one might expect the > TARGET_OVERRIDES_FORMAT_ATTRIBUTES / TARGET_OVERRIDES_FORMAT_INIT > definitions to handle things appropriately conditionally, so that printf > attributes are handled as gnu_printf for the "if mingw-w64 is configured > to target UCRT" case. Disregarding a built-in format attribute when one > is also specified explicitly in the header, even though the two are not > exactly equivalent attributes, as in this patch, seems more like the right > approach in the case where the attributes in installed header (at the time > GCC is run, not the time it is built) *are* the way in which GCC gets the > "configured to target UCRT" information - as opposed to it being something > available before the header is parsed. Indeed, the "configured to target X" information isn't available at the point when GCC is built - that gets set afterwards. And even while it is usually mostly fixed soon afterwards, you can even (with some amount of gotchas) change what CRT to build for by passing the appropriate defines that picks a different default in the mingw-w64 headers. Anyway - from the GCC point of view, it's not fixed, and whatever the parsed headers say, is the only thing that can be relied upon. So I think the approach of the patch is the right one, code style/issues notwithstanding. // Martin ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [12/11/10] Fix invalid format warnings on Windows 2022-05-11 16:43 ` Joseph Myers 2022-05-12 15:19 ` Martin Storsjö @ 2022-05-16 11:27 ` Tomas Kalibera 2022-07-04 16:40 ` Jeff Law 1 sibling, 1 reply; 10+ messages in thread From: Tomas Kalibera @ 2022-05-16 11:27 UTC (permalink / raw) To: Joseph Myers, Martin Liška Cc: gcc-patches, Marek Polacek, redi, Martin Storsjö [-- Attachment #1: Type: text/plain, Size: 1366 bytes --] On 5/11/22 18:43, Joseph Myers wrote: > There are various coding style issues in the patch; at least missing space > before '(' and '&&' at end of line (should be at start of line). It will > also need to be updated for .c files having been renamed to .cc in the GCC > source tree. Thanks, I've fixed the formatting issue and updated the patch for master, 12, 11 and 10. In addition to the renaming of .c to .cc files, there was also a change in the first argument of check_function_format. I've also removed a duplicated check for whether fndecl was null and fixed indentation. I've updated the patches for each version to also note that in c51f1e7427e6a5ae2a6d82b5a790df77a3adc99a gcc: Add `ll` and `L` length modifiers for `ms_printf` the ms_printf format has been taught to support (not warn about) printing the 64-bit integers using the "%ll" modifier (currently GCC 11 and newer). However, I assume there may be other differences between the ms_printf and gnu_printf formats people might run into, so it still makes sense to fix this not only in GCC 10, but also in newer versions. Furthermore, the attached patch is still needed (GCC 11, GCC 12, master) to get rid of duplicate warnings for an incorrect format (e.g. "%lu" used to print "unsigned long long"), when both ms_printf and gnu_printf formats are violated (PR 92292). Best Tomas [-- Attachment #2: master_0001-c-family-Let-stdio.h-override-built-in-printf-format.patch --] [-- Type: text/x-patch, Size: 2991 bytes --] From 7d72c9cc395df56ce044f4a0b0b77c151bfe2bf6 Mon Sep 17 00:00:00 2001 From: Tomas Kalibera <tomas.kalibera@gmail.com> Date: Mon, 16 May 2022 06:43:09 -0400 Subject: [PATCH] c-family: Let stdio.h override built in printf format [PR95130,PR92292] Mingw32 targets use ms_printf format for printf, but mingw-w64 when configured for UCRT uses gnu_format (via stdio.h). GCC checks both formats, which means that one gets a warning twice if the format string violates both formats: printf("Hello %lu\n", (long long unsigned) x); Fixed by disabling the built in format in case there are additional ones. This fixes also prevents issues if the print formats disagree. In the past it was the case when printing 64-bit integers, but GCC ms_printf format since c51f1e7427e6a5ae2a6d82b5a790df77a3adc99 supports %llu. gcc/c-family/ChangeLog: PR c/95130 PR c/92292 * c-format.cc (check_function_format): For builtin functions with a built in format and at least one more, do not check the first one. --- gcc/c-family/c-format.cc | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/gcc/c-family/c-format.cc b/gcc/c-family/c-format.cc index ea57fde801c..7590b467e81 100644 --- a/gcc/c-family/c-format.cc +++ b/gcc/c-family/c-format.cc @@ -1157,9 +1157,10 @@ void check_function_format (const_tree fn, tree attrs, int nargs, tree *argarray, vec<location_t> *arglocs) { - tree a; + tree a, aa; tree atname = get_identifier ("format"); + bool skipped_default_format = false; /* See if this function has any format attributes. */ for (a = attrs; a; a = TREE_CHAIN (a)) @@ -1170,6 +1171,32 @@ check_function_format (const_tree fn, tree attrs, int nargs, function_format_info info; decode_format_attr (fn, atname, TREE_VALUE (a), &info, /*validated=*/true); + + /* Mingw32 targets have traditionally used ms_printf format for the + printf function, and this format is built in GCC. But nowadays, + if mingw-w64 is configured to target UCRT, the printf function + uses the gnu_printf format (specified in the stdio.h header). This + causes GCC to check both formats, which means that GCC would + warn twice about the same issue when both formats are violated, + e.g. for %lu used to print long long unsigned. + + Hence, if there are multiple format specifiers, we skip the first + one. See PR 95130 (but note that GCC ms_printf already supports + %llu) and PR 92292. */ + + if (!skipped_default_format && fn && TREE_CODE (fn) == FUNCTION_DECL) + { + for(aa = TREE_CHAIN (a); aa; aa = TREE_CHAIN (aa)) + if (is_attribute_p ("format", get_attribute_name (aa)) + && fndecl_built_in_p (fn, BUILT_IN_NORMAL)) + { + skipped_default_format = true; + break; + } + if (skipped_default_format) + continue; + } + if (warn_format) { /* FIXME: Rewrite all the internal functions in this file -- 2.25.1 [-- Attachment #3: 12_0001-c-family-Let-stdio.h-override-built-in-printf-format.patch --] [-- Type: text/x-patch, Size: 4735 bytes --] From d64309760bc7f61db10a7f28baf3308d871ef1ed Mon Sep 17 00:00:00 2001 From: Tomas Kalibera <tomas.kalibera@gmail.com> Date: Mon, 16 May 2022 06:16:55 -0400 Subject: [PATCH] c-family: Let stdio.h override built in printf format [PR95130,PR92292] Mingw32 targets use ms_printf format for printf, but mingw-w64 when configured for UCRT uses gnu_format (via stdio.h). GCC checks both formats, which means that one gets a warning twice if the format string violates both formats: printf("Hello %lu\n", (long long unsigned) x); Fixed by disabling the built in format in case there are additional ones. This fixes also prevents issues if the print formats disagree. In the past it was the case when printing 64-bit integers, but GCC ms_printf format since c51f1e7427e6a5ae2a6d82b5a790df77a3adc99 supports %llu. gcc/c-family/ChangeLog: PR c/95130 PR c/92292 * c-common.cc (check_function_arguments): Pass also function declaration to check_function_format. * c-common.h (check_function_format): Extra argument - function declaration. * c-format.cc (check_function_format): For builtin functions with a built in format and at least one more, do not check the first one. --- gcc/c-family/c-common.cc | 2 +- gcc/c-family/c-common.h | 2 +- gcc/c-family/c-format.cc | 32 ++++++++++++++++++++++++++++++-- 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc index bb0544eeaea..a063468f26d 100644 --- a/gcc/c-family/c-common.cc +++ b/gcc/c-family/c-common.cc @@ -6071,7 +6071,7 @@ check_function_arguments (location_t loc, const_tree fndecl, const_tree fntype, /* Check for errors in format strings. */ if (warn_format || warn_suggest_attribute_format) - check_function_format (fntype, TYPE_ATTRIBUTES (fntype), nargs, argarray, + check_function_format (fndecl, fntype, TYPE_ATTRIBUTES (fntype), nargs, argarray, arglocs); if (warn_format) diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index 52a85bfb783..7b8c87bec19 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -857,7 +857,7 @@ extern void check_function_arguments_recurse (void (*) opt_code); extern bool check_builtin_function_arguments (location_t, vec<location_t>, tree, tree, int, tree *); -extern void check_function_format (const_tree, tree, int, tree *, +extern void check_function_format (const_tree, const_tree, tree, int, tree *, vec<location_t> *); extern bool attribute_fallthrough_p (tree); extern tree handle_format_attribute (tree *, tree, tree, int, bool *); diff --git a/gcc/c-family/c-format.cc b/gcc/c-family/c-format.cc index 98f28c0dcc6..55948915e44 100644 --- a/gcc/c-family/c-format.cc +++ b/gcc/c-family/c-format.cc @@ -1160,12 +1160,13 @@ decode_format_type (const char *s, bool *is_raw /* = NULL */) attribute themselves. */ void -check_function_format (const_tree fntype, tree attrs, int nargs, +check_function_format (const_tree fndecl, const_tree fntype, tree attrs, int nargs, tree *argarray, vec<location_t> *arglocs) { - tree a; + tree a, aa; tree atname = get_identifier ("format"); + bool skipped_default_format = false; /* See if this function has any format attributes. */ for (a = attrs; a; a = TREE_CHAIN (a)) @@ -1176,6 +1177,33 @@ check_function_format (const_tree fntype, tree attrs, int nargs, function_format_info info; decode_format_attr (fntype, atname, TREE_VALUE (a), &info, /*validated=*/true); + + /* Mingw32 targets have traditionally used ms_printf format for the + printf function, and this format is built in GCC. But nowadays, + if mingw-w64 is configured to target UCRT, the printf function + uses the gnu_printf format (specified in the stdio.h header). This + causes GCC to check both formats, which means that GCC would warn + twice about the same issue when both formats are violated, e.g. + for %lu used to print long long unsigned. Also, it would be + impossible to use features permitted by only one format. + + Hence, if there are multiple format specifiers, we skip the first + one. See PR 95130 (but note that GCC ms_printf already supports + %llu) and PR 92292. */ + + if (!skipped_default_format && fndecl) + { + for(aa = TREE_CHAIN (a); aa; aa = TREE_CHAIN (aa)) + if (is_attribute_p ("format", get_attribute_name (aa)) + && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)) + { + skipped_default_format = true; + break; + } + if (skipped_default_format) + continue; + } + if (warn_format) { /* FIXME: Rewrite all the internal functions in this file -- 2.25.1 [-- Attachment #4: 11_0001-c-family-Let-stdio.h-override-built-in-printf-format.patch --] [-- Type: text/x-patch, Size: 4737 bytes --] From f3802d8ef93f754b899a57a33afe720db21e4013 Mon Sep 17 00:00:00 2001 From: Tomas Kalibera <tomas.kalibera@gmail.com> Date: Mon, 16 May 2022 05:55:29 -0400 Subject: [PATCH] c-family: Let stdio.h override built in printf format [PR95130,PR92292] Mingw32 targets use ms_printf format for printf, but mingw-w64 when configured for UCRT uses gnu_format (via stdio.h). GCC checks both formats, which means that one gets a warning twice if the format string violates both formats: printf("Hello %lu\n", (long long unsigned) x); Fixed by disabling the built in format in case there are additional ones. This fixes also prevents issues if the print formats disagree. In the past it was the case when printing 64-bit integers, but GCC ms_printf format since c51f1e7427e6a5ae2a6d82b5a790df77a3adc99 supports %llu. gcc/c-family/ChangeLog: PR c/95130 PR c/92292 * c-common.c (check_function_arguments): Pass also function declaration to check_function_format. * c-common.h (check_function_format): Extra argument - function declaration. * c-format.c (check_function_format): For builtin functions with a built in format and at least one more, do not check the first one. --- gcc/c-family/c-common.c | 2 +- gcc/c-family/c-common.h | 2 +- gcc/c-family/c-format.c | 32 ++++++++++++++++++++++++++++++-- 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 16fc52302e5..5a8bcb6774f 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -5849,7 +5849,7 @@ check_function_arguments (location_t loc, const_tree fndecl, const_tree fntype, /* Check for errors in format strings. */ if (warn_format || warn_suggest_attribute_format) - check_function_format (fntype, TYPE_ATTRIBUTES (fntype), nargs, argarray, + check_function_format (fndecl, fntype, TYPE_ATTRIBUTES (fntype), nargs, argarray, arglocs); if (warn_format) diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index f30b6c6ac33..34d936e7b48 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -854,7 +854,7 @@ extern void check_function_arguments_recurse (void (*) unsigned HOST_WIDE_INT); extern bool check_builtin_function_arguments (location_t, vec<location_t>, tree, tree, int, tree *); -extern void check_function_format (const_tree, tree, int, tree *, +extern void check_function_format (const_tree, const_tree, tree, int, tree *, vec<location_t> *); extern bool attribute_fallthrough_p (tree); extern tree handle_format_attribute (tree *, tree, tree, int, bool *); diff --git a/gcc/c-family/c-format.c b/gcc/c-family/c-format.c index f4359657fc1..46a85bae712 100644 --- a/gcc/c-family/c-format.c +++ b/gcc/c-family/c-format.c @@ -1168,12 +1168,13 @@ decode_format_type (const char *s, bool *is_raw /* = NULL */) attribute themselves. */ void -check_function_format (const_tree fntype, tree attrs, int nargs, +check_function_format (const_tree fndecl, const_tree fntype, tree attrs, int nargs, tree *argarray, vec<location_t> *arglocs) { - tree a; + tree a, aa; tree atname = get_identifier ("format"); + bool skipped_default_format = false; /* See if this function has any format attributes. */ for (a = attrs; a; a = TREE_CHAIN (a)) @@ -1184,6 +1185,33 @@ check_function_format (const_tree fntype, tree attrs, int nargs, function_format_info info; decode_format_attr (fntype, atname, TREE_VALUE (a), &info, /*validated=*/true); + + /* Mingw32 targets have traditionally used ms_printf format for the + printf function, and this format is built in GCC. But nowadays, + if mingw-w64 is configured to target UCRT, the printf function + uses the gnu_printf format (specified in the stdio.h header). This + causes GCC to check both formats, which means that GCC would warn + twice about the same issue when both formats are violated, e.g. + for %lu used to print long long unsigned. Also, it would be + impossible to use features permitted by only one format. + + Hence, if there are multiple format specifiers, we skip the first + one. See PR 95130 (but note that GCC ms_printf already supports + %llu) and PR 92292. */ + + if (!skipped_default_format && fndecl) + { + for(aa = TREE_CHAIN (a); aa; aa = TREE_CHAIN (aa)) + if (is_attribute_p ("format", get_attribute_name (aa)) + && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)) + { + skipped_default_format = true; + break; + } + if (skipped_default_format) + continue; + } + if (warn_format) { /* FIXME: Rewrite all the internal functions in this file -- 2.25.1 [-- Attachment #5: 10_0001-c-family-Let-stdio.h-override-built-in-printf-format.patch --] [-- Type: text/x-patch, Size: 4822 bytes --] From 65a891fa7f96f95c4544a36a2dcc144fb4fa8e8c Mon Sep 17 00:00:00 2001 From: Tomas Kalibera <tomas.kalibera@gmail.com> Date: Mon, 16 May 2022 05:25:39 -0400 Subject: [PATCH] c-family: Let stdio.h override built in printf format [PR95130,PR92292] Mingw32 targets use ms_printf format for printf, but mingw-w64 when configured for UCRT uses gnu_format (via stdio.h). GCC then checks both formats, which means that one cannot print a 64-bit integer without a warning. All these lines issue a warning: printf("Hello %"PRIu64"\n", (uint64_t) x); printf("Hello %I64u\n", (long long unsigned) x); printf("Hello %llu\n", (long long unsigned) x); because each of them violates one of the formats. One gets a warning twice if the format string violates both formats: printf("Hello %lu\n", (long long unsigned) x); Fixed by disabling the built in format in case there are additional ones. gcc/c-family/ChangeLog: PR c/95130 PR c/92292 * c-common.c (check_function_arguments): Pass also function declaration to check_function_format. * c-common.h (check_function_format): Extra argument - function declaration. * c-format.c (check_function_format): For builtin functions with a built in format and at least one more, do not check the first one. --- gcc/c-family/c-common.c | 2 +- gcc/c-family/c-common.h | 2 +- gcc/c-family/c-format.c | 32 ++++++++++++++++++++++++++++++-- 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 8105a27ab56..5ddf2be5abf 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -5733,7 +5733,7 @@ check_function_arguments (location_t loc, const_tree fndecl, const_tree fntype, /* Check for errors in format strings. */ if (warn_format || warn_suggest_attribute_format) - check_function_format (fntype, TYPE_ATTRIBUTES (fntype), nargs, argarray, + check_function_format (fndecl, fntype, TYPE_ATTRIBUTES (fntype), nargs, argarray, arglocs); if (warn_format) diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index ed39b7764bf..61037b3b0df 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -835,7 +835,7 @@ extern void check_function_arguments_recurse (void (*) unsigned HOST_WIDE_INT); extern bool check_builtin_function_arguments (location_t, vec<location_t>, tree, tree, int, tree *); -extern void check_function_format (const_tree, tree, int, tree *, +extern void check_function_format (const_tree, const_tree, tree, int, tree *, vec<location_t> *); extern bool attribute_fallthrough_p (tree); extern tree handle_format_attribute (tree *, tree, tree, int, bool *); diff --git a/gcc/c-family/c-format.c b/gcc/c-family/c-format.c index 33a5b6d3965..04299e0794e 100644 --- a/gcc/c-family/c-format.c +++ b/gcc/c-family/c-format.c @@ -1168,12 +1168,13 @@ decode_format_type (const char *s, bool *is_raw /* = NULL */) attribute themselves. */ void -check_function_format (const_tree fntype, tree attrs, int nargs, +check_function_format (const_tree fndecl, const_tree fntype, tree attrs, int nargs, tree *argarray, vec<location_t> *arglocs) { - tree a; + tree a, aa; tree atname = get_identifier ("format"); + bool skipped_default_format = false; /* See if this function has any format attributes. */ for (a = attrs; a; a = TREE_CHAIN (a)) @@ -1184,6 +1185,33 @@ check_function_format (const_tree fntype, tree attrs, int nargs, function_format_info info; decode_format_attr (fntype, atname, TREE_VALUE (a), &info, /*validated=*/true); + + /* Mingw32 targets have traditionally used ms_printf format for the + printf function, and this format is built in GCC. But nowadays, + if mingw-w64 is configured to target UCRT, the printf function + uses the gnu_printf format (specified in the stdio.h header). This + causes GCC to check both formats, which means that there is no way + to e.g. print a long long unsigned without a warning (ms_printf + warns for %llu and gnu_printf warns for %I64u). Also, GCC would warn + twice about the same issue when both formats are violated, e.g. + for %lu used to print long long unsigned. + + Hence, if there are multiple format specifiers, we skip the first + one. See PR 95130, PR 92292. */ + + if (!skipped_default_format && fndecl) + { + for(aa = TREE_CHAIN (a); aa; aa = TREE_CHAIN (aa)) + if (is_attribute_p ("format", get_attribute_name (aa)) + && fndecl_built_in_p (fndecl, BUILT_IN_NORMAL)) + { + skipped_default_format = true; + break; + } + if (skipped_default_format) + continue; + } + if (warn_format) { /* FIXME: Rewrite all the internal functions in this file -- 2.25.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [12/11/10] Fix invalid format warnings on Windows 2022-05-16 11:27 ` Tomas Kalibera @ 2022-07-04 16:40 ` Jeff Law 0 siblings, 0 replies; 10+ messages in thread From: Jeff Law @ 2022-07-04 16:40 UTC (permalink / raw) To: gcc-patches On 5/16/2022 5:27 AM, Tomas Kalibera via Gcc-patches wrote: > > On 5/11/22 18:43, Joseph Myers wrote: >> There are various coding style issues in the patch; at least missing >> space >> before '(' and '&&' at end of line (should be at start of line). It >> will >> also need to be updated for .c files having been renamed to .cc in >> the GCC >> source tree. > > Thanks, I've fixed the formatting issue and updated the patch for > master, 12, 11 and 10. In addition to the renaming of .c to .cc files, > there was also a change in the first argument of > check_function_format. I've also removed a duplicated check for > whether fndecl was null and fixed indentation. > > I've updated the patches for each version to also note that in > > c51f1e7427e6a5ae2a6d82b5a790df77a3adc99a > gcc: Add `ll` and `L` length modifiers for `ms_printf` > > the ms_printf format has been taught to support (not warn about) > printing the 64-bit integers using the "%ll" modifier (currently GCC > 11 and newer). However, I assume there may be other differences > between the ms_printf and gnu_printf formats people might run into, so > it still makes sense to fix this not only in GCC 10, but also in newer > versions. > > Furthermore, the attached patch is still needed (GCC 11, GCC 12, > master) to get rid of duplicate warnings for an incorrect format (e.g. > "%lu" used to print "unsigned long long"), when both ms_printf and > gnu_printf formats are violated (PR 92292). I guess we're going to depend on the builtin-format always appearing first in the chain? While it's probably true in practice, I doubt we really want to depend on that. Is there any sensible way to distinguish between the builtin format and one that comes from the source? There's a trivial formatting nit: > + for(aa = TREE_CHAIN (a); aa; aa = TREE_CHAIN (aa)) Space between the "for" and its open paren. But I think the big question here is whether or not we want to assume the builtin format is always first on the chain. jeff ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-07-04 16:40 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-07 18:33 [PATCH] [12/11/10] Fix invalid format warnings on Windows Tomas Kalibera 2022-01-11 13:37 ` Martin Liška 2022-01-12 13:34 ` Tomas Kalibera 2022-01-13 9:40 ` Martin Liška 2022-01-13 11:00 ` Tomas Kalibera 2022-05-11 8:21 ` Martin Liška 2022-05-11 16:43 ` Joseph Myers 2022-05-12 15:19 ` Martin Storsjö 2022-05-16 11:27 ` Tomas Kalibera 2022-07-04 16:40 ` Jeff Law
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).