[1/6] c-family: Introduce pedpermerror

Message ID e7ee58c567233aab1b36b9d09f79af6d0108a98b.1699879818.git.fweimer@redhat.com
State Accepted
Headers
Series Turn some C warnings into errors by default |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Florian Weimer Nov. 13, 2023, 1:09 p.m. UTC
  It turns out that permerror_opt is not directly usable for
-fpermissive in the C front end.  The front end uses pedwarn
extensively, and pedwarns are not overridable by -Wno-* options,
yet permerrors are.  Add new pedpermerror helpers that turn into
pedwarns if -pedantic-errors is active.

Due to the dependency on flag_pedantic_errors, the new helpers
are specific to the C-family front ends.  For implementing the
rich location variant, export emit_diagnostic_valist from
gcc/diagnostic.cc in parallel to its location_t variant.

gcc/

	* diagnostic-core.h (emit_diagnostic_valist): Declare function.
	* diagnostic.cc (emit_diagnostic_valist): Define it.

gcc/c-family/

	* c-common.h (pedpermerror): Declare functions.
	* c-warn.cc (pedpermerror): Define them.
---
 gcc/c-family/c-common.h |  4 ++++
 gcc/c-family/c-warn.cc  | 34 ++++++++++++++++++++++++++++++++++
 gcc/diagnostic-core.h   |  3 +++
 gcc/diagnostic.cc       |  7 +++++++
 4 files changed, 48 insertions(+)
  

Comments

Jeff Law Nov. 13, 2023, 7:22 p.m. UTC | #1
On 11/13/23 06:09, Florian Weimer wrote:
> It turns out that permerror_opt is not directly usable for
> -fpermissive in the C front end.  The front end uses pedwarn
> extensively, and pedwarns are not overridable by -Wno-* options,
> yet permerrors are.  Add new pedpermerror helpers that turn into
> pedwarns if -pedantic-errors is active.
> 
> Due to the dependency on flag_pedantic_errors, the new helpers
> are specific to the C-family front ends.  For implementing the
> rich location variant, export emit_diagnostic_valist from
> gcc/diagnostic.cc in parallel to its location_t variant.
> 
> gcc/
> 
> 	* diagnostic-core.h (emit_diagnostic_valist): Declare function.
> 	* diagnostic.cc (emit_diagnostic_valist): Define it.
> 
> gcc/c-family/
> 
> 	* c-common.h (pedpermerror): Declare functions.
> 	* c-warn.cc (pedpermerror): Define them.
OK
jeff
  
Florian Weimer Nov. 14, 2023, 7:39 a.m. UTC | #2
* Florian Weimer:

> It turns out that permerror_opt is not directly usable for
> -fpermissive in the C front end.  The front end uses pedwarn
> extensively, and pedwarns are not overridable by -Wno-* options,
> yet permerrors are.  Add new pedpermerror helpers that turn into
> pedwarns if -pedantic-errors is active.
>
> Due to the dependency on flag_pedantic_errors, the new helpers
> are specific to the C-family front ends.  For implementing the
> rich location variant, export emit_diagnostic_valist from
> gcc/diagnostic.cc in parallel to its location_t variant.
>
> gcc/
>
> 	* diagnostic-core.h (emit_diagnostic_valist): Declare function.
> 	* diagnostic.cc (emit_diagnostic_valist): Define it.
>
> gcc/c-family/
>
> 	* c-common.h (pedpermerror): Declare functions.
> 	* c-warn.cc (pedpermerror): Define them.

Jason suggested off-list that this shouldn't be necessary, and the
description of -pedantic-errors is wrong (it is possible to undo
-pedantic-errors effects with -Wno-error=…).  The permerror_opt
interface should already do what I need.

It turns out that I was very unlucky and picked -Wreturn-type for my
tests.

This:

long i = "abc";
volatile j;
int f (void) { return; }

Gives, with GCC 13:

$ gcc -pedantic-errors -Wno-error=implicit-int -Wno-error=int-conversion -Wno-error=return-type test.c
test.c:1:10: warning: initialization of ‘long int’ from ‘char *’ makes integer from pointer without a cast [-Wint-conversion]
    1 | long i = "abc";
      |          ^~~~~
test.c:2:10: warning: type defaults to ‘int’ in declaration of ‘j’ [-Wimplicit-int]
    2 | volatile j;
      |          ^
test.c: In function ‘f’:
test.c:3:16: error: ‘return’ with no value, in function returning non-void
    3 | int f (void) { return; }
      |                ^~~~~~
test.c:3:5: note: declared here
    3 | int f (void) { return; }
      |     ^

This happens because we drop the OPT_Wreturn_type in some cases:

          if (flag_isoc99)
            warned_here = pedwarn
              (loc, warn_return_type >= 0 ? OPT_Wreturn_type : 0,
               "%<return%> with no value, in function returning non-void");
          else
            warned_here = warning_at
              (loc, OPT_Wreturn_type,
               "%<return%> with no value, in function returning non-void");

