From: "Martin Liška" <mliska@suse.cz>
To: Tomas Kalibera <tomas.kalibera@gmail.com>, gcc-patches@gcc.gnu.org
Cc: joseph@codesourcery.com, martin@martin.st, redi@gcc.gnu.org
Subject: Re: [PATCH] [12/11/10] Fix invalid format warnings on Windows
Date: Thu, 13 Jan 2022 10:40:52 +0100 [thread overview]
Message-ID: <c66ef9b8-aef4-4654-5726-16c17f97fd06@suse.cz> (raw)
In-Reply-To: <7fd8d3fb-e0c6-decc-374f-495ab81ab1ff@gmail.com>
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
>
next prev parent reply other threads:[~2022-01-13 9:40 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-07 18:33 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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c66ef9b8-aef4-4654-5726-16c17f97fd06@suse.cz \
--to=mliska@suse.cz \
--cc=gcc-patches@gcc.gnu.org \
--cc=joseph@codesourcery.com \
--cc=martin@martin.st \
--cc=redi@gcc.gnu.org \
--cc=tomas.kalibera@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).