Re: [PATCH] [12/11/10] Fix invalid format warnings on Windows

[Tomas Kalibera <tomas.kalibera@gmail.com>]

[-- 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