And for the other direction:

      if (TREE_CODE (TREE_TYPE (retval)) != VOID_TYPE)
        warned_here = pedwarn
          (xloc, warn_return_type >= 0 ? OPT_Wreturn_type : 0,
           "%<return%> with a value, in function returning void");

I think with the -Wreturn-mismatch split, we can drop the
warn_return_type >= 0 condition, and then permerror_opt should indeed
do the right thing.

I'll write the kitchen sink test now, use that to verify this theory,
and repost as appropriate.

Thanks,
Florian
  

Patch

diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index b57e83d7c5d..789e0bf2459 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -1486,6 +1486,10 @@  extern void warn_for_address_or_pointer_of_packed_member (tree, tree);
 extern void warn_parm_array_mismatch (location_t, tree, tree);
 extern void maybe_warn_sizeof_array_div (location_t, tree, tree, tree, tree);
 extern void do_warn_array_compare (location_t, tree_code, tree, tree);
+extern bool pedpermerror (location_t, int, const char *,
+			  ...) ATTRIBUTE_GCC_DIAG(3,4);
+extern bool pedpermerror (rich_location *, int, const char *,
+			  ...) ATTRIBUTE_GCC_DIAG(3,4);
 
 /* Places where an lvalue, or modifiable lvalue, may be required.
    Used to select diagnostic messages in lvalue_error and
diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc
index b1bd8ba9f42..475a3e4b42e 100644
--- a/gcc/c-family/c-warn.cc
+++ b/gcc/c-family/c-warn.cc
@@ -3932,3 +3932,37 @@  check_for_xor_used_as_pow (location_t lhs_loc, tree lhs_val,
 	      lhs_uhwi, lhs_uhwi);
     }
 }
+
+/* If !flag_pedantic_errors, equivalent to permerror_opt, otherwise to
+   pedwarn.  */
+
+bool
+pedpermerror (location_t location, int opt, const char *gmsgid, ...)
+{
+  auto_diagnostic_group d;
+  va_list ap;
+  va_start (ap, gmsgid);
+  rich_location richloc (line_table, location);
+  bool ret = emit_diagnostic_valist (flag_pedantic_errors
+				     ? DK_PEDWARN : DK_PERMERROR,
+				     location, opt, gmsgid, &ap);
+  va_end (ap);
+  return ret;
+}
+
+/* Same as "pedpermerror" above, but at RICHLOC.  */
+
+bool
+pedpermerror (rich_location *richloc, int opt, const char *gmsgid, ...)
+{
+  gcc_assert (richloc);
+
+  auto_diagnostic_group d;
+  va_list ap;
+  va_start (ap, gmsgid);
+  bool ret = emit_diagnostic_valist (flag_pedantic_errors
+				     ? DK_PEDWARN : DK_PERMERROR,
+				     richloc, opt, gmsgid, &ap);
+  va_end (ap);
+  return ret;
+}
diff --git a/gcc/diagnostic-core.h b/gcc/diagnostic-core.h
index 04eba3d140e..eac775fb573 100644
--- a/gcc/diagnostic-core.h
+++ b/gcc/diagnostic-core.h
@@ -121,6 +121,9 @@  extern bool emit_diagnostic (diagnostic_t, location_t, int,
 			     const char *, ...) ATTRIBUTE_GCC_DIAG(4,5);
 extern bool emit_diagnostic (diagnostic_t, rich_location *, int,
 			     const char *, ...) ATTRIBUTE_GCC_DIAG(4,5);
+extern bool emit_diagnostic_valist (diagnostic_t, rich_location *, int,
+				    const char *,
+				    va_list *) ATTRIBUTE_GCC_DIAG (4,0);
 extern bool emit_diagnostic_valist (diagnostic_t, location_t, int, const char *,
 				    va_list *) ATTRIBUTE_GCC_DIAG (4,0);
 extern bool seen_error (void);
diff --git a/gcc/diagnostic.cc b/gcc/diagnostic.cc
index addd6606eaa..6e73343dbdb 100644
--- a/gcc/diagnostic.cc
+++ b/gcc/diagnostic.cc
@@ -1829,6 +1829,13 @@  emit_diagnostic_valist (diagnostic_t kind, location_t location, int opt,
   return diagnostic_impl (&richloc, NULL, opt, gmsgid, ap, kind);
 }
 
+bool
+emit_diagnostic_valist (diagnostic_t kind, rich_location *richloc, int opt,
+			const char *gmsgid, va_list *ap)
+{
+  return diagnostic_impl (richloc, NULL, opt, gmsgid, ap, kind);
+}
+
 /* An informative note at LOCATION.  Use this for additional details on an error
    message.  */
 void