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

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

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