On Sat, 2025-01-11 at 20:51 +0100, Jakub Jelinek wrote: > On Sat, Jan 11, 2025 at 08:25:32PM +0100, Jakub Jelinek wrote: > > That is not true. > > Implementation-wise, in C17 say > void foo (); > void bar (void); > TYPE_ARG_TYPES (TREE_TYPE (foo_decl)) == NULL > TYPE_ARG_TYPES (TREE_TYPE (bar_decl)) == void_list_node > but in C23 both have TYPE_ARG_TYPES void_list_node, actually even > TREE_TYPE (foo_decl) == TREE_TYPE (bar_decl). > > So, if you want to actually point out in diagnostics the difference > (because it makes no sense to talk about some C23 change for code > which > wouldn't be valid in C17 either, say > void (*fp) (void); > void fn (int, int); > ... > fp = fn; > ) > we'd need to remember in the function type (some flag on it) whether > it > would be the unspecified arguments case in C17 and earlier or not. > But such flag can't affect type compatibility etc., it should be > there > just for diagnostic purposes only. > And on declaration merging, > void foo (); > void foo (); > should result in the flag still being used on the resulting function > type, > but > void foo (void); > void foo (); > or > void foo (); > void foo (void); > shouldn't have it on the resulting function type.
Thanks Jakub for both replies; I think I understand the distinction now, and it looks like there's no good way to implement a specific hint to the user for this case without extending the IR (e.g. with a flag as you mention). That said, the rest of the patch improves the user experience for - Wincompatible-pointer-types without mentioning C23, though in a way that I think will show up a lot for C23-by-default, based on the stats in: https://fedoraproject.org/wiki/User:Dmalcolm/gcc-15 So I've dropped the takes_int_p, takes_void_p, and maybe_inform_empty_args_c23_transition from the patch. Here's an updated version that keeps the rest of the changes. I'd like to get this into GCC 15 to make build failures due to C23-incompatibilities more readable. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. OK for trunk? Thanks Dave PR c/116871 notes that our diagnostics about incompatible function types could be improved. In particular, for the case of migrating to C23 I'm seeing a lot of build failures with signal handlers similar to this (simplified from alsa-tools-1.2.11, envy24control/profiles.c; see rhbz#2336278): typedef void (*__sighandler_t) (int); extern __sighandler_t signal (int __sig, __sighandler_t __handler) __attribute__ ((__nothrow__ , __leaf__)); void new_process(void) { void (*int_stat)(); int_stat = signal(2, ((__sighandler_t) 1)); signal(2, int_stat); } With trunk, cc1 fails with this message: t.c: In function 'new_process': t.c:18:12: error: assignment to 'void (*)(void)' from incompatible pointer type '__sighandler_t' {aka 'void (*)(int)'} [-Wincompatible-pointer-types] 18 | int_stat = signal(2, ((__sighandler_t) 1)); | ^ t.c:20:13: error: passing argument 2 of 'signal' from incompatible pointer type [-Wincompatible-pointer-types] 20 | signal(2, int_stat); | ^~~~~~~~ | | | void (*)(void) t.c:11:57: note: expected '__sighandler_t' {aka 'void (*)(int)'} but argument is of type 'void (*)(void)' 11 | extern __sighandler_t signal (int __sig, __sighandler_t __handler) | ~~~~~~~~~~~~~~~^~~~~~~~~ With this patch, cc1 emits: t.c: In function 'new_process': t.c:18:12: error: assignment to 'void (*)(void)' from incompatible pointer type '__sighandler_t' {aka 'void (*)(int)'} [-Wincompatible-pointer-types] 18 | int_stat = signal(2, ((__sighandler_t) 1)); | ^ t.c:9:16: note: '__sighandler_t' declared here 9 | typedef void (*__sighandler_t) (int); | ^~~~~~~~~~~~~~ t.c:20:13: error: passing argument 2 of 'signal' from incompatible pointer type [-Wincompatible-pointer-types] 20 | signal(2, int_stat); | ^~~~~~~~ | | | void (*)(void) t.c:11:57: note: expected '__sighandler_t' {aka 'void (*)(int)'} but argument is of type 'void (*)(void)' 11 | extern __sighandler_t signal (int __sig, __sighandler_t __handler) | ~~~~~~~~~~~~~~~^~~~~~~~~ t.c:9:16: note: '__sighandler_t' declared here 9 | typedef void (*__sighandler_t) (int); | ^~~~~~~~~~~~~~ showing the location of the pertinent typedef ("__sighandler_t") Another example, simplfied from a52dec-0.7.4: src/a52dec.c (rhbz#2336013): typedef void (*__sighandler_t) (int); extern __sighandler_t signal (int __sig, __sighandler_t __handler) __attribute__ ((__nothrow__ , __leaf__)); /* Mismatching return type. */ static RETSIGTYPE signal_handler (int sig) { } static void print_fps (int final) { signal (42, signal_handler); } With trunk, cc1 emits: t2.c: In function 'print_fps': t2.c:22:15: error: passing argument 2 of 'signal' from incompatible pointer type [-Wincompatible-pointer-types] 22 | signal (42, signal_handler); | ^~~~~~~~~~~~~~ | | | int (*)(int) t2.c:11:57: note: expected '__sighandler_t' {aka 'void (*)(int)'} but argument is of type 'int (*)(int)' 11 | extern __sighandler_t signal (int __sig, __sighandler_t __handler) | ~~~~~~~~~~~~~~~^~~~~~~~~ With this patch cc1 emits: t2.c: In function 'print_fps': t2.c:22:15: error: passing argument 2 of 'signal' from incompatible pointer type [-Wincompatible-pointer-types] 22 | signal (42, signal_handler); | ^~~~~~~~~~~~~~ | | | int (*)(int) t2.c:11:57: note: expected '__sighandler_t' {aka 'void (*)(int)'} but argument is of type 'int (*)(int)' 11 | extern __sighandler_t signal (int __sig, __sighandler_t __handler) | ~~~~~~~~~~~~~~~^~~~~~~~~ t2.c:16:19: note: 'signal_handler' declared here 16 | static RETSIGTYPE signal_handler (int sig) | ^~~~~~~~~~~~~~ t2.c:9:16: note: '__sighandler_t' declared here 9 | typedef void (*__sighandler_t) (int); | ^~~~~~~~~~~~~~ showing the location of the pertinent fndecl ("signal_handler"), and, as before, the pertinent typedef. The patch also updates the colorization in the messages to visually link and contrast the different types and typedefs. My hope is that this make it easier for users to decipher build failures seen with the new C23 default. Further improvements could be made to colorization in convert_for_assignment, and similar improvements to C++, but I'm punting those to GCC 16. gcc/c/ChangeLog: PR c/116871 * c-typeck.cc (pedwarn_permerror_init): Return bool for whether a warning was emitted. Only call print_spelling if we warned. (pedwarn_init): Return bool for whether a warning was emitted. (permerror_init): Likewise. (warning_init): Return bool for whether a warning was emitted. Only call print_spelling if we warned. (class pp_element_quoted_decl): New. (maybe_inform_typedef_location): New. (convert_for_assignment): For OPT_Wincompatible_pointer_types, move auto_diagnostic_group to cover all cases. Use %e and pp_element rather than %qT and tree to colorize the types. Capture whether a warning was emitted, and, if it was, show various notes: for a pointer to a function, show the function decl, for typedef types, and show the decls. gcc/testsuite/ChangeLog: * gcc.dg/c23-mismatching-fn-ptr-a52dec.c: New test. * gcc.dg/c23-mismatching-fn-ptr-alsatools.c: New test. * gcc.dg/c23-mismatching-fn-ptr.c: New test. Signed-off-by: David Malcolm <dmalc...@redhat.com> --- gcc/c/c-typeck.cc | 175 ++++++++++++++---- .../gcc.dg/c23-mismatching-fn-ptr-a52dec.c | 23 +++ .../gcc.dg/c23-mismatching-fn-ptr-alsatools.c | 21 +++ gcc/testsuite/gcc.dg/c23-mismatching-fn-ptr.c | 76 ++++++++ 4 files changed, 259 insertions(+), 36 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/c23-mismatching-fn-ptr-a52dec.c create mode 100644 gcc/testsuite/gcc.dg/c23-mismatching-fn-ptr-alsatools.c create mode 100644 gcc/testsuite/gcc.dg/c23-mismatching-fn-ptr.c diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc index 2a2083b847d8..c226d97bc61b 100644 --- a/gcc/c/c-typeck.cc +++ b/gcc/c/c-typeck.cc @@ -7477,7 +7477,7 @@ error_init (location_t loc, const char *gmsgid, ...) /* Used to implement pedwarn_init and permerror_init. */ -static void ATTRIBUTE_GCC_DIAG (3,0) +static bool ATTRIBUTE_GCC_DIAG (3,0) pedwarn_permerror_init (location_t loc, int opt, const char *gmsgid, va_list *ap, diagnostic_t kind) { @@ -7487,9 +7487,13 @@ pedwarn_permerror_init (location_t loc, int opt, const char *gmsgid, location_t exploc = expansion_point_location_if_in_system_header (loc); auto_diagnostic_group d; bool warned = emit_diagnostic_valist (kind, exploc, opt, gmsgid, ap); - char *ofwhat = print_spelling ((char *) alloca (spelling_length () + 1)); - if (*ofwhat && warned) - inform (exploc, "(near initialization for %qs)", ofwhat); + if (warned) + { + char *ofwhat = print_spelling ((char *) alloca (spelling_length () + 1)); + if (*ofwhat) + inform (exploc, "(near initialization for %qs)", ofwhat); + } + return warned; } /* Issue a pedantic warning for a bad initializer component. OPT is @@ -7497,24 +7501,26 @@ pedwarn_permerror_init (location_t loc, int opt, const char *gmsgid, it is unconditionally given. GMSGID identifies the message. The component name is taken from the spelling stack. */ -static void ATTRIBUTE_GCC_DIAG (3,0) +static bool ATTRIBUTE_GCC_DIAG (3,0) pedwarn_init (location_t loc, int opt, const char *gmsgid, ...) { va_list ap; va_start (ap, gmsgid); - pedwarn_permerror_init (loc, opt, gmsgid, &ap, DK_PEDWARN); + bool warned = pedwarn_permerror_init (loc, opt, gmsgid, &ap, DK_PEDWARN); va_end (ap); + return warned; } /* Like pedwarn_init, but issue a permerror. */ -static void ATTRIBUTE_GCC_DIAG (3,0) +static bool ATTRIBUTE_GCC_DIAG (3,0) permerror_init (location_t loc, int opt, const char *gmsgid, ...) { va_list ap; va_start (ap, gmsgid); - pedwarn_permerror_init (loc, opt, gmsgid, &ap, DK_PERMERROR); + bool warned = pedwarn_permerror_init (loc, opt, gmsgid, &ap, DK_PERMERROR); va_end (ap); + return warned; } /* Issue a warning for a bad initializer component. @@ -7538,9 +7544,12 @@ warning_init (location_t loc, int opt, const char *gmsgid) /* The gmsgid may be a format string with %< and %>. */ warned = warning_at (exploc, opt, gmsgid); - ofwhat = print_spelling ((char *) alloca (spelling_length () + 1)); - if (*ofwhat && warned) - inform (exploc, "(near initialization for %qs)", ofwhat); + if (warned) + { + ofwhat = print_spelling ((char *) alloca (spelling_length () + 1)); + if (*ofwhat) + inform (exploc, "(near initialization for %qs)", ofwhat); + } } /* If TYPE is an array type and EXPR is a parenthesized string @@ -7651,6 +7660,62 @@ maybe_warn_builtin_no_proto_arg (location_t loc, tree fundecl, int parmnum, fundecl); } +/* Print a declaration in quotes, with the given highlight_color. + Analogous to handler for %qD, but with a specific highlight color. */ + +class pp_element_quoted_decl : public pp_element +{ +public: + pp_element_quoted_decl (tree decl, const char *highlight_color) + : m_decl (decl), + m_highlight_color (highlight_color) + { + } + + void add_to_phase_2 (pp_markup::context &ctxt) override + { + ctxt.begin_quote (); + ctxt.begin_highlight_color (m_highlight_color); + + print_decl (ctxt); + + ctxt.end_highlight_color (); + ctxt.end_quote (); + } + + void print_decl (pp_markup::context &ctxt) + { + pretty_printer *const pp = &ctxt.m_pp; + pp->set_padding (pp_none); + if (DECL_NAME (m_decl)) + pp_identifier (pp, lang_hooks.decl_printable_name (m_decl, 2)); + else + pp_string (pp, _("({anonymous})")); + } + +private: + tree m_decl; + const char *m_highlight_color; +}; + +/* If TYPE is from a typedef, issue a note showing the location + to the user. + Use HIGHLIGHT_COLOR as the highlight color. */ + +static void +maybe_inform_typedef_location (tree type, const char *highlight_color) +{ + if (!typedef_variant_p (type)) + return; + + tree typedef_decl = TYPE_NAME (type); + gcc_assert (TREE_CODE (typedef_decl) == TYPE_DECL); + gcc_rich_location richloc (DECL_SOURCE_LOCATION (typedef_decl), + nullptr, highlight_color); + pp_element_quoted_decl e_typedef_decl (typedef_decl, highlight_color); + inform (&richloc, "%e declared here", &e_typedef_decl); +} + /* Convert value RHS to type TYPE as preparation for an assignment to an lvalue of type TYPE. If ORIGTYPE is not NULL_TREE, it is the original type of RHS; this differs from TREE_TYPE (RHS) for enum @@ -8471,59 +8536,97 @@ convert_for_assignment (location_t location, location_t expr_loc, tree type, /* Avoid warning about the volatile ObjC EH puts on decls. */ else if (!objc_ok) { + auto_diagnostic_group d; + bool warned = false; + pp_markup::element_expected_type e_type (type); + pp_markup::element_actual_type e_rhstype (rhstype); switch (errtype) { case ic_argpass: { - auto_diagnostic_group d; range_label_for_type_mismatch rhs_label (rhstype, type); gcc_rich_location richloc (expr_loc, &rhs_label, highlight_colors::actual); - if (permerror_opt (&richloc, OPT_Wincompatible_pointer_types, + warned + = permerror_opt (&richloc, OPT_Wincompatible_pointer_types, "passing argument %d of %qE from " "incompatible pointer type", - parmnum, rname)) + parmnum, rname); + if (warned) inform_for_arg (fundecl, expr_loc, parmnum, type, rhstype); } break; case ic_assign: if (bltin) - permerror_opt (location, OPT_Wincompatible_pointer_types, - "assignment to %qT from pointer to " - "%qD with incompatible type %qT", - type, bltin, rhstype); + warned + = permerror_opt (location, OPT_Wincompatible_pointer_types, + "assignment to %e from pointer to " + "%qD with incompatible type %e", + &e_type, bltin, &e_rhstype); else - permerror_opt (location, OPT_Wincompatible_pointer_types, - "assignment to %qT from incompatible pointer " - "type %qT", type, rhstype); + warned + = permerror_opt (location, OPT_Wincompatible_pointer_types, + "assignment to %e from incompatible " + "pointer type %e", + &e_type, &e_rhstype); break; case ic_init: case ic_init_const: if (bltin) - permerror_init (location, OPT_Wincompatible_pointer_types, - "initialization of %qT from pointer to " - "%qD with incompatible type %qT", - type, bltin, rhstype); + warned + = permerror_init (location, OPT_Wincompatible_pointer_types, + "initialization of %e from pointer to " + "%qD with incompatible type %e", + &e_type, bltin, &e_rhstype); else - permerror_init (location, OPT_Wincompatible_pointer_types, - "initialization of %qT from incompatible " - "pointer type %qT", - type, rhstype); + warned + = permerror_init (location, OPT_Wincompatible_pointer_types, + "initialization of %e from incompatible " + "pointer type %e", + &e_type, &e_rhstype); break; case ic_return: if (bltin) - permerror_opt (location, OPT_Wincompatible_pointer_types, - "returning pointer to %qD of type %qT from " - "a function with incompatible type %qT", - bltin, rhstype, type); + warned + = permerror_opt (location, OPT_Wincompatible_pointer_types, + "returning pointer to %qD of type %e from " + "a function with incompatible type %e", + bltin, &e_rhstype, &e_type); else - permerror_opt (location, OPT_Wincompatible_pointer_types, - "returning %qT from a function with " - "incompatible return type %qT", rhstype, type); + warned + = permerror_opt (location, OPT_Wincompatible_pointer_types, + "returning %e from a function with " + "incompatible return type %e", + &e_rhstype, &e_type); break; default: gcc_unreachable (); } + if (warned) + { + /* If the mismatching function type is a pointer to a function, + try to show the decl of the function. */ + if (TREE_CODE (rhs) == ADDR_EXPR + && TREE_CODE (TREE_OPERAND (rhs, 0)) == FUNCTION_DECL) + { + tree rhs_fndecl = TREE_OPERAND (rhs, 0); + if (!DECL_IS_UNDECLARED_BUILTIN (rhs_fndecl)) + { + gcc_rich_location richloc + (DECL_SOURCE_LOCATION (rhs_fndecl), nullptr, + highlight_colors::actual); + pp_element_quoted_decl e_rhs_fndecl + (rhs_fndecl, highlight_colors::actual); + inform (&richloc, + "%e declared here", &e_rhs_fndecl); + } + } + /* If either/both of the types are typedefs, show the decl. */ + maybe_inform_typedef_location (type, + highlight_colors::expected); + maybe_inform_typedef_location (rhstype, + highlight_colors::actual); + } } /* If RHS isn't an address, check pointer or array of packed diff --git a/gcc/testsuite/gcc.dg/c23-mismatching-fn-ptr-a52dec.c b/gcc/testsuite/gcc.dg/c23-mismatching-fn-ptr-a52dec.c new file mode 100644 index 000000000000..3c50889fa12a --- /dev/null +++ b/gcc/testsuite/gcc.dg/c23-mismatching-fn-ptr-a52dec.c @@ -0,0 +1,23 @@ +/* Examples of a mismatching function pointer type in + legacy code that got the wrong return type for the signal handler. + + Adapted from a52dec-0.7.4: src/a52dec.c which is GPLv2+. */ + +/* { dg-do compile } */ +/* { dg-options "-std=c23 -pedantic-errors" } */ + +typedef void (*__sighandler_t) (int); /* { dg-message "'__sighandler_t' declared here" } */ + +extern __sighandler_t signal (int __sig, __sighandler_t __handler) /* { dg-message "expected '__sighandler_t' \\{aka '\[^\n\r\]*'\\} but argument is of type '\[^\n\r\]*'" } */ + __attribute__ ((__nothrow__ , __leaf__)); + +/* Mismatching return type. */ +#define RETSIGTYPE int +static RETSIGTYPE signal_handler (int sig) /* { dg-message "'signal_handler' declared here" } */ +{ +} + +static void print_fps (int final) +{ + signal (42, signal_handler); /* { dg-error "passing argument 2 of 'signal' from incompatible pointer type" } */ +} diff --git a/gcc/testsuite/gcc.dg/c23-mismatching-fn-ptr-alsatools.c b/gcc/testsuite/gcc.dg/c23-mismatching-fn-ptr-alsatools.c new file mode 100644 index 000000000000..e3460e546a9a --- /dev/null +++ b/gcc/testsuite/gcc.dg/c23-mismatching-fn-ptr-alsatools.c @@ -0,0 +1,21 @@ +/* Examples of a mismatching function pointer types in + legacy code compiled with C23 that assumed () meant (int). + + Adapted from alsa-tools-1.2.11:envy24control/profiles.c which is GPLv2+. */ + +/* { dg-do compile } */ +/* { dg-options "-std=c23 -pedantic-errors" } */ + +typedef void (*__sighandler_t) (int); /* { dg-message "'__sighandler_t' declared here" } */ + +extern __sighandler_t signal (int __sig, __sighandler_t __handler) + __attribute__ ((__nothrow__ , __leaf__)); + +void new_process(void) +{ + void (*int_stat)(); + + int_stat = signal(2, ((__sighandler_t) 1)); /* { dg-error "assignment to '\[^\n\r\]*' from incompatible pointer type '__sighandler_t' \\{aka '\[^\n\r\]*'\\}" } */ + + signal(2, int_stat); /* { dg-error "passing argument 2 of 'signal' from incompatible pointer type" } */ +} diff --git a/gcc/testsuite/gcc.dg/c23-mismatching-fn-ptr.c b/gcc/testsuite/gcc.dg/c23-mismatching-fn-ptr.c new file mode 100644 index 000000000000..4db44f48a3f2 --- /dev/null +++ b/gcc/testsuite/gcc.dg/c23-mismatching-fn-ptr.c @@ -0,0 +1,76 @@ +/* Verify that when we complain about incompatible pointer types + involving function pointers, we show the declaration of the + function. + + In particular, check the case of + extern void fn (); + changing meaning in C23 (from taking int to taking void). */ + +/* { dg-do compile } */ +/* { dg-options "-std=c23 -pedantic-errors" } */ + +/* We're pretending that this is legacy code that was written before C23, + hence it uses NULL rather than nullptr. */ +#define NULL ((void*)0) + +typedef void (*void_int_fnptr_t) (int); + +extern void takes_void_int_fnptr (void_int_fnptr_t fn); /* { dg-message "expected 'void_int_fnptr_t' \\{aka '\[^\n\r\]*'\\} but argument is of type '\[^\n\r\]*'" } */ +extern void fn_argpass(); /* { dg-message "declared here" } */ +void test_argpass () +{ + takes_void_int_fnptr (&fn_argpass); /* { dg-error "passing argument 1 of 'takes_void_int_fnptr' from incompatible pointer type" } */ +} + +extern void fn_assign(); /* { dg-message "declared here" } */ +void test_assign () +{ + void (*assigned_to) (int); + assigned_to = &fn_assign; /* { dg-error "assignment to '\[^\n\r\]*' from incompatible pointer type '\[^\n\r\]*'" } */ +} + +extern void fn_init(); /* { dg-message "declared here" } */ +void test_init () +{ + void (*initialized) (int) = &fn_init; /* { dg-error "initialization of '\[^\n\r\]*' from incompatible pointer type '\[^\n\r\]*'" } */ +} + +extern void fn_return(); /* { dg-message "declared here" } */ +void_int_fnptr_t +test_return () +{ + return &fn_return; /* { dg-error "returning '\[^\n\r\]*' from a function with incompatible return type '\[^\n\r\]*'" } */ +} + +/* Test of storing a sighandler_t where the declaration of the + destination might be relying on implicit int arg, which + becomes void in C23. + In particular, verify that we show the locations of typedefs. */ + +typedef void (*sighandler_t)(int); /* { dg-message "'sighandler_t' declared here" } */ +sighandler_t signal(int signum, sighandler_t handler); + +typedef void (*wrong_sighandler_t)(void); /* { dg-message "'wrong_sighandler_t' declared here" } */ +extern void takes_wrong_sighandler_type (wrong_sighandler_t fn); /* { dg-message "expected 'wrong_sighandler_t' \\{aka '\[^\n\r\]*'\\} but argument is of type 'sighandler_t' \\{aka '\[^\n\r\]*'\\}" } */ + +void test_argpass_from_signal_result () +{ + takes_wrong_sighandler_type (signal (42, NULL)); /* { dg-error "passing argument 1 of 'takes_wrong_sighandler_type' from incompatible pointer type" } */ +} + +void test_assign_from_signal_result () +{ + wrong_sighandler_t assigned_to; + assigned_to = signal (42, NULL); /* { dg-error "assignment to 'wrong_sighandler_t' \\{aka '\[^\n\r\]*'\\} from incompatible pointer type 'sighandler_t' \\{aka '\[^\n\r\]*'\\}" } */ +} + +void test_init_from_signal_result () +{ + wrong_sighandler_t initialized = signal (42, NULL); /* { dg-error "initialization of 'wrong_sighandler_t' \\{aka '\[^\n\r\]*'\\} from incompatible pointer type 'sighandler_t' \\{aka '\[^\n\r\]*'\\}" } */ +} + +wrong_sighandler_t +test_return_signal_result () +{ + return signal (42, NULL); /* { dg-error "returning 'sighandler_t' \\{aka '\[^\n\r\]*'\\} from a function with incompatible return type 'wrong_sighandler_t' \\{aka '\[^\n\r\]*'\\}" } */ +} -- 2.26.3
