[RFA] [PATCH] Add a warning for invalid function casts

[RFA] [PATCH] Add a warning for invalid function casts

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]

Hi!

I have implemented a warning -Wcast-function-type that analyzes
type casts which change the function signatures.

I would consider function pointers with different result type
invalid, also if both function types have a non-null TYPE_ARG_TYPES
I would say this deserves a warning.  As an exception I have
used for instance in recog.h, the warning allows casting
a function with the type typedef rtx (*stored_funcptr) (...);
to any function with the same result type.

I would think a warning like that should be enabled with -Wextra.

Attached is a first version of the patch and as you can see
the warning found already lots of suspicious type casts.  The worst
is the splay-tree which always calls functions with uintptr_t
instead of the correct parameter type.  I was unable to find
a solution for this, and just silenced the warning with a
second type-cast.

Note that I also changed one line in libgo, but that is only
a quick hack which I only did to make the boot-strap with
all languages succeed.

I'm not sure if this warning may be a bit too strict, but I think
so far it just triggered on rather questionable code.

Thoughts?


Bernd.

[-- Attachment #2: changelog-cast-function-type.txt --]
[-- Type: text/plain, Size: 1720 bytes --]

gcc:
2017-10-03  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* doc/invoke.texi: Document -Wcast-function-type.
	* gengtype.c (write_root): Remove cast.
	* ggc.h (gt_pch_n_S_nonconst, gt_ggc_m_S_nonconst): Declare.
	* ggc-page.c (gt_ggc_m_S_nonconst): New function.
	* stringpool.c (gt_pch_n_S_nonconst): New function.
	* tree-pass.h (do_per_function_toporder): Adjust header.
	* passes.c (do_per_function_toporder): Change signature.
	(execute_ipa_pass_list): Remove cast.
	* recog.h (f0..f15): Fix return types.
	(stored_funcptr): Use variadic parameter list.
	* tree-dump.c (dump_node): Avoid warning.
	* typed-splay-tree.h (typed_splay_tree): Avoid warning.

libcpp:
2017-10-03  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* include/symtab.h (ht_forall_internal): Declare.
	* symtab.c (ht_forall_internal): New function.
	* internal.h (maybe_print_line): Change signature.
	
c-family:
2017-10-03  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c.opt (Wcast-function-type): New warning option.
	* c-lex.c (get_fileinfo): Avoid warning.
	* c-ppoutput.c (scan_translation_unit_directives_only): Remove cast.

c:
2017-10-03  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-typeck.c (build_c_cast): Implement -Wcast_function_type.

cp:
2017-10-03  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* cxx-pretty-print.c (pp_c_type_specifier_seq,
	pp_c_parameter_declaration_clause): New wrapper functions.
	(cxx_pretty_printer::cxx_pretty_printer): Remove cast.
	* decl2.c (start_static_storage_duration_function): Aboid warning.
	* typeck.c (build_reinterpret_cast_1): Implement -Wcast_function_type.

testsuite:
2017-10-03  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-c++-common/Wcast-function-type.c: New test.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: patch-cast-function-type.diff --]
[-- Type: text/x-patch; name="patch-cast-function-type.diff", Size: 24484 bytes --]

Index: gcc/c/c-typeck.c
===================================================================
--- gcc/c/c-typeck.c	(revision 253328)
+++ gcc/c/c-typeck.c	(working copy)
@@ -5667,6 +5667,20 @@ build_c_cast (location_t loc, tree type, tree expr
 	pedwarn (loc, OPT_Wpedantic, "ISO C forbids "
 		 "conversion of object pointer to function pointer type");
 
+      if (TREE_CODE (type) == POINTER_TYPE
+	  && TREE_CODE (otype) == POINTER_TYPE
+	  && TREE_CODE (TREE_TYPE (type)) == FUNCTION_TYPE
+	  && TREE_CODE (TREE_TYPE (otype)) == FUNCTION_TYPE
+	  && (TYPE_ARG_TYPES (TREE_TYPE (type))
+	      && TYPE_ARG_TYPES (TREE_TYPE (otype))
+	      ? !comptypes (TREE_TYPE (type),
+			    TREE_TYPE (otype))
+	      : !comptypes (TREE_TYPE (TREE_TYPE (type)),
+			    TREE_TYPE (TREE_TYPE (otype)))))
+	warning_at (loc, OPT_Wcast_function_type,
+		    "cast between incompatible function types"
+		    " from %qT to %qT", otype, type);
+
       ovalue = value;
       value = convert (type, value);
 
Index: gcc/c-family/c-lex.c
===================================================================
--- gcc/c-family/c-lex.c	(revision 253328)
+++ gcc/c-family/c-lex.c	(working copy)
@@ -101,9 +101,11 @@ get_fileinfo (const char *name)
   struct c_fileinfo *fi;
 
   if (!file_info_tree)
-    file_info_tree = splay_tree_new ((splay_tree_compare_fn) strcmp,
+    file_info_tree = splay_tree_new ((splay_tree_compare_fn)
+				     (uintptr_t) strcmp,
 				     0,
-				     (splay_tree_delete_value_fn) free);
+				     (splay_tree_delete_value_fn)
+				     (uintptr_t) free);
 
   n = splay_tree_lookup (file_info_tree, (splay_tree_key) name);
   if (n)
Index: gcc/c-family/c-ppoutput.c
===================================================================
--- gcc/c-family/c-ppoutput.c	(revision 253328)
+++ gcc/c-family/c-ppoutput.c	(working copy)
@@ -299,7 +299,7 @@ scan_translation_unit_directives_only (cpp_reader
   struct _cpp_dir_only_callbacks cb;
 
   cb.print_lines = print_lines_directives_only;
-  cb.maybe_print_line = (void (*) (source_location)) maybe_print_line;
+  cb.maybe_print_line = maybe_print_line;
 
   _cpp_preprocess_dir_only (pfile, &cb);
 }
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 253328)
+++ gcc/c-family/c.opt	(working copy)
@@ -384,6 +384,10 @@ Wc++17-compat
 C++ ObjC++ Var(warn_cxx17_compat) Warning LangEnabledBy(C++ ObjC++,Wall)
 Warn about C++ constructs whose meaning differs between ISO C++ 2014 and ISO C++ 2017.
 
+Wcast-function-type
+C ObjC C++ ObjC++ Var(warn_cast_function_type) Warning EnabledBy(Wextra)
+Warn about casts between incompatible function types.
+
 Wcast-qual
 C ObjC C++ ObjC++ Var(warn_cast_qual) Warning
 Warn about casts which discard qualifiers.
Index: gcc/cp/cxx-pretty-print.c
===================================================================
--- gcc/cp/cxx-pretty-print.c	(revision 253328)
+++ gcc/cp/cxx-pretty-print.c	(working copy)
@@ -2935,8 +2935,16 @@ pp_cxx_constraint (cxx_pretty_printer *pp, tree t)
 
 
 \f
-typedef c_pretty_print_fn pp_fun;
+static void pp_c_type_specifier_seq (c_pretty_printer *pp, tree t)
+{
+  pp_cxx_type_specifier_seq ((cxx_pretty_printer *)pp, t);
+}
 
+static void pp_c_parameter_declaration_clause (c_pretty_printer *pp, tree t)
+{
+  pp_cxx_parameter_declaration_clause ((cxx_pretty_printer *)pp, t);
+}
+
 /* Initialization of a C++ pretty-printer object.  */
 
 cxx_pretty_printer::cxx_pretty_printer ()
@@ -2943,6 +2951,6 @@ cxx_pretty_printer::cxx_pretty_printer ()
   : c_pretty_printer (),
     enclosing_scope (global_namespace)
 {
-  type_specifier_seq = (pp_fun) pp_cxx_type_specifier_seq;
-  parameter_list = (pp_fun) pp_cxx_parameter_declaration_clause;
+  type_specifier_seq = pp_c_type_specifier_seq;
+  parameter_list = pp_c_parameter_declaration_clause;
 }
Index: gcc/cp/decl2.c
===================================================================
--- gcc/cp/decl2.c	(revision 253328)
+++ gcc/cp/decl2.c	(working copy)
@@ -3480,7 +3480,8 @@ start_static_storage_duration_function (unsigned c
       priority_info_map = splay_tree_new (splay_tree_compare_ints,
 					  /*delete_key_fn=*/0,
 					  /*delete_value_fn=*/
-					  (splay_tree_delete_value_fn) &free);
+					  (splay_tree_delete_value_fn)
+					  (uintptr_t) free);
 
       /* We always need to generate functions for the
 	 DEFAULT_INIT_PRIORITY so enter it now.  That way when we walk
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 253328)
+++ gcc/cp/typeck.c	(working copy)
@@ -7261,8 +7261,21 @@ build_reinterpret_cast_1 (tree type, tree expr, bo
 	   && same_type_p (type, intype))
     /* DR 799 */
     return rvalue (expr);
-  else if ((TYPE_PTRFN_P (type) && TYPE_PTRFN_P (intype))
-	   || (TYPE_PTRMEMFUNC_P (type) && TYPE_PTRMEMFUNC_P (intype)))
+  else if (TYPE_PTRFN_P (type) && TYPE_PTRFN_P (intype))
+    {
+      if ((complain & tf_warning)
+	  && (TYPE_ARG_TYPES (TREE_TYPE (type))
+	      && TYPE_ARG_TYPES (TREE_TYPE (intype))
+	      ? !same_type_p (TREE_TYPE (type),
+			      TREE_TYPE (intype))
+	      : !same_type_p (TREE_TYPE (TREE_TYPE (type)),
+			      TREE_TYPE (TREE_TYPE (intype)))))
+	warning (OPT_Wcast_function_type,
+		 "cast between incompatible function types"
+		 " from %qH to %qI", intype, type);
+      return build_nop (type, expr);
+    }
+  else if (TYPE_PTRMEMFUNC_P (type) && TYPE_PTRMEMFUNC_P (intype))
     return build_nop (type, expr);
   else if ((TYPE_PTRDATAMEM_P (type) && TYPE_PTRDATAMEM_P (intype))
 	   || (TYPE_PTROBV_P (type) && TYPE_PTROBV_P (intype)))
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 253328)
+++ gcc/doc/invoke.texi	(working copy)
@@ -267,7 +267,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wno-builtin-declaration-mismatch @gol
 -Wno-builtin-macro-redefined  -Wc90-c99-compat  -Wc99-c11-compat @gol
 -Wc++-compat  -Wc++11-compat  -Wc++14-compat  @gol
--Wcast-align  -Wcast-align=strict  -Wcast-qual  @gol
+-Wcast-align  -Wcast-align=strict  -Wcast-function-type  -Wcast-qual  @gol
 -Wchar-subscripts  -Wchkp  -Wcatch-value  -Wcatch-value=@var{n} @gol
 -Wclobbered  -Wcomment  -Wconditionally-supported @gol
 -Wconversion  -Wcoverage-mismatch  -Wno-cpp  -Wdangling-else  -Wdate-time @gol
@@ -3899,6 +3899,7 @@ This enables some extra warning flags that are not
 name is still supported, but the newer name is more descriptive.)
 
 @gccoptlist{-Wclobbered  @gol
+-Wcast-function-type  @gol
 -Wempty-body  @gol
 -Wignored-qualifiers @gol
 -Wimplicit-fallthrough=3 @gol
@@ -5948,6 +5949,15 @@ Warn whenever a pointer is cast such that the requ
 target is increased.  For example, warn if a @code{char *} is cast to
 an @code{int *} regardless of the target machine.
 
+@item -Wcast-function-type
+@opindex Wcast-function-type
+@opindex Wno-cast-function-type
+Warn when a function pointer is cast to an incompatible function pointer.
+When one of the function types uses variable arguments like this
+@code{int f(...);}, then only the return value is checked, otherwise
+the return value and the parameter types are checked.
+This warning is enabled by @option{-Wextra}.
+
 @item -Wwrite-strings
 @opindex Wwrite-strings
 @opindex Wno-write-strings
Index: gcc/gengtype.c
===================================================================
--- gcc/gengtype.c	(revision 253328)
+++ gcc/gengtype.c	(working copy)
@@ -4406,8 +4406,8 @@ write_root (outf_p f, pair_p v, type_p type, const
 	if (!start_root_entry (f, v, name, line))
 	  return;
 
-	oprintf (f, "    (gt_pointer_walker) &gt_ggc_m_S,\n");
-	oprintf (f, "    (gt_pointer_walker) &gt_pch_n_S\n");
+	oprintf (f, "    &gt_ggc_m_S_nonconst,\n");
+	oprintf (f, "    &gt_pch_n_S_nonconst\n");
 	oprintf (f, "  },\n");
       }
       break;
Index: gcc/ggc-page.c
===================================================================
--- gcc/ggc-page.c	(revision 253328)
+++ gcc/ggc-page.c	(working copy)
@@ -1500,6 +1500,11 @@ gt_ggc_m_S (const void *p)
   return;
 }
 
+void
+gt_ggc_m_S_nonconst (void *p)
+{
+  gt_ggc_m_S (CONST_CAST(const void *, p));
+}
 
 /* User-callable entry points for marking string X.  */
 
Index: gcc/ggc.h
===================================================================
--- gcc/ggc.h	(revision 253328)
+++ gcc/ggc.h	(working copy)
@@ -98,6 +98,8 @@ extern int ggc_marked_p	(const void *);
 /* PCH and GGC handling for strings, mostly trivial.  */
 extern void gt_pch_n_S (const void *);
 extern void gt_ggc_m_S (const void *);
+extern void gt_pch_n_S_nonconst (void *);
+extern void gt_ggc_m_S_nonconst (void *);
 
 /* End of GTY machinery API.  */
 
Index: gcc/passes.c
===================================================================
--- gcc/passes.c	(revision 253328)
+++ gcc/passes.c	(working copy)
@@ -1694,7 +1694,8 @@ remove_cgraph_node_from_order (cgraph_node *node,
    call CALLBACK on the current function.
    This function is global so that plugins can use it.  */
 void
-do_per_function_toporder (void (*callback) (function *, void *data), void *data)
+do_per_function_toporder (void (*callback) (function *, opt_pass *data),
+			  opt_pass *data)
 {
   int i;
 
@@ -2932,9 +2933,7 @@ execute_ipa_pass_list (opt_pass *pass)
 	  if (pass->sub->type == GIMPLE_PASS)
 	    {
 	      invoke_plugin_callbacks (PLUGIN_EARLY_GIMPLE_PASSES_START, NULL);
-	      do_per_function_toporder ((void (*)(function *, void *))
-					  execute_pass_list,
-					pass->sub);
+	      do_per_function_toporder (execute_pass_list, pass->sub);
 	      invoke_plugin_callbacks (PLUGIN_EARLY_GIMPLE_PASSES_END, NULL);
 	    }
 	  else if (pass->sub->type == SIMPLE_IPA_PASS
Index: gcc/recog.h
===================================================================
--- gcc/recog.h	(revision 253328)
+++ gcc/recog.h	(working copy)
@@ -276,43 +276,60 @@ typedef const char * (*insn_output_fn) (rtx *, rtx
 
 struct insn_gen_fn
 {
-  typedef rtx_insn * (*f0) (void);
-  typedef rtx_insn * (*f1) (rtx);
-  typedef rtx_insn * (*f2) (rtx, rtx);
-  typedef rtx_insn * (*f3) (rtx, rtx, rtx);
-  typedef rtx_insn * (*f4) (rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f5) (rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f6) (rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f7) (rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f8) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f9) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f10) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f11) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f12) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f13) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f14) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f15) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
-  typedef rtx_insn * (*f16) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
+  typedef rtx (*f0) (void);
+  typedef rtx (*f1) (rtx);
+  typedef rtx (*f2) (rtx, rtx);
+  typedef rtx (*f3) (rtx, rtx, rtx);
+  typedef rtx (*f4) (rtx, rtx, rtx, rtx);
+  typedef rtx (*f5) (rtx, rtx, rtx, rtx, rtx);
+  typedef rtx (*f6) (rtx, rtx, rtx, rtx, rtx, rtx);
+  typedef rtx (*f7) (rtx, rtx, rtx, rtx, rtx, rtx, rtx);
+  typedef rtx (*f8) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
+  typedef rtx (*f9) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
+  typedef rtx (*f10) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
+  typedef rtx (*f11) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
+  typedef rtx (*f12) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
+  typedef rtx (*f13) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
+  typedef rtx (*f14) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
+  typedef rtx (*f15) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
+  typedef rtx (*f16) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
 
-  typedef f0 stored_funcptr;
+  typedef rtx (*stored_funcptr) (...);
 
-  rtx_insn * operator () (void) const { return ((f0)func) (); }
-  rtx_insn * operator () (rtx a0) const { return ((f1)func) (a0); }
-  rtx_insn * operator () (rtx a0, rtx a1) const { return ((f2)func) (a0, a1); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2) const { return ((f3)func) (a0, a1, a2); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3) const { return ((f4)func) (a0, a1, a2, a3); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4) const { return ((f5)func) (a0, a1, a2, a3, a4); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5) const { return ((f6)func) (a0, a1, a2, a3, a4, a5); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6) const { return ((f7)func) (a0, a1, a2, a3, a4, a5, a6); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7) const { return ((f8)func) (a0, a1, a2, a3, a4, a5, a6, a7); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8) const { return ((f9)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9) const { return ((f10)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9, rtx a10) const { return ((f11)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9, rtx a10, rtx a11) const { return ((f12)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9, rtx a10, rtx a11, rtx a12) const { return ((f13)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9, rtx a10, rtx a11, rtx a12, rtx a13) const { return ((f14)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9, rtx a10, rtx a11, rtx a12, rtx a13, rtx a14) const { return ((f15)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13, a14); }
-  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9, rtx a10, rtx a11, rtx a12, rtx a13, rtx a14, rtx a15) const { return ((f16)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13, a14, a15); }
+  rtx_insn * operator () (void) const
+  { return (rtx_insn *) ((f0)func) (); }
+  rtx_insn * operator () (rtx a0) const
+  { return (rtx_insn *) ((f1)func) (a0); }
+  rtx_insn * operator () (rtx a0, rtx a1) const
+  { return (rtx_insn *) ((f2)func) (a0, a1); }
+  rtx_insn * operator () (rtx a0, rtx a1, rtx a2) const
+  { return (rtx_insn *) ((f3)func) (a0, a1, a2); }
+  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3) const
+  { return (rtx_insn *) ((f4)func) (a0, a1, a2, a3); }
+  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4) const
+  { return (rtx_insn *) ((f5)func) (a0, a1, a2, a3, a4); }
+  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5) const
+  { return (rtx_insn *) ((f6)func) (a0, a1, a2, a3, a4, a5); }
+  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6) const
+  { return (rtx_insn *) ((f7)func) (a0, a1, a2, a3, a4, a5, a6); }
+  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7) const
+  { return (rtx_insn *) ((f8)func) (a0, a1, a2, a3, a4, a5, a6, a7); }
+  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8) const
+  { return (rtx_insn *) ((f9)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8); }
+  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9) const
+  { return (rtx_insn *) ((f10)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9); }
+  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9, rtx a10) const
+  { return (rtx_insn *) ((f11)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10); }
+  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9, rtx a10, rtx a11) const
+  { return (rtx_insn *)((f12)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11); }
+  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9, rtx a10, rtx a11, rtx a12) const
+  { return (rtx_insn *) ((f13)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12); }
+  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9, rtx a10, rtx a11, rtx a12, rtx a13) const
+  { return (rtx_insn *) ((f14)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13); }
+  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9, rtx a10, rtx a11, rtx a12, rtx a13, rtx a14) const
+  { return (rtx_insn *) ((f15)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13, a14); }
+  rtx_insn * operator () (rtx a0, rtx a1, rtx a2, rtx a3, rtx a4, rtx a5, rtx a6, rtx a7, rtx a8, rtx a9, rtx a10, rtx a11, rtx a12, rtx a13, rtx a14, rtx a15) const
+  { return (rtx_insn *) ((f16)func) (a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11, a12, a13, a14, a15); }
 
   // This is for compatibility of code that invokes functions like
   //   (*funcptr) (arg)
Index: gcc/stringpool.c
===================================================================
--- gcc/stringpool.c	(revision 253328)
+++ gcc/stringpool.c	(working copy)
@@ -196,6 +196,11 @@ gt_pch_n_S (const void *x)
 		      &gt_pch_p_S);
 }
 
+void
+gt_pch_n_S_nonconst (void *x)
+{
+  gt_pch_n_S (CONST_CAST (const void *, x));
+}
 
 /* User-callable entry point for marking string X.  */
 
Index: gcc/testsuite/c-c++-common/Wcast-function-type.c
===================================================================
--- gcc/testsuite/c-c++-common/Wcast-function-type.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wcast-function-type.c	(working copy)
@@ -0,0 +1,28 @@
+/* { dg-do compile } */
+/* { dg-options "-Wcast-function-type" } */
+
+int f(long);
+
+typedef int (f1)(long);
+typedef int (f2)(void*);
+#ifdef __cplusplus
+typedef int (f3)(...);
+typedef void (f4)(...);
+#else
+typedef int (f3)();
+typedef void (f4)();
+#endif
+
+f1 *a;
+f2 *b;
+f3 *c;
+f4 *d;
+
+void
+foo (void)
+{
+  a = (f1 *) f; /* { dg-bogus   "incompatible function types" } */
+  b = (f2 *) f; /* { dg-warning "incompatible function types" } */
+  c = (f3 *) f; /* { dg-bogus   "incompatible function types" } */
+  d = (f4 *) f; /* { dg-warning "incompatible function types" } */
+}
Index: gcc/tree-dump.c
===================================================================
--- gcc/tree-dump.c	(revision 253328)
+++ gcc/tree-dump.c	(working copy)
@@ -735,7 +735,8 @@ dump_node (const_tree t, dump_flags_t flags, FILE
   di.flags = flags;
   di.node = t;
   di.nodes = splay_tree_new (splay_tree_compare_pointers, 0,
-			     (splay_tree_delete_value_fn) &free);
+			     (splay_tree_delete_value_fn)
+			     (uintptr_t) free);
 
   /* Queue up the first node.  */
   queue (&di, t, DUMP_NONE);
Index: gcc/tree-pass.h
===================================================================
--- gcc/tree-pass.h	(revision 253328)
+++ gcc/tree-pass.h	(working copy)
@@ -646,7 +646,8 @@ extern void register_one_dump_file (opt_pass *);
 extern bool function_called_by_processed_nodes_p (void);
 
 /* Declare for plugins.  */
-extern void do_per_function_toporder (void (*) (function *, void *), void *);
+extern void do_per_function_toporder (void (*) (function *, opt_pass *),
+				      opt_pass *);
 
 extern void disable_pass (const char *);
 extern void enable_pass (const char *);
Index: gcc/typed-splay-tree.h
===================================================================
--- gcc/typed-splay-tree.h	(revision 253328)
+++ gcc/typed-splay-tree.h	(working copy)
@@ -75,9 +75,12 @@ inline typed_splay_tree<KEY_TYPE, VALUE_TYPE>::
 		    delete_key_fn delete_key_fn,
 		    delete_value_fn delete_value_fn)
 {
-  m_inner = splay_tree_new ((splay_tree_compare_fn)compare_fn,
-			    (splay_tree_delete_key_fn)delete_key_fn,
-			    (splay_tree_delete_value_fn)delete_value_fn);
+  m_inner = splay_tree_new ((splay_tree_compare_fn)
+			    (uintptr_t) compare_fn,
+			    (splay_tree_delete_key_fn)
+			    (uintptr_t) delete_key_fn,
+			    (splay_tree_delete_value_fn)
+			    (uintptr_t) delete_value_fn);
 }
 
 /* Destructor for typed_splay_tree <K, V>.  */
Index: libcpp/identifiers.c
===================================================================
--- libcpp/identifiers.c	(revision 253328)
+++ libcpp/identifiers.c	(working copy)
@@ -116,5 +116,5 @@ extern char proxy_assertion_broken[offsetof (struc
 void
 cpp_forall_identifiers (cpp_reader *pfile, cpp_cb cb, void *v)
 {
-  ht_forall (pfile->hash_table, (ht_cb) cb, v);
+  ht_forall_internal (pfile->hash_table, cb, v);
 }
Index: libcpp/include/symtab.h
===================================================================
--- libcpp/include/symtab.h	(revision 253328)
+++ libcpp/include/symtab.h	(working copy)
@@ -88,6 +88,11 @@ extern hashnode ht_lookup_with_hash (cpp_hash_tabl
    if the callback returns zero.  */
 typedef int (*ht_cb) (struct cpp_reader *, hashnode, const void *);
 extern void ht_forall (cpp_hash_table *, ht_cb, const void *);
+struct cpp_hashnode;
+extern void ht_forall_internal (cpp_hash_table *,
+				int (*)(struct cpp_reader *,
+					cpp_hashnode *, void *),
+				void *);
 
 /* For all nodes in TABLE, call the callback.  If the callback returns
    a nonzero value, the node is removed from the table.  */
Index: libcpp/internal.h
===================================================================
--- libcpp/internal.h	(revision 253328)
+++ libcpp/internal.h	(working copy)
@@ -708,7 +708,7 @@ struct _cpp_dir_only_callbacks
 {
   /* Called to print a block of lines. */
   void (*print_lines) (int, const void *, size_t);
-  void (*maybe_print_line) (source_location);
+  bool (*maybe_print_line) (source_location);
 };
 
 extern void _cpp_preprocess_dir_only (cpp_reader *,
Index: libcpp/symtab.c
===================================================================
--- libcpp/symtab.c	(revision 253328)
+++ libcpp/symtab.c	(working copy)
@@ -236,6 +236,26 @@ ht_forall (cpp_hash_table *table, ht_cb cb, const
   while (++p < limit);
 }
 
+/* Like ht_forall, but with different parameter types in the callback.  */
+void
+ht_forall_internal (cpp_hash_table *table,
+		    int (*cb)(struct cpp_reader *,
+			      cpp_hashnode *, void *),
+		    void *v)
+{
+  hashnode *p, *limit;
+
+  p = table->entries;
+  limit = p + table->nslots;
+  do
+    if (*p && *p != DELETED)
+      {
+	if ((*cb) (table->pfile, (cpp_hashnode *)*p, v) == 0)
+	  break;
+      }
+  while (++p < limit);
+}
+
 /* Like ht_forall, but a nonzero return from the callback means that
    the entry should be removed from the table.  */
 void
Index: libgo/runtime/runtime.h
===================================================================
--- libgo/runtime/runtime.h	(revision 253328)
+++ libgo/runtime/runtime.h	(working copy)
@@ -94,7 +94,7 @@ struct String
 
 struct FuncVal
 {
-	void	(*fn)(void);
+	void	(*fn)();
 	// variable-size, fn-specific data here
 };
 

Re: [RFA] [PATCH] Add a warning for invalid function casts

[Joseph Myers]

On Tue, 3 Oct 2017, Bernd Edlinger wrote:

> invalid, also if both function types have a non-null TYPE_ARG_TYPES
> I would say this deserves a warning.  As an exception I have

I'm not convinced by the TYPE_ARG_TYPES check, at least for C.

In C, unprototyped function types are not compatible with variadic 
function types or functions with argument types changed by default 
argument promotions (that is, int () and int (char) and int (int, ...) are 
all incompatible).  I'd think it appropriate to warn about such 
conversions, given that they are cases where calling the converted 
function has undefined behavior.

There may well be cases of interfaces where void (*) (void) is used as a 
generic function pointer type (always converted to / from the actual type 
of the function in question), for which this warning would not be 
suitable.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [RFA] [PATCH] Add a warning for invalid function casts

[Bernd Edlinger]

On 10/03/17 23:34, Joseph Myers wrote:
> On Tue, 3 Oct 2017, Bernd Edlinger wrote:
> 
>> invalid, also if both function types have a non-null TYPE_ARG_TYPES
>> I would say this deserves a warning.  As an exception I have
> 
> I'm not convinced by the TYPE_ARG_TYPES check, at least for C.
> 

I will drop that, for C and C++, and try to get it working without
that kludge.

> In C, unprototyped function types are not compatible with variadic
> function types or functions with argument types changed by default
> argument promotions (that is, int () and int (char) and int (int, ...) are
> all incompatible).  I'd think it appropriate to warn about such
> conversions, given that they are cases where calling the converted
> function has undefined behavior.
> 

Right, interesting is that this does not produce a warning,

int test(int);
int foo()
{

   int (*x)();
   x = test;
   return x(1);
}


while the following example produces a warning:

int test(int,...);
int foo()
{

   int (*x)(int);
   x = test;
   return x(1);
}

gcc -Wall -W -S t1.c
t1.c: In function 'foo':
t1.c:6:5: warning: assignment to 'int (*)(int)' from incompatible 
pointer type 'int (*)(int)' [-Wincompatible-pointer-types]
    x = test;
      ^

I will send a patch that adds ", ..." to the parameter list
in the diagnostics...

But why is int(*)(int) compatible to (int)(*)() but not
to int(*)(int,...) ?


> There may well be cases of interfaces where void (*) (void) is used as a
> generic function pointer type (always converted to / from the actual type
> of the function in question), for which this warning would not be
> suitable.
> 

Yes, those would get a warning, or have to add a cast to uintptr_t, if
it is not possible to fix the code otherwise.  libffi does this...


Bernd.

Re: [RFA] [PATCH] Add a warning for invalid function casts

[Joseph Myers]

On Thu, 5 Oct 2017, Bernd Edlinger wrote:

> But why is int(*)(int) compatible to (int)(*)() but not
> to int(*)(int,...) ?

I think it's a matter of what function types were possible in K&R C.  
<stdarg.h> variadic types weren't (there was an older <varargs.h>), and 
neither were types with arguments of type float or narrower-than-int 
integers (because those always got promoted to a wider type when passed as 
function arguments to an unprototyped function).  And those types that 
were impossible in K&R C always require function prototypes.  (The 
possibility of function types without a prototype is a legacy feature.  
There was some suggestion of removing it for C11, but no-one ever produced 
a paper for WG14 proposing the actual wording changes that would have been 
needed.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [RFA] [PATCH] Add a warning for invalid function casts

[Eric Gallager]

On Tue, Oct 3, 2017 at 3:33 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi!
>
> I have implemented a warning -Wcast-function-type that analyzes
> type casts which change the function signatures.
>
> I would consider function pointers with different result type
> invalid, also if both function types have a non-null TYPE_ARG_TYPES
> I would say this deserves a warning.  As an exception I have
> used for instance in recog.h, the warning allows casting
> a function with the type typedef rtx (*stored_funcptr) (...);
> to any function with the same result type.
>
> I would think a warning like that should be enabled with -Wextra.
>
> Attached is a first version of the patch and as you can see
> the warning found already lots of suspicious type casts.  The worst
> is the splay-tree which always calls functions with uintptr_t
> instead of the correct parameter type.  I was unable to find
> a solution for this, and just silenced the warning with a
> second type-cast.
>
> Note that I also changed one line in libgo, but that is only
> a quick hack which I only did to make the boot-strap with
> all languages succeed.
>
> I'm not sure if this warning may be a bit too strict, but I think
> so far it just triggered on rather questionable code.
>
> Thoughts?
>
>
> Bernd.

Sorry if this is a stupid question, but could you explain how this
warning is different from -Wbad-function-cast? Something about direct
calls to functions vs. passing them as function pointers?

Re: [RFA] [PATCH] Add a warning for invalid function casts

[Bernd Edlinger]

On 10/05/17 02:24, Eric Gallager wrote:
> Sorry if this is a stupid question, but could you explain how this
> warning is different from -Wbad-function-cast? Something about direct
> calls to functions vs. passing them as function pointers?

No, it is not :)

-Wbad-function-cast is IMHO a strange legacy warning.

It is C-only, and it triggers only if the result of a function call
is cast to a type with a different TREE_CODE, so for instance
int <-> float <-> pointer.

It would trigger for perfectly valid code like this:

i = (int) floor (f);

while we have no warning for

i = floor (f);

What I want to diagnose is assigning a function pointer via an explicit
type cast to another function pointer, when there is no possible
implicit conversion between the two function pointer types.
Thus the cast was used to silence a warning/error in the first place.

The idea for this warning came up when someone spotted a place in
openssl, where a type cast was used to change the return value of a
callback function from long to int:

https://github.com/openssl/openssl/issues/4413

But due to the type cast there was never ever any warning from this
invalid type cast.


Bernd.

Re: [RFA] [PATCH] Add a warning for invalid function casts

[Andreas Schwab]

On Okt 05 2017, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:

> The idea for this warning came up when someone spotted a place in
> openssl, where a type cast was used to change the return value of a
> callback function from long to int:
>
> https://github.com/openssl/openssl/issues/4413
>
> But due to the type cast there was never ever any warning from this
> invalid type cast.

Note that the type cast itself is perfectly valid (all function pointers
are alike), but it's the call site that is problematic: unless it casts
back to the real type of the function before the call it is causing
undefined behviour.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [RFA] [PATCH] Add a warning for invalid function casts

[Eric Gallager]

On 10/5/17, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> On 10/05/17 02:24, Eric Gallager wrote:
>> Sorry if this is a stupid question, but could you explain how this
>> warning is different from -Wbad-function-cast? Something about direct
>> calls to functions vs. passing them as function pointers?
>
> No, it is not :)
>
> -Wbad-function-cast is IMHO a strange legacy warning.
>
> It is C-only, and it triggers only if the result of a function call
> is cast to a type with a different TREE_CODE, so for instance
> int <-> float <-> pointer.
>
> It would trigger for perfectly valid code like this:
>
> i = (int) floor (f);
>
> while we have no warning for
>
> i = floor (f);
>
> What I want to diagnose is assigning a function pointer via an explicit
> type cast to another function pointer, when there is no possible
> implicit conversion between the two function pointer types.
> Thus the cast was used to silence a warning/error in the first place.

OK, thanks for explaining! I kinda worry about warning on code that
was added to silence other warnings in the first place, as it can lead
to frustration when making a fix expecting the number of warnings when
compiling to decrease, but then they don't actually decrease. But as
long as there's a simple way to fix the warned-on code that silences
both the original warning being avoided, and this new one, I see how
it'll be useful.

>
> The idea for this warning came up when someone spotted a place in
> openssl, where a type cast was used to change the return value of a
> callback function from long to int:
>
> https://github.com/openssl/openssl/issues/4413
>
> But due to the type cast there was never ever any warning from this
> invalid type cast.
>
>
> Bernd.
>

Thanks for working on this!

Eric

Re: [RFA] [PATCH] Add a warning for invalid function casts

[Martin Sebor]

On 10/03/2017 01:33 PM, Bernd Edlinger wrote:
> Hi!
>
> I have implemented a warning -Wcast-function-type that analyzes
> type casts which change the function signatures.
>
> I would consider function pointers with different result type
> invalid, also if both function types have a non-null TYPE_ARG_TYPES
> I would say this deserves a warning.  As an exception I have
> used for instance in recog.h, the warning allows casting
> a function with the type typedef rtx (*stored_funcptr) (...);
> to any function with the same result type.
>
> I would think a warning like that should be enabled with -Wextra.
>
> Attached is a first version of the patch and as you can see
> the warning found already lots of suspicious type casts.  The worst
> is the splay-tree which always calls functions with uintptr_t
> instead of the correct parameter type.  I was unable to find
> a solution for this, and just silenced the warning with a
> second type-cast.
>
> Note that I also changed one line in libgo, but that is only
> a quick hack which I only did to make the boot-strap with
> all languages succeed.
>
> I'm not sure if this warning may be a bit too strict, but I think
> so far it just triggered on rather questionable code.
>
> Thoughts?

My initial thought is that although casts between incompatible
function types undoubtedly mask bugs, the purpose of such casts
is to make such conversions possible.  Indiscriminately diagnosing
them would essentially eliminate this feature of the type system.
Having to add another cast to a different type(*) to suppress
the warning when such a conversion is intended doesn't seem like
a good solution (the next logical step to find bugs in those
conversions would then be to add another warning to see through
those additional casts, and so on).

With that, my question is: under what circumstances does the
warning not trigger on a cast to a function of an incompatible
type?

In my (very quick) tests the warning appears to trigger on all
strictly incompatible conversions, even if they are otherwise
benign, such as:

   int f (const int*);
   typedef int F (int*);

   F* pf1 = f;        // -Wincompatible-pointer-types
   F* pf2 = (F*)f;    // -Wcast-function-type

Likewise by:

   int f (signed char);
   typedef int F (unsigned char);

   F* pf = (F*)f;    // -Wcast-function-type

I'd expect these conversions to be useful and would view warning
for them with -Wextra as excessive.  In fact, I'm not sure I see
the benefit of warning on these casts under any circumstances.

Similarly, for casts between pointers to the same integer type
with a different sign, or those involving ILP32/LP64 portability
issues I'd expect the warning not to trigger unless requested
(e.g., by some other option targeting those issues).

So based on these initial observations and despite the bugs it
may have uncovered, I share your concern that the warning in
its present form is too strict to be suitable for -Wextra, or
possibly even too noisy to be of practical use on its own and
outside of -Wextra.

Out of curiosity, have you done any tests on other code bases
besides GCC to see how many issues (true and false positives)
it finds?

Martin

[*] Strictly speaking, the semantics of casting a function
pointer to intptr_t aren't necessarily well-defined.  Only void*
can be portably converted to intptr_t and back, and only object
pointers are required to be convertible to void* and back.  And
although GCC defines the semantics of these conversions, forcing
programs to abandon a well defined language feature in favor of
one that's not as cleanly specified would undermine the goal
the warning is meant to achieve.

Re: [RFA] [PATCH] Add a warning for invalid function casts

[Bernd Edlinger]

On 10/05/17 18:16, Martin Sebor wrote:
> On 10/03/2017 01:33 PM, Bernd Edlinger wrote:
>>
>> I'm not sure if this warning may be a bit too strict, but I think
>> so far it just triggered on rather questionable code.
>>
>> Thoughts?
> 
> My initial thought is that although casts between incompatible
> function types undoubtedly mask bugs, the purpose of such casts
> is to make such conversions possible.  Indiscriminately diagnosing
> them would essentially eliminate this feature of the type system.
> Having to add another cast to a different type(*) to suppress
> the warning when such a conversion is intended doesn't seem like
> a good solution (the next logical step to find bugs in those
> conversions would then be to add another warning to see through
> those additional casts, and so on).
> 
> With that, my question is: under what circumstances does the
> warning not trigger on a cast to a function of an incompatible
> type?
> 
> In my (very quick) tests the warning appears to trigger on all
> strictly incompatible conversions, even if they are otherwise
> benign, such as:
> 
>    int f (const int*);
>    typedef int F (int*);
> 
>    F* pf1 = f;        // -Wincompatible-pointer-types
>    F* pf2 = (F*)f;    // -Wcast-function-type
> 
> Likewise by:
> 
>    int f (signed char);
>    typedef int F (unsigned char);
> 
>    F* pf = (F*)f;    // -Wcast-function-type
> 
> I'd expect these conversions to be useful and would view warning
> for them with -Wextra as excessive.  In fact, I'm not sure I see
> the benefit of warning on these casts under any circumstances.
> 
> Similarly, for casts between pointers to the same integer type
> with a different sign, or those involving ILP32/LP64 portability
> issues I'd expect the warning not to trigger unless requested
> (e.g., by some other option targeting those issues).
> 
> So based on these initial observations and despite the bugs it
> may have uncovered, I share your concern that the warning in
> its present form is too strict to be suitable for -Wextra, or
> possibly even too noisy to be of practical use on its own and
> outside of -Wextra.
> 
> Out of curiosity, have you done any tests on other code bases
> besides GCC to see how many issues (true and false positives)
> it finds?
> 
> Martin
> 
> [*] Strictly speaking, the semantics of casting a function
> pointer to intptr_t aren't necessarily well-defined.  Only void*
> can be portably converted to intptr_t and back, and only object
> pointers are required to be convertible to void* and back.  And
> although GCC defines the semantics of these conversions, forcing
> programs to abandon a well defined language feature in favor of
> one that's not as cleanly specified would undermine the goal
> the warning is meant to achieve.

Thanks for your advice.

I did look into openssl and linux, both have plenty of
-Wcast-function-type warnings.

In the case of openssl it is lots of similar stuff
crypto/aes/aes_cfb.c:25:27: warning: cast between incompatible function 
types from 'void (*)(const unsigned char *, unsigned char *, const
AES_KEY *) {aka void (*)(const unsigned char *, unsigned char *, const 
struct aes_key_st *)}' to 'void (*)(const unsigned char *, unsigned
char *, const void *)' [-Wcast-function-type]
                            (block128_f) AES_encrypt);

The problem is the function is of course called with a different
signature than what is declared.  They take it somehow for granted,
that "void*" or "const void*" parameter are an alias for
any pointer or any const pointer.  Either as parameter or as return
code.

I would believe this is not well-defined by the c-standard.

But it makes the warning less useful because it would be impossible
to spot the few places where the call will actually abort at
runtime.

Then I tried to compile linux, I noticed that there is a new
warning for the alias to incompatible function.

I saw it very often, and it is always when a system call
is defined:

./include/linux/compat.h:50:18: Warnung: »compat_sys_ptrace« alias 
between functions of incompatible types »long int(compat_long_t, 
compat_long_t,  compat_long_t,  compat_long_t) {alias long int(int, 
int,  int,  int)}« and »long int(long int,  long int,  long int,  long 
int)« [-Wattributes]
   asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))\

./include/linux/syscalls.h:211:18: Warnung: »sys_rt_sigprocmask« alias 
between functions of incompatible types »long int(int,  sigset_t *, 
sigset_t *, size_t) {alias long int(int,  struct <anonym> *, struct 
<anonym> *, long unsigned int)}« and »long int(long int,  long int, 
long int,  long int)« [-Wattributes]
   asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \


Needless to say that even more Wcast-function-type warning
happen.

./include/linux/timer.h:178:23: Warnung: cast between incompatible 
function types from »void (*)(struct timer_list *)« to »void (*)(long 
unsigned int)« [-Wcast-function-type]
   __setup_timer(timer, (TIMER_FUNC_TYPE)callback,

So they assume obviously that any int / long / pointer value
are compatible with uintptr_t and intptr_t.

If the Wattribute stays this way, I can garantee that Linus will simply
add -Wno-attribute to the linux makefiles, which is bad because
-Wattribute is more than one warning alone.

At the least we should rename this particular warning into
something else, so that it is not necessary to disable everything
when only this specific warning is too hard to fix.

Maybe it would be good to not warn in type-casts, when they can be
assumed to be safe, for instance
void* <-> any pointer (parameter or result),
uintptr_t <-> any int, any pointer (parameter or result),
void (*) (void) and void (*) (...) <-> any function pointer.

I think that applies to both warnings.

What do you think?


Bernd.

Re: [RFA] [PATCH] Add a warning for invalid function casts

[Joseph Myers]

On Thu, 5 Oct 2017, Bernd Edlinger wrote:

> Maybe it would be good to not warn in type-casts, when they can be
> assumed to be safe, for instance
> void* <-> any pointer (parameter or result),
> uintptr_t <-> any int, any pointer (parameter or result),
> void (*) (void) and void (*) (...) <-> any function pointer.

Well, void * and uintptr_t aren't necessarily interchangable at the ABI 
level.  At least, the m68k ABI returns integers in %d0 and pointers in 
%a0; I don't know if any other ABIs have that peculiarity.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [RFA] [PATCH] Add a warning for invalid function casts

[Jeff Law]

On 10/05/2017 03:47 PM, Joseph Myers wrote:
> On Thu, 5 Oct 2017, Bernd Edlinger wrote:
> 
>> Maybe it would be good to not warn in type-casts, when they can be
>> assumed to be safe, for instance
>> void* <-> any pointer (parameter or result),
>> uintptr_t <-> any int, any pointer (parameter or result),
>> void (*) (void) and void (*) (...) <-> any function pointer.
> 
> Well, void * and uintptr_t aren't necessarily interchangable at the ABI 
> level.  At least, the m68k ABI returns integers in %d0 and pointers in 
> %a0; I don't know if any other ABIs have that peculiarity.
> 
The mn103 (live) and mn102 (dead) probably do.  But my memory is getting
fuzzy on those.

jeff

Re: [RFA] [PATCH] Add a warning for invalid function casts

[Martin Sebor]

On 10/05/2017 03:04 PM, Bernd Edlinger wrote:
> On 10/05/17 18:16, Martin Sebor wrote:
>> On 10/03/2017 01:33 PM, Bernd Edlinger wrote:
>>>
>>> I'm not sure if this warning may be a bit too strict, but I think
>>> so far it just triggered on rather questionable code.
>>>
>>> Thoughts?
>>
>> My initial thought is that although casts between incompatible
>> function types undoubtedly mask bugs, the purpose of such casts
>> is to make such conversions possible.  Indiscriminately diagnosing
>> them would essentially eliminate this feature of the type system.
>> Having to add another cast to a different type(*) to suppress
>> the warning when such a conversion is intended doesn't seem like
>> a good solution (the next logical step to find bugs in those
>> conversions would then be to add another warning to see through
>> those additional casts, and so on).
>>
>> With that, my question is: under what circumstances does the
>> warning not trigger on a cast to a function of an incompatible
>> type?
>>
>> In my (very quick) tests the warning appears to trigger on all
>> strictly incompatible conversions, even if they are otherwise
>> benign, such as:
>>
>>    int f (const int*);
>>    typedef int F (int*);
>>
>>    F* pf1 = f;        // -Wincompatible-pointer-types
>>    F* pf2 = (F*)f;    // -Wcast-function-type
>>
>> Likewise by:
>>
>>    int f (signed char);
>>    typedef int F (unsigned char);
>>
>>    F* pf = (F*)f;    // -Wcast-function-type
>>
>> I'd expect these conversions to be useful and would view warning
>> for them with -Wextra as excessive.  In fact, I'm not sure I see
>> the benefit of warning on these casts under any circumstances.
>>
>> Similarly, for casts between pointers to the same integer type
>> with a different sign, or those involving ILP32/LP64 portability
>> issues I'd expect the warning not to trigger unless requested
>> (e.g., by some other option targeting those issues).
>>
>> So based on these initial observations and despite the bugs it
>> may have uncovered, I share your concern that the warning in
>> its present form is too strict to be suitable for -Wextra, or
>> possibly even too noisy to be of practical use on its own and
>> outside of -Wextra.
>>
>> Out of curiosity, have you done any tests on other code bases
>> besides GCC to see how many issues (true and false positives)
>> it finds?
>>
>> Martin
>>
>> [*] Strictly speaking, the semantics of casting a function
>> pointer to intptr_t aren't necessarily well-defined.  Only void*
>> can be portably converted to intptr_t and back, and only object
>> pointers are required to be convertible to void* and back.  And
>> although GCC defines the semantics of these conversions, forcing
>> programs to abandon a well defined language feature in favor of
>> one that's not as cleanly specified would undermine the goal
>> the warning is meant to achieve.
>
> Thanks for your advice.
>
> I did look into openssl and linux, both have plenty of
> -Wcast-function-type warnings.
>
> In the case of openssl it is lots of similar stuff
> crypto/aes/aes_cfb.c:25:27: warning: cast between incompatible function
> types from 'void (*)(const unsigned char *, unsigned char *, const
> AES_KEY *) {aka void (*)(const unsigned char *, unsigned char *, const
> struct aes_key_st *)}' to 'void (*)(const unsigned char *, unsigned
> char *, const void *)' [-Wcast-function-type]
>                             (block128_f) AES_encrypt);
>
> The problem is the function is of course called with a different
> signature than what is declared.  They take it somehow for granted,
> that "void*" or "const void*" parameter are an alias for
> any pointer or any const pointer.  Either as parameter or as return
> code.
>
> I would believe this is not well-defined by the c-standard.
>
> But it makes the warning less useful because it would be impossible
> to spot the few places where the call will actually abort at
> runtime.
>
> Then I tried to compile linux, I noticed that there is a new
> warning for the alias to incompatible function.
>
> I saw it very often, and it is always when a system call
> is defined:
>
> ./include/linux/compat.h:50:18: Warnung: »compat_sys_ptrace« alias
> between functions of incompatible types »long int(compat_long_t,
> compat_long_t,  compat_long_t,  compat_long_t) {alias long int(int,
> int,  int,  int)}« and »long int(long int,  long int,  long int,  long
> int)« [-Wattributes]
>    asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))\
>
> ./include/linux/syscalls.h:211:18: Warnung: »sys_rt_sigprocmask« alias
> between functions of incompatible types »long int(int,  sigset_t *,
> sigset_t *, size_t) {alias long int(int,  struct <anonym> *, struct
> <anonym> *, long unsigned int)}« and »long int(long int,  long int,
> long int,  long int)« [-Wattributes]
>    asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
>
>
> Needless to say that even more Wcast-function-type warning
> happen.
>
> ./include/linux/timer.h:178:23: Warnung: cast between incompatible
> function types from »void (*)(struct timer_list *)« to »void (*)(long
> unsigned int)« [-Wcast-function-type]
>    __setup_timer(timer, (TIMER_FUNC_TYPE)callback,
>
> So they assume obviously that any int / long / pointer value
> are compatible with uintptr_t and intptr_t.
>
> If the Wattribute stays this way, I can garantee that Linus will simply
> add -Wno-attribute to the linux makefiles, which is bad because
> -Wattribute is more than one warning alone.
>
> At the least we should rename this particular warning into
> something else, so that it is not necessary to disable everything
> when only this specific warning is too hard to fix.
>
> Maybe it would be good to not warn in type-casts, when they can be
> assumed to be safe, for instance
> void* <-> any pointer (parameter or result),
> uintptr_t <-> any int, any pointer (parameter or result),
> void (*) (void) and void (*) (...) <-> any function pointer.
>
> I think that applies to both warnings.
>
> What do you think?

The attribute alias/ifunc incompatibilities are conflicts between
declarations of the same symbol.  Such conflicts are invalid in C
and ordinarily rejected with a hard error.  With the exception of
ifunc resolvers returning void*, I see the alias/ifunc conflicts
as signs of bugs or, at best, benign mistakes.  I also can think
of no reason why they shouldn't in most cases be corrected to
conform to the C requirements.  Where they can't be fixed it's
usually easy to suppress the warnings by declaring the functions
without a prototype.  That also makes it clear to the reader that
type checking has been suppressed.

On the other hand, explicit casts between incompatible pointers
are a well-defined feature of the language and their use a clear
indication that the programmer was aware of the incompatibility
and intentionally suppressed it.  There no doubt are bugs among
these that would be helpful to find so I think the challenge
here is to distinguish the intentional and benign conversions
from the others while minimizing false positives.  Since there
are likely to be a large proportion of the intentional and benign
cases, unless the heuristic to find the others is very good, there
should also be an easy way to suppress the false positives without
compromising readability.

Martin

PS We are discussing the kernel warnings in bug 82435.  I've
already moved them under -Wincompatible-pointer-types but it
sounds like a new option altogether might be preferable.
Ideally, though, if it's possible it would be best to fix them
like we did for Glibc so the kernel too would benefit from the
better type checking.  I should look into that.

Re: [RFA] [PATCH] Add a warning for invalid function casts

[Bernd Edlinger]

On 10/05/17 18:16, Martin Sebor wrote:
> In my (very quick) tests the warning appears to trigger on all
> strictly incompatible conversions, even if they are otherwise
> benign, such as:
> 
>    int f (const int*);
>    typedef int F (int*);
> 
>    F* pf1 = f;        // -Wincompatible-pointer-types
>    F* pf2 = (F*)f;    // -Wcast-function-type
> 
> Likewise by:
> 
>    int f (signed char);
>    typedef int F (unsigned char);
> 
>    F* pf = (F*)f;    // -Wcast-function-type
> 
> I'd expect these conversions to be useful and would view warning
> for them with -Wextra as excessive.  In fact, I'm not sure I see
> the benefit of warning on these casts under any circumstances.
> 

Well, while the first example should be safe,
the second one is probably not safe:

Because the signed and unsigned char are promoted to int,
by the ABI but one is in the range -128..127 while the
other is in the range 0..255, right?


Bernd.

Re: [RFA] [PATCH] Add a warning for invalid function casts

[Martin Sebor]

On 10/06/2017 07:25 AM, Bernd Edlinger wrote:
> On 10/05/17 18:16, Martin Sebor wrote:
>> In my (very quick) tests the warning appears to trigger on all
>> strictly incompatible conversions, even if they are otherwise
>> benign, such as:
>>
>>    int f (const int*);
>>    typedef int F (int*);
>>
>>    F* pf1 = f;        // -Wincompatible-pointer-types
>>    F* pf2 = (F*)f;    // -Wcast-function-type
>>
>> Likewise by:
>>
>>    int f (signed char);
>>    typedef int F (unsigned char);
>>
>>    F* pf = (F*)f;    // -Wcast-function-type
>>
>> I'd expect these conversions to be useful and would view warning
>> for them with -Wextra as excessive.  In fact, I'm not sure I see
>> the benefit of warning on these casts under any circumstances.
>>
>
> Well, while the first example should be safe,
> the second one is probably not safe:
>
> Because the signed and unsigned char are promoted to int,
> by the ABI but one is in the range -128..127 while the
> other is in the range 0..255, right?

Right.  The cast is always safe but whether or not a call to such
a function through the incompatible pointer is also safe depends
on the definition of the function (and on the caller).  If the
function uses just the low bits of the argument then it's most
likely fine for any argument.  If the caller only calls it with
values in the 7-bit range (e.g., the ASCII subset) then it's also
fine.  Otherwise there's the potential for the problem you pointed
out.  (Similarly, if in the first example I gave the cast added
constness to the argument rather than removing it and the function
modified the pointed-to object calling it through the incompatible
pointer on a constant object would also be unsafe.)

Another class of cases to consider are casts between functions
taking pointers to different but related structs.  Code like this
could be written to mimic C++ calling a base class function on
a derived object.

   struct Base { ... };
   struct Derived { Base b; ... };

   typedef void F (Derived*);

   void foo (Base*);

   F* pf = (F*)foo;

Martin

Re: [RFA] [PATCH] Add a warning for invalid function casts

[Bernd Edlinger]

On 10/06/17 17:43, Martin Sebor wrote:
> On 10/06/2017 07:25 AM, Bernd Edlinger wrote:
>> On 10/05/17 18:16, Martin Sebor wrote:
>>> In my (very quick) tests the warning appears to trigger on all
>>> strictly incompatible conversions, even if they are otherwise
>>> benign, such as:
>>>
>>>    int f (const int*);
>>>    typedef int F (int*);
>>>
>>>    F* pf1 = f;        // -Wincompatible-pointer-types
>>>    F* pf2 = (F*)f;    // -Wcast-function-type
>>>
>>> Likewise by:
>>>
>>>    int f (signed char);
>>>    typedef int F (unsigned char);
>>>
>>>    F* pf = (F*)f;    // -Wcast-function-type
>>>
>>> I'd expect these conversions to be useful and would view warning
>>> for them with -Wextra as excessive.  In fact, I'm not sure I see
>>> the benefit of warning on these casts under any circumstances.
>>>
>>
>> Well, while the first example should be safe,
>> the second one is probably not safe:
>>
>> Because the signed and unsigned char are promoted to int,
>> by the ABI but one is in the range -128..127 while the
>> other is in the range 0..255, right?
> 
> Right.  The cast is always safe but whether or not a call to such
> a function through the incompatible pointer is also safe depends
> on the definition of the function (and on the caller).  If the
> function uses just the low bits of the argument then it's most
> likely fine for any argument.  If the caller only calls it with
> values in the 7-bit range (e.g., the ASCII subset) then it's also
> fine.  Otherwise there's the potential for the problem you pointed
> out.  (Similarly, if in the first example I gave the cast added
> constness to the argument rather than removing it and the function
> modified the pointed-to object calling it through the incompatible
> pointer on a constant object would also be unsafe.)
> 
> Another class of cases to consider are casts between functions
> taking pointers to different but related structs.  Code like this
> could be written to mimic C++ calling a base class function on
> a derived object.
> 
>    struct Base { ... };
>    struct Derived { Base b; ... };
> 
>    typedef void F (Derived*);
> 
>    void foo (Base*);
> 
>    F* pf = (F*)foo;
> 

Hmm, yes.

I start to believe, that this warning should treat all pointers
as equivalent, but everything else need to be of the same type.
That would at least cover the majority of all "safe" use cases.

And I need a way to by-pass the warning with a generic function
pointer type.  uintptr_t is not the right choice, as you pointed
out already.

But I also see problems with "int (*) ()" as a escape mechanism
because this declaration creates a warning in C with
-Wstrict-prototypes, and in C++ this syntax means "int (*) (void)"
while the C++ type "int (*) (...)" is rejected by the C front end.

I start to believe that the type "void (*) (void)" is better suited for
this purpose, and it is already used in many programs as a type-less
wildcard function type.  I see examples in libgo and libffi at least.



Bernd.

Re: [RFA] [PATCH] Add a warning for invalid function casts

[Martin Sebor]

On 10/06/2017 12:06 PM, Bernd Edlinger wrote:
> On 10/06/17 17:43, Martin Sebor wrote:
>> On 10/06/2017 07:25 AM, Bernd Edlinger wrote:
>>> On 10/05/17 18:16, Martin Sebor wrote:
>>>> In my (very quick) tests the warning appears to trigger on all
>>>> strictly incompatible conversions, even if they are otherwise
>>>> benign, such as:
>>>>
>>>>    int f (const int*);
>>>>    typedef int F (int*);
>>>>
>>>>    F* pf1 = f;        // -Wincompatible-pointer-types
>>>>    F* pf2 = (F*)f;    // -Wcast-function-type
>>>>
>>>> Likewise by:
>>>>
>>>>    int f (signed char);
>>>>    typedef int F (unsigned char);
>>>>
>>>>    F* pf = (F*)f;    // -Wcast-function-type
>>>>
>>>> I'd expect these conversions to be useful and would view warning
>>>> for them with -Wextra as excessive.  In fact, I'm not sure I see
>>>> the benefit of warning on these casts under any circumstances.
>>>>
>>>
>>> Well, while the first example should be safe,
>>> the second one is probably not safe:
>>>
>>> Because the signed and unsigned char are promoted to int,
>>> by the ABI but one is in the range -128..127 while the
>>> other is in the range 0..255, right?
>>
>> Right.  The cast is always safe but whether or not a call to such
>> a function through the incompatible pointer is also safe depends
>> on the definition of the function (and on the caller).  If the
>> function uses just the low bits of the argument then it's most
>> likely fine for any argument.  If the caller only calls it with
>> values in the 7-bit range (e.g., the ASCII subset) then it's also
>> fine.  Otherwise there's the potential for the problem you pointed
>> out.  (Similarly, if in the first example I gave the cast added
>> constness to the argument rather than removing it and the function
>> modified the pointed-to object calling it through the incompatible
>> pointer on a constant object would also be unsafe.)
>>
>> Another class of cases to consider are casts between functions
>> taking pointers to different but related structs.  Code like this
>> could be written to mimic C++ calling a base class function on
>> a derived object.
>>
>>    struct Base { ... };
>>    struct Derived { Base b; ... };
>>
>>    typedef void F (Derived*);
>>
>>    void foo (Base*);
>>
>>    F* pf = (F*)foo;
>>
>
> Hmm, yes.
>
> I start to believe, that this warning should treat all pointers
> as equivalent, but everything else need to be of the same type.
> That would at least cover the majority of all "safe" use cases.

Perhaps basing the warning on some form of structural equivalence
between function arguments might be useful.  For instance, in
ILP32, casting between 'int foo (int)' and 'long foo (long)' and
calling the function is probably generally considered safe (even
though it's undefined by the language) and works as people expect
so avoiding the warning there would help keep the false positive
rate down. (Something like this might also work for the kernel
alias macros.)

Similarly, casting between a function that returns a scalar smaller
than int and int and then calling it is probably safe (or maybe
even long, depending on the ABI).

Casting a function returning a value to one returning void and
calling it through the result should always be safe.

I would also suggest to consider disregarding qualifiers as those
are often among the reasons for intentional casts (e.g., when
mixing a legacy API and a more modern const-correct one).

Casts are also not uncommon between variadic and ordinary function
types so some heuristic might be appropriate there.

>
> And I need a way to by-pass the warning with a generic function
> pointer type.  uintptr_t is not the right choice, as you pointed
> out already.
>
> But I also see problems with "int (*) ()" as a escape mechanism
> because this declaration creates a warning in C with
> -Wstrict-prototypes, and in C++ this syntax means "int (*) (void)"
> while the C++ type "int (*) (...)" is rejected by the C front end.

I wouldn't consider it a problem if the suppression mechanism were
different between languages.  Most such casts are going to be in
source files (as opposed to C headers) so it should be fine to use
each language's unique form of function without a prototype.

Martin

Re: [RFA] [PATCH] Add a warning for invalid function casts

[Jeff Law]

On 10/06/2017 09:43 AM, Martin Sebor wrote:
> On 10/06/2017 07:25 AM, Bernd Edlinger wrote:
>> On 10/05/17 18:16, Martin Sebor wrote:
>>> In my (very quick) tests the warning appears to trigger on all
>>> strictly incompatible conversions, even if they are otherwise
>>> benign, such as:
>>>
>>>    int f (const int*);
>>>    typedef int F (int*);
>>>
>>>    F* pf1 = f;        // -Wincompatible-pointer-types
>>>    F* pf2 = (F*)f;    // -Wcast-function-type
>>>
>>> Likewise by:
>>>
>>>    int f (signed char);
>>>    typedef int F (unsigned char);
>>>
>>>    F* pf = (F*)f;    // -Wcast-function-type
>>>
>>> I'd expect these conversions to be useful and would view warning
>>> for them with -Wextra as excessive.  In fact, I'm not sure I see
>>> the benefit of warning on these casts under any circumstances.
>>>
>>
>> Well, while the first example should be safe,
>> the second one is probably not safe:
>>
>> Because the signed and unsigned char are promoted to int,
>> by the ABI but one is in the range -128..127 while the
>> other is in the range 0..255, right?
> 
> Right.  The cast is always safe but whether or not a call to such
> a function through the incompatible pointer is also safe depends
> on the definition of the function (and on the caller).  If the
> function uses just the low bits of the argument then it's most
> likely fine for any argument.  If the caller only calls it with
> values in the 7-bit range (e.g., the ASCII subset) then it's also
> fine.  Otherwise there's the potential for the problem you pointed
> out.  (Similarly, if in the first example I gave the cast added
> constness to the argument rather than removing it and the function
> modified the pointed-to object calling it through the incompatible
> pointer on a constant object would also be unsafe.)
> 
> Another class of cases to consider are casts between functions
> taking pointers to different but related structs.  Code like this
> could be written to mimic C++ calling a base class function on
> a derived object.
> 
>   struct Base { ... };
>   struct Derived { Base b; ... };
> 
>   typedef void F (Derived*);
> 
>   void foo (Base*);
> 
>   F* pf = (F*)foo;
Yea.  And one might even find such code in BFD.  It certainly mimicks
C++ base and derived classes using C, so it has significant potential to
have this kind of code.
jeff

Re: [RFA] [PATCH] Add a warning for invalid function casts

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 2884 bytes --]

On 10/06/17 22:50, Jeff Law wrote:
> On 10/06/2017 09:43 AM, Martin Sebor wrote:
>> On 10/06/2017 07:25 AM, Bernd Edlinger wrote:
>>> On 10/05/17 18:16, Martin Sebor wrote:
>>>> In my (very quick) tests the warning appears to trigger on all
>>>> strictly incompatible conversions, even if they are otherwise
>>>> benign, such as:
>>>>
>>>>     int f (const int*);
>>>>     typedef int F (int*);
>>>>
>>>>     F* pf1 = f;        // -Wincompatible-pointer-types
>>>>     F* pf2 = (F*)f;    // -Wcast-function-type
>>>>
>>>> Likewise by:
>>>>
>>>>     int f (signed char);
>>>>     typedef int F (unsigned char);
>>>>
>>>>     F* pf = (F*)f;    // -Wcast-function-type
>>>>
>>>> I'd expect these conversions to be useful and would view warning
>>>> for them with -Wextra as excessive.  In fact, I'm not sure I see
>>>> the benefit of warning on these casts under any circumstances.
>>>>
>>>
>>> Well, while the first example should be safe,
>>> the second one is probably not safe:
>>>
>>> Because the signed and unsigned char are promoted to int,
>>> by the ABI but one is in the range -128..127 while the
>>> other is in the range 0..255, right?
>>
>> Right.  The cast is always safe but whether or not a call to such
>> a function through the incompatible pointer is also safe depends
>> on the definition of the function (and on the caller).  If the
>> function uses just the low bits of the argument then it's most
>> likely fine for any argument.  If the caller only calls it with
>> values in the 7-bit range (e.g., the ASCII subset) then it's also
>> fine.  Otherwise there's the potential for the problem you pointed
>> out.  (Similarly, if in the first example I gave the cast added
>> constness to the argument rather than removing it and the function
>> modified the pointed-to object calling it through the incompatible
>> pointer on a constant object would also be unsafe.)
>>
>> Another class of cases to consider are casts between functions
>> taking pointers to different but related structs.  Code like this
>> could be written to mimic C++ calling a base class function on
>> a derived object.
>>
>>    struct Base { ... };
>>    struct Derived { Base b; ... };
>>
>>    typedef void F (Derived*);
>>
>>    void foo (Base*);
>>
>>    F* pf = (F*)foo;
> Yea.  And one might even find such code in BFD.  It certainly mimicks
> C++ base and derived classes using C, so it has significant potential to
> have this kind of code.
> jeff
> 

Yes, absolutely.

This use case makes up 99% of all places where I saw the warning until
now.  I will try to ignore all pointer types and see how that works out
on some code bases I have access to.

When that works as expected we should be able to see what heuristics
need to be added next.

FYI I have attached what I am currently bootstrapping.


Bernd.

[-- Attachment #2: changelog-cast-function-type.txt --]
[-- Type: text/plain, Size: 1163 bytes --]

gcc:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* doc/invoke.texi: Document -Wcast-function-type.
	* recog.h (stored_funcptr): Change signature.
	* tree-dump.c (dump_node): Avoid warning.
	* typed-splay-tree.h (typed_splay_tree): Avoid warning.

libcpp:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* internal.h (maybe_print_line): Change signature.
	
c-family:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c.opt (Wcast-function-type): New warning option.
	* c-lex.c (get_fileinfo): Avoid warning.
	* c-ppoutput.c (scan_translation_unit_directives_only): Remove cast.

c:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-typeck.c (c_safe_function_type_cast_p): New helper function.
	(build_c_cast): Implement -Wcast_function_type.

cp:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* decl2.c (start_static_storage_duration_function): Aboid warning.
	* typeck.c (+cxx_safe_function_type_cast_p): New helper function.
	(build_reinterpret_cast_1): Implement -Wcast_function_type.

testsuite:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-c++-common/Wcast-function-type.c: New test.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: patch-cast-function-type.diff --]
[-- Type: text/x-patch; name="patch-cast-function-type.diff", Size: 11645 bytes --]

Index: gcc/c/c-typeck.c
===================================================================
--- gcc/c/c-typeck.c	(revision 253493)
+++ gcc/c/c-typeck.c	(working copy)
@@ -5474,6 +5474,36 @@ handle_warn_cast_qual (location_t loc, tree type,
   while (TREE_CODE (in_type) == POINTER_TYPE);
 }
 
+/* Check if a type cast between two function types can be considered safe.  */
+
+static bool
+c_safe_function_type_cast_p (tree t1, tree t2)
+{
+  if (TREE_TYPE (t1) == void_type_node &&
+      TYPE_ARG_TYPES (t1) == void_list_node)
+    return true;
+
+  if (TREE_TYPE (t2) == void_type_node &&
+      TYPE_ARG_TYPES (t2) == void_list_node)
+    return true;
+
+  if (!comptypes (TREE_TYPE (t1), TREE_TYPE (t2)))
+    return false;
+
+  for (t1 = TYPE_ARG_TYPES (t1), t2 = TYPE_ARG_TYPES (t2);
+       t1 && t2;
+       t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2))
+    {
+      if (TREE_CODE (TREE_VALUE (t1)) == POINTER_TYPE
+	  && TREE_CODE (TREE_VALUE (t2)) == POINTER_TYPE)
+	continue;
+      if (!comptypes (TREE_VALUE (t1), TREE_VALUE (t2)))
+	return false;
+    }
+
+  return true;
+}
+
 /* Build an expression representing a cast to type TYPE of expression EXPR.
    LOC is the location of the cast-- typically the open paren of the cast.  */
 
@@ -5667,6 +5697,16 @@ build_c_cast (location_t loc, tree type, tree expr
 	pedwarn (loc, OPT_Wpedantic, "ISO C forbids "
 		 "conversion of object pointer to function pointer type");
 
+      if (TREE_CODE (type) == POINTER_TYPE
+	  && TREE_CODE (otype) == POINTER_TYPE
+	  && TREE_CODE (TREE_TYPE (type)) == FUNCTION_TYPE
+	  && TREE_CODE (TREE_TYPE (otype)) == FUNCTION_TYPE
+	  && !c_safe_function_type_cast_p (TREE_TYPE (type),
+					   TREE_TYPE (otype)))
+	warning_at (loc, OPT_Wcast_function_type,
+		    "cast between incompatible function types"
+		    " from %qT to %qT", otype, type);
+
       ovalue = value;
       value = convert (type, value);
 
Index: gcc/c-family/c-lex.c
===================================================================
--- gcc/c-family/c-lex.c	(revision 253493)
+++ gcc/c-family/c-lex.c	(working copy)
@@ -101,9 +101,11 @@ get_fileinfo (const char *name)
   struct c_fileinfo *fi;
 
   if (!file_info_tree)
-    file_info_tree = splay_tree_new ((splay_tree_compare_fn) strcmp,
+    file_info_tree = splay_tree_new ((splay_tree_compare_fn)
+				     (void (*) (void)) strcmp,
 				     0,
-				     (splay_tree_delete_value_fn) free);
+				     (splay_tree_delete_value_fn)
+				     (void (*) (void)) free);
 
   n = splay_tree_lookup (file_info_tree, (splay_tree_key) name);
   if (n)
Index: gcc/c-family/c-ppoutput.c
===================================================================
--- gcc/c-family/c-ppoutput.c	(revision 253493)
+++ gcc/c-family/c-ppoutput.c	(working copy)
@@ -299,7 +299,7 @@ scan_translation_unit_directives_only (cpp_reader
   struct _cpp_dir_only_callbacks cb;
 
   cb.print_lines = print_lines_directives_only;
-  cb.maybe_print_line = (void (*) (source_location)) maybe_print_line;
+  cb.maybe_print_line = maybe_print_line;
 
   _cpp_preprocess_dir_only (pfile, &cb);
 }
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 253493)
+++ gcc/c-family/c.opt	(working copy)
@@ -384,6 +384,10 @@ Wc++17-compat
 C++ ObjC++ Var(warn_cxx17_compat) Warning LangEnabledBy(C++ ObjC++,Wall)
 Warn about C++ constructs whose meaning differs between ISO C++ 2014 and ISO C++ 2017.
 
+Wcast-function-type
+C ObjC C++ ObjC++ Var(warn_cast_function_type) Warning EnabledBy(Wextra)
+Warn about casts between incompatible function types.
+
 Wcast-qual
 C ObjC C++ ObjC++ Var(warn_cast_qual) Warning
 Warn about casts which discard qualifiers.
Index: gcc/cp/decl2.c
===================================================================
--- gcc/cp/decl2.c	(revision 253493)
+++ gcc/cp/decl2.c	(working copy)
@@ -3484,7 +3484,8 @@ start_static_storage_duration_function (unsigned c
       priority_info_map = splay_tree_new (splay_tree_compare_ints,
 					  /*delete_key_fn=*/0,
 					  /*delete_value_fn=*/
-					  (splay_tree_delete_value_fn) &free);
+					  (splay_tree_delete_value_fn)
+					  (void (*) (void)) free);
 
       /* We always need to generate functions for the
 	 DEFAULT_INIT_PRIORITY so enter it now.  That way when we walk
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 253493)
+++ gcc/cp/typeck.c	(working copy)
@@ -1173,6 +1173,40 @@ comp_template_parms_position (tree t1, tree t2)
   return true;
 }
 
+/* Check if a type cast between two function types can be considered safe.  */
+
+static bool
+cxx_safe_function_type_cast_p (tree t1, tree t2)
+{
+  if (TREE_CODE (t1) != FUNCTION_TYPE
+      || TREE_CODE (t2) != FUNCTION_TYPE)
+    return same_type_p (TREE_TYPE (t1), TREE_TYPE (t2));
+
+  if (TREE_TYPE (t1) == void_type_node &&
+      TYPE_ARG_TYPES (t1) == void_list_node)
+    return true;
+
+  if (TREE_TYPE (t2) == void_type_node &&
+      TYPE_ARG_TYPES (t2) == void_list_node)
+    return true;
+
+  if (!same_type_p (TREE_TYPE (t1), TREE_TYPE (t2)))
+    return false;
+
+  for (t1 = TYPE_ARG_TYPES (t1), t2 = TYPE_ARG_TYPES (t2);
+       t1 && t2;
+       t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2))
+    {
+      if (TREE_CODE (TREE_VALUE (t1)) == POINTER_TYPE
+	  && TREE_CODE (TREE_VALUE (t2)) == POINTER_TYPE)
+	continue;
+      if (!same_type_p (TREE_VALUE (t1), TREE_VALUE (t2)))
+	return false;
+    }
+
+  return true;
+}
+
 /* Subroutine in comptypes.  */
 
 static bool
@@ -7263,7 +7297,15 @@ build_reinterpret_cast_1 (tree type, tree expr, bo
     return rvalue (expr);
   else if ((TYPE_PTRFN_P (type) && TYPE_PTRFN_P (intype))
 	   || (TYPE_PTRMEMFUNC_P (type) && TYPE_PTRMEMFUNC_P (intype)))
-    return build_nop (type, expr);
+    {
+      if ((complain & tf_warning)
+	  && !cxx_safe_function_type_cast_p (TREE_TYPE (type),
+					     TREE_TYPE (intype)))
+	warning (OPT_Wcast_function_type,
+		 "cast between incompatible function types"
+		 " from %qH to %qI", intype, type);
+      return build_nop (type, expr);
+    }
   else if ((TYPE_PTRDATAMEM_P (type) && TYPE_PTRDATAMEM_P (intype))
 	   || (TYPE_PTROBV_P (type) && TYPE_PTROBV_P (intype)))
     {
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 253493)
+++ gcc/doc/invoke.texi	(working copy)
@@ -267,7 +267,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wno-builtin-declaration-mismatch @gol
 -Wno-builtin-macro-redefined  -Wc90-c99-compat  -Wc99-c11-compat @gol
 -Wc++-compat  -Wc++11-compat  -Wc++14-compat  @gol
--Wcast-align  -Wcast-align=strict  -Wcast-qual  @gol
+-Wcast-align  -Wcast-align=strict  -Wcast-function-type  -Wcast-qual  @gol
 -Wchar-subscripts  -Wchkp  -Wcatch-value  -Wcatch-value=@var{n} @gol
 -Wclobbered  -Wcomment  -Wconditionally-supported @gol
 -Wconversion  -Wcoverage-mismatch  -Wno-cpp  -Wdangling-else  -Wdate-time @gol
@@ -3899,6 +3899,7 @@ This enables some extra warning flags that are not
 name is still supported, but the newer name is more descriptive.)
 
 @gccoptlist{-Wclobbered  @gol
+-Wcast-function-type  @gol
 -Wempty-body  @gol
 -Wignored-qualifiers @gol
 -Wimplicit-fallthrough=3 @gol
@@ -5959,6 +5960,15 @@ Warn whenever a pointer is cast such that the requ
 target is increased.  For example, warn if a @code{char *} is cast to
 an @code{int *} regardless of the target machine.
 
+@item -Wcast-function-type
+@opindex Wcast-function-type
+@opindex Wno-cast-function-type
+Warn when a function pointer is cast to an incompatible function pointer.
+When one of the function types uses variable arguments like this
+@code{int f(...);}, then only the return value is checked, otherwise
+the return value and the parameter types are checked.
+This warning is enabled by @option{-Wextra}.
+
 @item -Wwrite-strings
 @opindex Wwrite-strings
 @opindex Wno-write-strings
Index: gcc/recog.h
===================================================================
--- gcc/recog.h	(revision 253493)
+++ gcc/recog.h	(working copy)
@@ -294,7 +294,7 @@ struct insn_gen_fn
   typedef rtx_insn * (*f15) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
   typedef rtx_insn * (*f16) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
 
-  typedef f0 stored_funcptr;
+  typedef void (*stored_funcptr) (void);
 
   rtx_insn * operator () (void) const { return ((f0)func) (); }
   rtx_insn * operator () (rtx a0) const { return ((f1)func) (a0); }
Index: gcc/testsuite/c-c++-common/Wcast-function-type.c
===================================================================
--- gcc/testsuite/c-c++-common/Wcast-function-type.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wcast-function-type.c	(working copy)
@@ -0,0 +1,31 @@
+/* { dg-do compile } */
+/* { dg-options "-Wcast-function-type" } */
+
+int f(long);
+
+typedef int (f1)(long);
+typedef int (f2)(void*);
+#ifdef __cplusplus
+typedef int (f3)(...);
+typedef void (f4)(...);
+#else
+typedef int (f3)();
+typedef void (f4)();
+#endif
+typedef void (f5)(void);
+
+f1 *a;
+f2 *b;
+f3 *c;
+f4 *d;
+f5 *e;
+
+void
+foo (void)
+{
+  a = (f1 *) f; /* { dg-bogus   "incompatible function types" } */
+  b = (f2 *) f; /* { dg-warning "incompatible function types" } */
+  c = (f3 *) f; /* { dg-bogus   "incompatible function types" } */
+  d = (f4 *) f; /* { dg-warning "incompatible function types" } */
+  e = (f5 *) f; /* { dg-bogus   "incompatible function types" } */
+}
Index: gcc/tree-dump.c
===================================================================
--- gcc/tree-dump.c	(revision 253493)
+++ gcc/tree-dump.c	(working copy)
@@ -735,7 +735,8 @@ dump_node (const_tree t, dump_flags_t flags, FILE
   di.flags = flags;
   di.node = t;
   di.nodes = splay_tree_new (splay_tree_compare_pointers, 0,
-			     (splay_tree_delete_value_fn) &free);
+			     (splay_tree_delete_value_fn)
+			     (void (*) (void)) free);
 
   /* Queue up the first node.  */
   queue (&di, t, DUMP_NONE);
Index: gcc/typed-splay-tree.h
===================================================================
--- gcc/typed-splay-tree.h	(revision 253493)
+++ gcc/typed-splay-tree.h	(working copy)
@@ -75,9 +75,12 @@ inline typed_splay_tree<KEY_TYPE, VALUE_TYPE>::
 		    delete_key_fn delete_key_fn,
 		    delete_value_fn delete_value_fn)
 {
-  m_inner = splay_tree_new ((splay_tree_compare_fn)compare_fn,
-			    (splay_tree_delete_key_fn)delete_key_fn,
-			    (splay_tree_delete_value_fn)delete_value_fn);
+  m_inner = splay_tree_new ((splay_tree_compare_fn)
+			    (void (*) (void)) compare_fn,
+			    (splay_tree_delete_key_fn)
+			    (void (*) (void)) delete_key_fn,
+			    (splay_tree_delete_value_fn)
+			    (void (*) (void)) delete_value_fn);
 }
 
 /* Destructor for typed_splay_tree <K, V>.  */
Index: libcpp/internal.h
===================================================================
--- libcpp/internal.h	(revision 253493)
+++ libcpp/internal.h	(working copy)
@@ -708,7 +708,7 @@ struct _cpp_dir_only_callbacks
 {
   /* Called to print a block of lines. */
   void (*print_lines) (int, const void *, size_t);
-  void (*maybe_print_line) (source_location);
+  bool (*maybe_print_line) (source_location);
 };
 
 extern void _cpp_preprocess_dir_only (cpp_reader *,

[PATCH] Add a warning for invalid function casts

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 1301 bytes --]

Hi!

I think I have now something useful, it has a few more heuristics
added, to reduce the number of false-positives so that it
is able to find real bugs, for instance in openssl it triggers
at a function cast which has already a TODO on it.

The heuristics are:
- handle void (*)(void) as a wild-card function type.
- ignore volatile, const qualifiers on parameters/return.
- handle any pointers as equivalent.
- handle integral types, enums, and booleans of same precision
   and signedness as equivalent.
- stop parameter validation at the first "...".

I cannot convince myself to handle uintptr_t and pointers as
equivalent.  It is possible that targets like m68k have
an ABI where uintptr_t and void* are different as return values
but are identical as parameter values.

IMHO adding an exception for uintptr_t vs. pointer as parameters
could easily prevent detection of real bugs.  Even if it is safe
for all targets.

However it happens: Examples are the libiberty splay-tree functions,
and also one single place in linux, where an argument of type
"long int" is used to call a timer function with a pointer
argument.  Note, linux does not enable -Wextra.


Patch was bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.

[-- Attachment #2: changelog-cast-function-type.txt --]
[-- Type: text/plain, Size: 1209 bytes --]

gcc:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* doc/invoke.texi: Document -Wcast-function-type.
	* recog.h (stored_funcptr): Change signature.
	* tree-dump.c (dump_node): Avoid warning.
	* typed-splay-tree.h (typed_splay_tree): Avoid warning.

libcpp:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* internal.h (maybe_print_line): Change signature.
	
c-family:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c.opt (Wcast-function-type): New warning option.
	* c-lex.c (get_fileinfo): Avoid warning.
	* c-ppoutput.c (scan_translation_unit_directives_only): Remove cast.

c:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-typeck.c (c_abi_equiv_type_p, c_safe_function_type_cast_p): New.
	(build_c_cast): Implement -Wcast_function_type.

cp:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* decl2.c (start_static_storage_duration_function): Avoid warning.
	* typeck.c (cxx_abi_equiv_type_p, cxx_safe_function_type_cast_p): New.
	(build_reinterpret_cast_1): Implement -Wcast_function_type.

testsuite:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-c++-common/Wcast-function-type.c: New test.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: patch-cast-function-type.diff --]
[-- Type: text/x-patch; name="patch-cast-function-type.diff", Size: 12982 bytes --]

Index: gcc/c/c-typeck.c
===================================================================
--- gcc/c/c-typeck.c	(revision 253493)
+++ gcc/c/c-typeck.c	(working copy)
@@ -5474,6 +5474,52 @@ handle_warn_cast_qual (location_t loc, tree type,
   while (TREE_CODE (in_type) == POINTER_TYPE);
 }
 
+/* Heuristic check if two parameter types can be considered ABI-equivalent.  */
+
+static bool
+c_abi_equiv_type_p (tree t1, tree t2)
+{
+  t1 = TYPE_MAIN_VARIANT (t1);
+  t2 = TYPE_MAIN_VARIANT (t2);
+
+  if (TREE_CODE (t1) == POINTER_TYPE
+      && TREE_CODE (t2) == POINTER_TYPE)
+    return true;
+
+  if (INTEGRAL_TYPE_P (t1)
+      && INTEGRAL_TYPE_P (t2)
+      && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
+      && TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2))
+    return true;
+
+  return comptypes (t1, t2);
+}
+
+/* Check if a type cast between two function types can be considered safe.  */
+
+static bool
+c_safe_function_type_cast_p (tree t1, tree t2)
+{
+  if (TREE_TYPE (t1) == void_type_node &&
+      TYPE_ARG_TYPES (t1) == void_list_node)
+    return true;
+
+  if (TREE_TYPE (t2) == void_type_node &&
+      TYPE_ARG_TYPES (t2) == void_list_node)
+    return true;
+
+  if (!c_abi_equiv_type_p (TREE_TYPE (t1), TREE_TYPE (t2)))
+    return false;
+
+  for (t1 = TYPE_ARG_TYPES (t1), t2 = TYPE_ARG_TYPES (t2);
+       t1 && t2;
+       t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2))
+    if (!c_abi_equiv_type_p (TREE_VALUE (t1), TREE_VALUE (t2)))
+      return false;
+
+  return true;
+}
+
 /* Build an expression representing a cast to type TYPE of expression EXPR.
    LOC is the location of the cast-- typically the open paren of the cast.  */
 
@@ -5667,6 +5713,16 @@ build_c_cast (location_t loc, tree type, tree expr
 	pedwarn (loc, OPT_Wpedantic, "ISO C forbids "
 		 "conversion of object pointer to function pointer type");
 
+      if (TREE_CODE (type) == POINTER_TYPE
+	  && TREE_CODE (otype) == POINTER_TYPE
+	  && TREE_CODE (TREE_TYPE (type)) == FUNCTION_TYPE
+	  && TREE_CODE (TREE_TYPE (otype)) == FUNCTION_TYPE
+	  && !c_safe_function_type_cast_p (TREE_TYPE (type),
+					   TREE_TYPE (otype)))
+	warning_at (loc, OPT_Wcast_function_type,
+		    "cast between incompatible function types"
+		    " from %qT to %qT", otype, type);
+
       ovalue = value;
       value = convert (type, value);
 
Index: gcc/c-family/c-lex.c
===================================================================
--- gcc/c-family/c-lex.c	(revision 253493)
+++ gcc/c-family/c-lex.c	(working copy)
@@ -101,9 +101,11 @@ get_fileinfo (const char *name)
   struct c_fileinfo *fi;
 
   if (!file_info_tree)
-    file_info_tree = splay_tree_new ((splay_tree_compare_fn) strcmp,
+    file_info_tree = splay_tree_new ((splay_tree_compare_fn)
+				     (void (*) (void)) strcmp,
 				     0,
-				     (splay_tree_delete_value_fn) free);
+				     (splay_tree_delete_value_fn)
+				     (void (*) (void)) free);
 
   n = splay_tree_lookup (file_info_tree, (splay_tree_key) name);
   if (n)
Index: gcc/c-family/c-ppoutput.c
===================================================================
--- gcc/c-family/c-ppoutput.c	(revision 253493)
+++ gcc/c-family/c-ppoutput.c	(working copy)
@@ -299,7 +299,7 @@ scan_translation_unit_directives_only (cpp_reader
   struct _cpp_dir_only_callbacks cb;
 
   cb.print_lines = print_lines_directives_only;
-  cb.maybe_print_line = (void (*) (source_location)) maybe_print_line;
+  cb.maybe_print_line = maybe_print_line;
 
   _cpp_preprocess_dir_only (pfile, &cb);
 }
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 253493)
+++ gcc/c-family/c.opt	(working copy)
@@ -384,6 +384,10 @@ Wc++17-compat
 C++ ObjC++ Var(warn_cxx17_compat) Warning LangEnabledBy(C++ ObjC++,Wall)
 Warn about C++ constructs whose meaning differs between ISO C++ 2014 and ISO C++ 2017.
 
+Wcast-function-type
+C ObjC C++ ObjC++ Var(warn_cast_function_type) Warning EnabledBy(Wextra)
+Warn about casts between incompatible function types.
+
 Wcast-qual
 C ObjC C++ ObjC++ Var(warn_cast_qual) Warning
 Warn about casts which discard qualifiers.
Index: gcc/cp/decl2.c
===================================================================
--- gcc/cp/decl2.c	(revision 253493)
+++ gcc/cp/decl2.c	(working copy)
@@ -3484,7 +3484,8 @@ start_static_storage_duration_function (unsigned c
       priority_info_map = splay_tree_new (splay_tree_compare_ints,
 					  /*delete_key_fn=*/0,
 					  /*delete_value_fn=*/
-					  (splay_tree_delete_value_fn) &free);
+					  (splay_tree_delete_value_fn)
+					  (void (*) (void)) free);
 
       /* We always need to generate functions for the
 	 DEFAULT_INIT_PRIORITY so enter it now.  That way when we walk
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 253493)
+++ gcc/cp/typeck.c	(working copy)
@@ -1173,6 +1173,52 @@ comp_template_parms_position (tree t1, tree t2)
   return true;
 }
 
+/* Heuristic check if two parameter types can be considered ABI-equivalent.  */
+
+static bool
+cxx_abi_equiv_type_p (tree t1, tree t2)
+{
+  t1 = TYPE_MAIN_VARIANT (t1);
+  t2 = TYPE_MAIN_VARIANT (t2);
+
+  if (TREE_CODE (t1) == POINTER_TYPE
+      && TREE_CODE (t2) == POINTER_TYPE)
+    return true;
+
+  if (INTEGRAL_TYPE_P (t1)
+      && INTEGRAL_TYPE_P (t2)
+      && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
+      && TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2))
+    return true;
+
+  return same_type_p (t1, t2);
+}
+
+/* Check if a type cast between two function types can be considered safe.  */
+
+static bool
+cxx_safe_function_type_cast_p (tree t1, tree t2)
+{
+  if (TREE_TYPE (t1) == void_type_node &&
+      TYPE_ARG_TYPES (t1) == void_list_node)
+    return true;
+
+  if (TREE_TYPE (t2) == void_type_node &&
+      TYPE_ARG_TYPES (t2) == void_list_node)
+    return true;
+
+  if (!cxx_abi_equiv_type_p (TREE_TYPE (t1), TREE_TYPE (t2)))
+    return false;
+
+  for (t1 = TYPE_ARG_TYPES (t1), t2 = TYPE_ARG_TYPES (t2);
+       t1 && t2;
+       t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2))
+    if (!cxx_abi_equiv_type_p (TREE_VALUE (t1), TREE_VALUE (t2)))
+      return false;
+
+  return true;
+}
+
 /* Subroutine in comptypes.  */
 
 static bool
@@ -7263,7 +7309,19 @@ build_reinterpret_cast_1 (tree type, tree expr, bo
     return rvalue (expr);
   else if ((TYPE_PTRFN_P (type) && TYPE_PTRFN_P (intype))
 	   || (TYPE_PTRMEMFUNC_P (type) && TYPE_PTRMEMFUNC_P (intype)))
-    return build_nop (type, expr);
+    {
+      if ((complain & tf_warning)
+	  && TREE_CODE (type) == POINTER_TYPE
+	  && TREE_CODE (intype) == POINTER_TYPE
+	  && TREE_CODE (TREE_TYPE (type)) == FUNCTION_TYPE
+	  && TREE_CODE (TREE_TYPE (intype)) == FUNCTION_TYPE
+	  && !cxx_safe_function_type_cast_p (TREE_TYPE (type),
+					     TREE_TYPE (intype)))
+	warning (OPT_Wcast_function_type,
+		 "cast between incompatible function types"
+		 " from %qH to %qI", intype, type);
+      return build_nop (type, expr);
+    }
   else if ((TYPE_PTRDATAMEM_P (type) && TYPE_PTRDATAMEM_P (intype))
 	   || (TYPE_PTROBV_P (type) && TYPE_PTROBV_P (intype)))
     {
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 253493)
+++ gcc/doc/invoke.texi	(working copy)
@@ -267,7 +267,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wno-builtin-declaration-mismatch @gol
 -Wno-builtin-macro-redefined  -Wc90-c99-compat  -Wc99-c11-compat @gol
 -Wc++-compat  -Wc++11-compat  -Wc++14-compat  @gol
--Wcast-align  -Wcast-align=strict  -Wcast-qual  @gol
+-Wcast-align  -Wcast-align=strict  -Wcast-function-type  -Wcast-qual  @gol
 -Wchar-subscripts  -Wchkp  -Wcatch-value  -Wcatch-value=@var{n} @gol
 -Wclobbered  -Wcomment  -Wconditionally-supported @gol
 -Wconversion  -Wcoverage-mismatch  -Wno-cpp  -Wdangling-else  -Wdate-time @gol
@@ -3899,6 +3899,7 @@ This enables some extra warning flags that are not
 name is still supported, but the newer name is more descriptive.)
 
 @gccoptlist{-Wclobbered  @gol
+-Wcast-function-type  @gol
 -Wempty-body  @gol
 -Wignored-qualifiers @gol
 -Wimplicit-fallthrough=3 @gol
@@ -5959,6 +5960,21 @@ Warn whenever a pointer is cast such that the requ
 target is increased.  For example, warn if a @code{char *} is cast to
 an @code{int *} regardless of the target machine.
 
+@item -Wcast-function-type
+@opindex Wcast-function-type
+@opindex Wno-cast-function-type
+Warn when a function pointer is cast to an incompatible function pointer.
+When one of the function types uses variable arguments like this
+@code{int f(...);}, then only the return value and the parameters before
+the @code{...} are checked, otherwise the return value and all parametes
+are checked for binary compatibility.  Any parameter of pointer-type
+matches any other pointer-type.  Any benign differences in integral types
+are ignored, like @code{int} vs.  @code{long} on ILP32 targets.  Likewise
+@code{const} and @code{volatile} qualifiers are ignored.  The function
+type @code{void (*) (void);} is special and matches everything, which can
+be used to suppress this warning.
+This warning is enabled by @option{-Wextra}.
+
 @item -Wwrite-strings
 @opindex Wwrite-strings
 @opindex Wno-write-strings
Index: gcc/recog.h
===================================================================
--- gcc/recog.h	(revision 253493)
+++ gcc/recog.h	(working copy)
@@ -294,7 +294,7 @@ struct insn_gen_fn
   typedef rtx_insn * (*f15) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
   typedef rtx_insn * (*f16) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
 
-  typedef f0 stored_funcptr;
+  typedef void (*stored_funcptr) (void);
 
   rtx_insn * operator () (void) const { return ((f0)func) (); }
   rtx_insn * operator () (rtx a0) const { return ((f1)func) (a0); }
Index: gcc/testsuite/c-c++-common/Wcast-function-type.c
===================================================================
--- gcc/testsuite/c-c++-common/Wcast-function-type.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wcast-function-type.c	(working copy)
@@ -0,0 +1,31 @@
+/* { dg-do compile } */
+/* { dg-options "-Wcast-function-type" } */
+
+int f(long);
+
+typedef int (f1)(long);
+typedef int (f2)(void*);
+#ifdef __cplusplus
+typedef int (f3)(...);
+typedef void (f4)(...);
+#else
+typedef int (f3)();
+typedef void (f4)();
+#endif
+typedef void (f5)(void);
+
+f1 *a;
+f2 *b;
+f3 *c;
+f4 *d;
+f5 *e;
+
+void
+foo (void)
+{
+  a = (f1 *) f; /* { dg-bogus   "incompatible function types" } */
+  b = (f2 *) f; /* { dg-warning "incompatible function types" } */
+  c = (f3 *) f; /* { dg-bogus   "incompatible function types" } */
+  d = (f4 *) f; /* { dg-warning "incompatible function types" } */
+  e = (f5 *) f; /* { dg-bogus   "incompatible function types" } */
+}
Index: gcc/tree-dump.c
===================================================================
--- gcc/tree-dump.c	(revision 253493)
+++ gcc/tree-dump.c	(working copy)
@@ -735,7 +735,8 @@ dump_node (const_tree t, dump_flags_t flags, FILE
   di.flags = flags;
   di.node = t;
   di.nodes = splay_tree_new (splay_tree_compare_pointers, 0,
-			     (splay_tree_delete_value_fn) &free);
+			     (splay_tree_delete_value_fn)
+			     (void (*) (void)) free);
 
   /* Queue up the first node.  */
   queue (&di, t, DUMP_NONE);
Index: gcc/typed-splay-tree.h
===================================================================
--- gcc/typed-splay-tree.h	(revision 253493)
+++ gcc/typed-splay-tree.h	(working copy)
@@ -75,9 +75,12 @@ inline typed_splay_tree<KEY_TYPE, VALUE_TYPE>::
 		    delete_key_fn delete_key_fn,
 		    delete_value_fn delete_value_fn)
 {
-  m_inner = splay_tree_new ((splay_tree_compare_fn)compare_fn,
-			    (splay_tree_delete_key_fn)delete_key_fn,
-			    (splay_tree_delete_value_fn)delete_value_fn);
+  m_inner = splay_tree_new ((splay_tree_compare_fn)
+			    (void (*) (void)) compare_fn,
+			    (splay_tree_delete_key_fn)
+			    (void (*) (void)) delete_key_fn,
+			    (splay_tree_delete_value_fn)
+			    (void (*) (void)) delete_value_fn);
 }
 
 /* Destructor for typed_splay_tree <K, V>.  */
Index: libcpp/internal.h
===================================================================
--- libcpp/internal.h	(revision 253493)
+++ libcpp/internal.h	(working copy)
@@ -708,7 +708,7 @@ struct _cpp_dir_only_callbacks
 {
   /* Called to print a block of lines. */
   void (*print_lines) (int, const void *, size_t);
-  void (*maybe_print_line) (source_location);
+  bool (*maybe_print_line) (source_location);
 };
 
 extern void _cpp_preprocess_dir_only (cpp_reader *,

Re: [PATCH] Add a warning for invalid function casts

[Martin Sebor]

On 10/07/2017 10:48 AM, Bernd Edlinger wrote:
> Hi!
>
> I think I have now something useful, it has a few more heuristics
> added, to reduce the number of false-positives so that it
> is able to find real bugs, for instance in openssl it triggers
> at a function cast which has already a TODO on it.
>
> The heuristics are:
> - handle void (*)(void) as a wild-card function type.
> - ignore volatile, const qualifiers on parameters/return.
> - handle any pointers as equivalent.
> - handle integral types, enums, and booleans of same precision
>    and signedness as equivalent.
> - stop parameter validation at the first "...".

These sound quite reasonable to me.  I have a reservation about
just one of them, and some comments about other aspects of the
warning.  Sorry if this seems like a lot.  I'm hoping you'll
find the feedback constructive.

I don't think using void(*)(void) to suppress the warning is
a robust solution because it's not safe to call a function that
takes arguments through such a pointer (especially not if one
or more of the arguments is a pointer).  Depending on the ABI,
calling a function that expects arguments with none could also
mess up the stack as the callee may pop arguments that were
never passed to it.

Instead, I think the warning should be suppressed for casts to
function types without a prototype.  Calling those is (or should
for the most part be) safe and they are the natural way to express
that we don't care about type safety.

Let me also clarify that I understand bullet 4 correctly.
In my tests the warning triggers on enum/integral/boolean
incompatibilities that the text above suggests should be accepted.
  E.g.:

   typedef enum E { e } E;
   E f (void);

   typedef int F (void);

   F *pf = (F *)f;   // -Wcast-function-type

Is the warning here intended?  (My reading of the documentation
suggests it's not.)

In testing the warning in C++, I noticed it's not issued for
casts between incompatible pointers to member functions, or for
casts between member functions to ordinary functions (those are
diagnosed with -Wpedantic

   struct S { void foo (int*); };

   typedef void (S::*MF)(int);
   typedef void F (int*);

   MF pmf = (MF)&S::foo;   // no warning
   F *pf = (F*)&S::foo;    // -Wpedantic only

These both look like they would be worth diagnosing under the new
option (the second one looks like an opportunity to provide
a dedicated option to control the existing pedantic warning).

> I cannot convince myself to handle uintptr_t and pointers as
> equivalent.  It is possible that targets like m68k have
> an ABI where uintptr_t and void* are different as return values
> but are identical as parameter values.
>
> IMHO adding an exception for uintptr_t vs. pointer as parameters
> could easily prevent detection of real bugs.  Even if it is safe
> for all targets.
>
> However it happens: Examples are the libiberty splay-tree functions,
> and also one single place in linux, where an argument of type
> "long int" is used to call a timer function with a pointer
> argument.  Note, linux does not enable -Wextra.
>
>
> Patch was bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?

A few comments on the documentation:

   When one of the function types uses variable arguments like this
   @code{int f(...);}, then only the return value and the parameters
   before the @code{...} are checked,

Although G++ accepts 'int f(...)' since GCC does not I would suggest
to avoid showing the prototype in the descriptive text and instead
refer to "functions taking variable arguments."   Also, it's the
types of the arguments that are considered (not the value).  With
that I would suggest rewording the sentence along these lines:

   In a cast involving a function types with a variable argument
   list only the types of initial arguments that are provided are
   considered.

The sentence

   Any benign differences in integral types are ignored...

leaves open the question of what's considered benign.  In my view,
if pointer types are ignored (which is reasonable as we discussed
but which can lead to serious problems) it seems that that
signed/unsigned differences should also be considered benign.
  The consequences of those differences seem considerably less
dangerous than calling a function that takes an char* with an int*
argument.

Regarding qualifiers, unless some types qualifiers are not ignored
(e.g., _Atomic and restrict) I would suggest to correct the sentence
that mentions const and volatile to simply refer to "type qualifiers"
instead to make it clear that no qualifiers are considered.

Martin

Re: [PATCH] Add a warning for invalid function casts

[Bernd Edlinger]

On 10/09/17 18:44, Martin Sebor wrote:
> On 10/07/2017 10:48 AM, Bernd Edlinger wrote:
>> Hi!
>>
>> I think I have now something useful, it has a few more heuristics
>> added, to reduce the number of false-positives so that it
>> is able to find real bugs, for instance in openssl it triggers
>> at a function cast which has already a TODO on it.
>>
>> The heuristics are:
>> - handle void (*)(void) as a wild-card function type.
>> - ignore volatile, const qualifiers on parameters/return.
>> - handle any pointers as equivalent.
>> - handle integral types, enums, and booleans of same precision
>>    and signedness as equivalent.
>> - stop parameter validation at the first "...".
> 
> These sound quite reasonable to me.  I have a reservation about
> just one of them, and some comments about other aspects of the
> warning.  Sorry if this seems like a lot.  I'm hoping you'll
> find the feedback constructive.
> 
> I don't think using void(*)(void) to suppress the warning is
> a robust solution because it's not safe to call a function that
> takes arguments through such a pointer (especially not if one
> or more of the arguments is a pointer).  Depending on the ABI,
> calling a function that expects arguments with none could also
> mess up the stack as the callee may pop arguments that were
> never passed to it.
> 

This is of course only a heuristic, and if there is no warning
that does not mean any guarantee that there can't be a problem
at runtime.  The heuristic is only meant to separate the
bad from the very bad type-cast.  In my personal opinion there
is not a single good type cast.

> Instead, I think the warning should be suppressed for casts to
> function types without a prototype.  Calling those is (or should
> for the most part be) safe and they are the natural way to express
> that we don't care about type safety.
> 
> Let me also clarify that I understand bullet 4 correctly.
> In my tests the warning triggers on enum/integral/boolean
> incompatibilities that the text above suggests should be accepted.
>   E.g.:
> 
>    typedef enum E { e } E;
>    E f (void);
> 
>    typedef int F (void);
> 
>    F *pf = (F *)f;   // -Wcast-function-type
> 
> Is the warning here intended?  (My reading of the documentation
> suggests it's not.)
> 

Aehm no, that is unintentional, thanks for testing...

It looks like the warning does not trigger with
typedef unsigned int F (void);

But this enum should promote to int, likewise for _Bool,
So I think this should be fixable.


> In testing the warning in C++, I noticed it's not issued for
> casts between incompatible pointers to member functions, or for
> casts between member functions to ordinary functions (those are
> diagnosed with -Wpedantic
> 
>    struct S { void foo (int*); };
> 
>    typedef void (S::*MF)(int);
>    typedef void F (int*);
> 
>    MF pmf = (MF)&S::foo;   // no warning
>    F *pf = (F*)&S::foo;    // -Wpedantic only
> 
> These both look like they would be worth diagnosing under the new
> option (the second one looks like an opportunity to provide
> a dedicated option to control the existing pedantic warning).
> 

Yes I agree.

I think all pointer-to member function casts where the types
are different should warn, although I don't have seen any of that
crap so far.


>> I cannot convince myself to handle uintptr_t and pointers as
>> equivalent.  It is possible that targets like m68k have
>> an ABI where uintptr_t and void* are different as return values
>> but are identical as parameter values.
>>
>> IMHO adding an exception for uintptr_t vs. pointer as parameters
>> could easily prevent detection of real bugs.  Even if it is safe
>> for all targets.
>>
>> However it happens: Examples are the libiberty splay-tree functions,
>> and also one single place in linux, where an argument of type
>> "long int" is used to call a timer function with a pointer
>> argument.  Note, linux does not enable -Wextra.
>>
>>
>> Patch was bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
> 
> A few comments on the documentation:
> 
>    When one of the function types uses variable arguments like this
>    @code{int f(...);}, then only the return value and the parameters
>    before the @code{...} are checked,
> 
> Although G++ accepts 'int f(...)' since GCC does not I would suggest
> to avoid showing the prototype in the descriptive text and instead
> refer to "functions taking variable arguments."   Also, it's the
> types of the arguments that are considered (not the value).  With
> that I would suggest rewording the sentence along these lines:
>  >    In a cast involving a function types with a variable argument
>    list only the types of initial arguments that are provided are
>    considered.
> 
> The sentence
> 
>    Any benign differences in integral types are ignored...
> 
> leaves open the question of what's considered benign.  In my view,
> if pointer types are ignored (which is reasonable as we discussed
> but which can lead to serious problems) it seems that that
> signed/unsigned differences should also be considered benign.
>   The consequences of those differences seem considerably less
> dangerous than calling a function that takes an char* with an int*
> argument.

I am not sure if the sign is relevant for int/unsigned int.

What I heard from the linux people is, the callee expects the
inputs to be correctly promoted according to the type of the
parameter, the example was an unsigned char which is used
as an array index, but the actual register may be negative
for instance, which results in undefined behavior.

So I am not sure if that applies only to values with
precision smaller than int, or also for int64_t
i.e. if the target has 64bit registers.

> 
> Regarding qualifiers, unless some types qualifiers are not ignored
> (e.g., _Atomic and restrict) I would suggest to correct the sentence
> that mentions const and volatile to simply refer to "type qualifiers"
> instead to make it clear that no qualifiers are considered.
> 

Suggested doc changes sound good, I will update the doc accordingly.


Thanks
Bernd.

Re: [PATCH] Add a warning for invalid function casts

[Martin Sebor]

On 10/09/2017 11:50 AM, Bernd Edlinger wrote:
> On 10/09/17 18:44, Martin Sebor wrote:
>> On 10/07/2017 10:48 AM, Bernd Edlinger wrote:
>>> Hi!
>>>
>>> I think I have now something useful, it has a few more heuristics
>>> added, to reduce the number of false-positives so that it
>>> is able to find real bugs, for instance in openssl it triggers
>>> at a function cast which has already a TODO on it.
>>>
>>> The heuristics are:
>>> - handle void (*)(void) as a wild-card function type.
>>> - ignore volatile, const qualifiers on parameters/return.
>>> - handle any pointers as equivalent.
>>> - handle integral types, enums, and booleans of same precision
>>>    and signedness as equivalent.
>>> - stop parameter validation at the first "...".
>>
>> These sound quite reasonable to me.  I have a reservation about
>> just one of them, and some comments about other aspects of the
>> warning.  Sorry if this seems like a lot.  I'm hoping you'll
>> find the feedback constructive.
>>
>> I don't think using void(*)(void) to suppress the warning is
>> a robust solution because it's not safe to call a function that
>> takes arguments through such a pointer (especially not if one
>> or more of the arguments is a pointer).  Depending on the ABI,
>> calling a function that expects arguments with none could also
>> mess up the stack as the callee may pop arguments that were
>> never passed to it.
>>
>
> This is of course only a heuristic, and if there is no warning
> that does not mean any guarantee that there can't be a problem
> at runtime.  The heuristic is only meant to separate the
> bad from the very bad type-cast.  In my personal opinion there
> is not a single good type cast.

I agree.  Since the warning uses one kind of a cast as an escape
mechanism from the checking it should be one whose result can
the most likely be used to call the function without undefined
behavior.

Since it's possible to call any function through a pointer to
a function with no arguments (simply by providing arguments of
matching types) it's a reasonable candidate.

On the other hand, since it is not safe to call an arbitrary
function through void (*)(void), it's not as good a candidate.

Another reason why I think a protoype-less function is a good
choice is because the alias and ifunc attributes already use it
as an escape mechanism from their type incompatibility warning.

Martin

Re: [PATCH] Add a warning for invalid function casts

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 4218 bytes --]

On 10/09/17 20:34, Martin Sebor wrote:
> On 10/09/2017 11:50 AM, Bernd Edlinger wrote:
>> On 10/09/17 18:44, Martin Sebor wrote:
>>> On 10/07/2017 10:48 AM, Bernd Edlinger wrote:
>>>> Hi!
>>>>
>>>> I think I have now something useful, it has a few more heuristics
>>>> added, to reduce the number of false-positives so that it
>>>> is able to find real bugs, for instance in openssl it triggers
>>>> at a function cast which has already a TODO on it.
>>>>
>>>> The heuristics are:
>>>> - handle void (*)(void) as a wild-card function type.
>>>> - ignore volatile, const qualifiers on parameters/return.
>>>> - handle any pointers as equivalent.
>>>> - handle integral types, enums, and booleans of same precision
>>>>    and signedness as equivalent.
>>>> - stop parameter validation at the first "...".
>>>
>>> These sound quite reasonable to me.  I have a reservation about
>>> just one of them, and some comments about other aspects of the
>>> warning.  Sorry if this seems like a lot.  I'm hoping you'll
>>> find the feedback constructive.
>>>
>>> I don't think using void(*)(void) to suppress the warning is
>>> a robust solution because it's not safe to call a function that
>>> takes arguments through such a pointer (especially not if one
>>> or more of the arguments is a pointer).  Depending on the ABI,
>>> calling a function that expects arguments with none could also
>>> mess up the stack as the callee may pop arguments that were
>>> never passed to it.
>>>
>>
>> This is of course only a heuristic, and if there is no warning
>> that does not mean any guarantee that there can't be a problem
>> at runtime.  The heuristic is only meant to separate the
>> bad from the very bad type-cast.  In my personal opinion there
>> is not a single good type cast.
> 
> I agree.  Since the warning uses one kind of a cast as an escape
> mechanism from the checking it should be one whose result can
> the most likely be used to call the function without undefined
> behavior.
> 
> Since it's possible to call any function through a pointer to
> a function with no arguments (simply by providing arguments of
> matching types) it's a reasonable candidate.
> 
> On the other hand, since it is not safe to call an arbitrary
> function through void (*)(void), it's not as good a candidate.
> 
> Another reason why I think a protoype-less function is a good
> choice is because the alias and ifunc attributes already use it
> as an escape mechanism from their type incompatibility warning.
> 

I know of pre-existing code-bases where a type-cast to type:
void (*) (void);

.. is already used as a generic function pointer: libffi and
libgo, I would not want to break these.

Actually when I have a type:
X (*) (...);

I would like to make sure that the warning checks that
only functions returning X are assigned.

and for X (*) (Y, ....);

I would like to check that anything returning X with
first argument of type Y is assigned.

There are code bases where such a scheme is used.
For instance one that I myself maintain: the OPC/UA AnsiC Stack,
where I have this type definition:

typedef OpcUa_StatusCode (OpcUa_PfnInvokeService)(OpcUa_Endpoint 
hEndpoint, ...);

And this plays well together with this warning, because only
functions are assigned that match up to the ...);
Afterwards this pointer is cast back to the original signature,
so everything is perfectly fine.

Regarding the cast from pointer to member to function, I see also a
warning without -Wpedantic:
Warnung: converting from »void (S::*)(int*)« to »void (*)(int*)« 
[-Wpmf-conversions]
    F *pf = (F*)&S::foo;
                    ^~~

And this one is even default-enabled, so I think that should be
more than sufficient.

I also changed the heuristic, so that your example with the enum should
now work.  I did not add it to the test case, because it would
break with -fshort-enums :(

Attached I have an updated patch that extends this warning to the
pointer-to-member function cast, and relaxes the heuristic on the
benign integral type differences a bit further.


Is it OK for trunk after bootstrap and reg-testing?


Thanks
Bernd.

[-- Attachment #2: changelog-cast-function-type.txt --]
[-- Type: text/plain, Size: 1253 bytes --]

gcc:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* doc/invoke.texi: Document -Wcast-function-type.
	* recog.h (stored_funcptr): Change signature.
	* tree-dump.c (dump_node): Avoid warning.
	* typed-splay-tree.h (typed_splay_tree): Avoid warning.

libcpp:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* internal.h (maybe_print_line): Change signature.
	
c-family:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c.opt (Wcast-function-type): New warning option.
	* c-lex.c (get_fileinfo): Avoid warning.
	* c-ppoutput.c (scan_translation_unit_directives_only): Remove cast.

c:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-typeck.c (c_abi_equiv_type_p, c_safe_function_type_cast_p): New.
	(build_c_cast): Implement -Wcast_function_type.

cp:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* decl2.c (start_static_storage_duration_function): Avoid warning.
	* typeck.c (cxx_abi_equiv_type_p, cxx_safe_function_type_cast_p): New.
	(build_reinterpret_cast_1): Implement -Wcast_function_type.

testsuite:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-c++-common/Wcast-function-type.c: New test.
	* g++.dg/Wcast-function-type.C: New test.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: patch-cast-function-type.diff --]
[-- Type: text/x-patch; name="patch-cast-function-type.diff", Size: 13946 bytes --]

Index: gcc/c/c-typeck.c
===================================================================
--- gcc/c/c-typeck.c	(revision 253493)
+++ gcc/c/c-typeck.c	(working copy)
@@ -5474,6 +5474,53 @@ handle_warn_cast_qual (location_t loc, tree type,
   while (TREE_CODE (in_type) == POINTER_TYPE);
 }
 
+/* Heuristic check if two parameter types can be considered ABI-equivalent.  */
+
+static bool
+c_abi_equiv_type_p (tree t1, tree t2)
+{
+  t1 = TYPE_MAIN_VARIANT (t1);
+  t2 = TYPE_MAIN_VARIANT (t2);
+
+  if (TREE_CODE (t1) == POINTER_TYPE
+      && TREE_CODE (t2) == POINTER_TYPE)
+    return true;
+
+  if (INTEGRAL_TYPE_P (t1)
+      && INTEGRAL_TYPE_P (t2)
+      && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
+      && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
+	  || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
+    return true;
+
+  return comptypes (t1, t2);
+}
+
+/* Check if a type cast between two function types can be considered safe.  */
+
+static bool
+c_safe_function_type_cast_p (tree t1, tree t2)
+{
+  if (TREE_TYPE (t1) == void_type_node &&
+      TYPE_ARG_TYPES (t1) == void_list_node)
+    return true;
+
+  if (TREE_TYPE (t2) == void_type_node &&
+      TYPE_ARG_TYPES (t2) == void_list_node)
+    return true;
+
+  if (!c_abi_equiv_type_p (TREE_TYPE (t1), TREE_TYPE (t2)))
+    return false;
+
+  for (t1 = TYPE_ARG_TYPES (t1), t2 = TYPE_ARG_TYPES (t2);
+       t1 && t2;
+       t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2))
+    if (!c_abi_equiv_type_p (TREE_VALUE (t1), TREE_VALUE (t2)))
+      return false;
+
+  return true;
+}
+
 /* Build an expression representing a cast to type TYPE of expression EXPR.
    LOC is the location of the cast-- typically the open paren of the cast.  */
 
@@ -5667,6 +5714,16 @@ build_c_cast (location_t loc, tree type, tree expr
 	pedwarn (loc, OPT_Wpedantic, "ISO C forbids "
 		 "conversion of object pointer to function pointer type");
 
+      if (TREE_CODE (type) == POINTER_TYPE
+	  && TREE_CODE (otype) == POINTER_TYPE
+	  && TREE_CODE (TREE_TYPE (type)) == FUNCTION_TYPE
+	  && TREE_CODE (TREE_TYPE (otype)) == FUNCTION_TYPE
+	  && !c_safe_function_type_cast_p (TREE_TYPE (type),
+					   TREE_TYPE (otype)))
+	warning_at (loc, OPT_Wcast_function_type,
+		    "cast between incompatible function types"
+		    " from %qT to %qT", otype, type);
+
       ovalue = value;
       value = convert (type, value);
 
Index: gcc/c-family/c-lex.c
===================================================================
--- gcc/c-family/c-lex.c	(revision 253493)
+++ gcc/c-family/c-lex.c	(working copy)
@@ -101,9 +101,11 @@ get_fileinfo (const char *name)
   struct c_fileinfo *fi;
 
   if (!file_info_tree)
-    file_info_tree = splay_tree_new ((splay_tree_compare_fn) strcmp,
+    file_info_tree = splay_tree_new ((splay_tree_compare_fn)
+				     (void (*) (void)) strcmp,
 				     0,
-				     (splay_tree_delete_value_fn) free);
+				     (splay_tree_delete_value_fn)
+				     (void (*) (void)) free);
 
   n = splay_tree_lookup (file_info_tree, (splay_tree_key) name);
   if (n)
Index: gcc/c-family/c-ppoutput.c
===================================================================
--- gcc/c-family/c-ppoutput.c	(revision 253493)
+++ gcc/c-family/c-ppoutput.c	(working copy)
@@ -299,7 +299,7 @@ scan_translation_unit_directives_only (cpp_reader
   struct _cpp_dir_only_callbacks cb;
 
   cb.print_lines = print_lines_directives_only;
-  cb.maybe_print_line = (void (*) (source_location)) maybe_print_line;
+  cb.maybe_print_line = maybe_print_line;
 
   _cpp_preprocess_dir_only (pfile, &cb);
 }
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 253493)
+++ gcc/c-family/c.opt	(working copy)
@@ -384,6 +384,10 @@ Wc++17-compat
 C++ ObjC++ Var(warn_cxx17_compat) Warning LangEnabledBy(C++ ObjC++,Wall)
 Warn about C++ constructs whose meaning differs between ISO C++ 2014 and ISO C++ 2017.
 
+Wcast-function-type
+C ObjC C++ ObjC++ Var(warn_cast_function_type) Warning EnabledBy(Wextra)
+Warn about casts between incompatible function types.
+
 Wcast-qual
 C ObjC C++ ObjC++ Var(warn_cast_qual) Warning
 Warn about casts which discard qualifiers.
Index: gcc/cp/decl2.c
===================================================================
--- gcc/cp/decl2.c	(revision 253493)
+++ gcc/cp/decl2.c	(working copy)
@@ -3484,7 +3484,8 @@ start_static_storage_duration_function (unsigned c
       priority_info_map = splay_tree_new (splay_tree_compare_ints,
 					  /*delete_key_fn=*/0,
 					  /*delete_value_fn=*/
-					  (splay_tree_delete_value_fn) &free);
+					  (splay_tree_delete_value_fn)
+					  (void (*) (void)) free);
 
       /* We always need to generate functions for the
 	 DEFAULT_INIT_PRIORITY so enter it now.  That way when we walk
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 253493)
+++ gcc/cp/typeck.c	(working copy)
@@ -1173,6 +1173,53 @@ comp_template_parms_position (tree t1, tree t2)
   return true;
 }
 
+/* Heuristic check if two parameter types can be considered ABI-equivalent.  */
+
+static bool
+cxx_abi_equiv_type_p (tree t1, tree t2)
+{
+  t1 = TYPE_MAIN_VARIANT (t1);
+  t2 = TYPE_MAIN_VARIANT (t2);
+
+  if (TREE_CODE (t1) == POINTER_TYPE
+      && TREE_CODE (t2) == POINTER_TYPE)
+    return true;
+
+  if (INTEGRAL_TYPE_P (t1)
+      && INTEGRAL_TYPE_P (t2)
+      && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
+      && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
+	  || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
+    return true;
+
+  return same_type_p (t1, t2);
+}
+
+/* Check if a type cast between two function types can be considered safe.  */
+
+static bool
+cxx_safe_function_type_cast_p (tree t1, tree t2)
+{
+  if (TREE_TYPE (t1) == void_type_node &&
+      TYPE_ARG_TYPES (t1) == void_list_node)
+    return true;
+
+  if (TREE_TYPE (t2) == void_type_node &&
+      TYPE_ARG_TYPES (t2) == void_list_node)
+    return true;
+
+  if (!cxx_abi_equiv_type_p (TREE_TYPE (t1), TREE_TYPE (t2)))
+    return false;
+
+  for (t1 = TYPE_ARG_TYPES (t1), t2 = TYPE_ARG_TYPES (t2);
+       t1 && t2;
+       t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2))
+    if (!cxx_abi_equiv_type_p (TREE_VALUE (t1), TREE_VALUE (t2)))
+      return false;
+
+  return true;
+}
+
 /* Subroutine in comptypes.  */
 
 static bool
@@ -7261,9 +7308,25 @@ build_reinterpret_cast_1 (tree type, tree expr, bo
 	   && same_type_p (type, intype))
     /* DR 799 */
     return rvalue (expr);
-  else if ((TYPE_PTRFN_P (type) && TYPE_PTRFN_P (intype))
-	   || (TYPE_PTRMEMFUNC_P (type) && TYPE_PTRMEMFUNC_P (intype)))
-    return build_nop (type, expr);
+  else if (TYPE_PTRFN_P (type) && TYPE_PTRFN_P (intype))
+    {
+      if ((complain & tf_warning)
+	  && !cxx_safe_function_type_cast_p (TREE_TYPE (type),
+					     TREE_TYPE (intype)))
+	warning (OPT_Wcast_function_type,
+		 "cast between incompatible function types"
+		 " from %qH to %qI", intype, type);
+      return build_nop (type, expr);
+    }
+  else if (TYPE_PTRMEMFUNC_P (type) && TYPE_PTRMEMFUNC_P (intype))
+    {
+      if ((complain & tf_warning)
+	  && !same_type_p (type, intype))
+	warning (OPT_Wcast_function_type,
+		 "cast between incompatible pointer to member types"
+		 " from %qH to %qI", intype, type);
+      return build_nop (type, expr);
+    }
   else if ((TYPE_PTRDATAMEM_P (type) && TYPE_PTRDATAMEM_P (intype))
 	   || (TYPE_PTROBV_P (type) && TYPE_PTROBV_P (intype)))
     {
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 253493)
+++ gcc/doc/invoke.texi	(working copy)
@@ -267,7 +267,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wno-builtin-declaration-mismatch @gol
 -Wno-builtin-macro-redefined  -Wc90-c99-compat  -Wc99-c11-compat @gol
 -Wc++-compat  -Wc++11-compat  -Wc++14-compat  @gol
--Wcast-align  -Wcast-align=strict  -Wcast-qual  @gol
+-Wcast-align  -Wcast-align=strict  -Wcast-function-type  -Wcast-qual  @gol
 -Wchar-subscripts  -Wchkp  -Wcatch-value  -Wcatch-value=@var{n} @gol
 -Wclobbered  -Wcomment  -Wconditionally-supported @gol
 -Wconversion  -Wcoverage-mismatch  -Wno-cpp  -Wdangling-else  -Wdate-time @gol
@@ -3899,6 +3899,7 @@ This enables some extra warning flags that are not
 name is still supported, but the newer name is more descriptive.)
 
 @gccoptlist{-Wclobbered  @gol
+-Wcast-function-type  @gol
 -Wempty-body  @gol
 -Wignored-qualifiers @gol
 -Wimplicit-fallthrough=3 @gol
@@ -5959,6 +5960,21 @@ Warn whenever a pointer is cast such that the requ
 target is increased.  For example, warn if a @code{char *} is cast to
 an @code{int *} regardless of the target machine.
 
+@item -Wcast-function-type
+@opindex Wcast-function-type
+@opindex Wno-cast-function-type
+Warn when a function pointer is cast to an incompatible function pointer.
+In a cast involving function types with a variable argument list only
+the types of initial arguments that are provided are considered.
+Any parameter of pointer-type matches any other pointer-type.  Any benign
+differences in integral types are ignored, like @code{int} vs. @code{long}
+on ILP32 targets.  Likewise type qualifiers are ignored.  The function
+type @code{void (*) (void);} is special and matches everything, which can
+be used to suppress this warning.
+In a cast involving pointer to member types this warning warns whenever
+the type cast is changing the pointer to member type.
+This warning is enabled by @option{-Wextra}.
+
 @item -Wwrite-strings
 @opindex Wwrite-strings
 @opindex Wno-write-strings
Index: gcc/recog.h
===================================================================
--- gcc/recog.h	(revision 253493)
+++ gcc/recog.h	(working copy)
@@ -294,7 +294,7 @@ struct insn_gen_fn
   typedef rtx_insn * (*f15) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
   typedef rtx_insn * (*f16) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
 
-  typedef f0 stored_funcptr;
+  typedef void (*stored_funcptr) (void);
 
   rtx_insn * operator () (void) const { return ((f0)func) (); }
   rtx_insn * operator () (rtx a0) const { return ((f1)func) (a0); }
Index: gcc/testsuite/c-c++-common/Wcast-function-type.c
===================================================================
--- gcc/testsuite/c-c++-common/Wcast-function-type.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wcast-function-type.c	(working copy)
@@ -0,0 +1,31 @@
+/* { dg-do compile } */
+/* { dg-options "-Wcast-function-type" } */
+
+int f(long);
+
+typedef int (f1)(long);
+typedef int (f2)(void*);
+#ifdef __cplusplus
+typedef int (f3)(...);
+typedef void (f4)(...);
+#else
+typedef int (f3)();
+typedef void (f4)();
+#endif
+typedef void (f5)(void);
+
+f1 *a;
+f2 *b;
+f3 *c;
+f4 *d;
+f5 *e;
+
+void
+foo (void)
+{
+  a = (f1 *) f; /* { dg-bogus   "incompatible function types" } */
+  b = (f2 *) f; /* { dg-warning "incompatible function types" } */
+  c = (f3 *) f; /* { dg-bogus   "incompatible function types" } */
+  d = (f4 *) f; /* { dg-warning "incompatible function types" } */
+  e = (f5 *) f; /* { dg-bogus   "incompatible function types" } */
+}
Index: gcc/testsuite/g++.dg/Wcast-function-type.C
===================================================================
--- gcc/testsuite/g++.dg/Wcast-function-type.C	(revision 0)
+++ gcc/testsuite/g++.dg/Wcast-function-type.C	(working copy)
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-Wcast-function-type" } */
+
+struct S
+{
+  void foo (int*);
+  void bar (int);
+};
+
+typedef void (S::*MF)(int);
+
+void
+foo (void)
+{
+  MF p1 = (MF)&S::foo; /* { dg-warning "pointer to member" } */
+  MF p2 = (MF)&S::bar; /* { dg-bogus   "pointer to member" } */
+}
Index: gcc/tree-dump.c
===================================================================
--- gcc/tree-dump.c	(revision 253493)
+++ gcc/tree-dump.c	(working copy)
@@ -735,7 +735,8 @@ dump_node (const_tree t, dump_flags_t flags, FILE
   di.flags = flags;
   di.node = t;
   di.nodes = splay_tree_new (splay_tree_compare_pointers, 0,
-			     (splay_tree_delete_value_fn) &free);
+			     (splay_tree_delete_value_fn)
+			     (void (*) (void)) free);
 
   /* Queue up the first node.  */
   queue (&di, t, DUMP_NONE);
Index: gcc/typed-splay-tree.h
===================================================================
--- gcc/typed-splay-tree.h	(revision 253493)
+++ gcc/typed-splay-tree.h	(working copy)
@@ -75,9 +75,12 @@ inline typed_splay_tree<KEY_TYPE, VALUE_TYPE>::
 		    delete_key_fn delete_key_fn,
 		    delete_value_fn delete_value_fn)
 {
-  m_inner = splay_tree_new ((splay_tree_compare_fn)compare_fn,
-			    (splay_tree_delete_key_fn)delete_key_fn,
-			    (splay_tree_delete_value_fn)delete_value_fn);
+  m_inner = splay_tree_new ((splay_tree_compare_fn)
+			    (void (*) (void)) compare_fn,
+			    (splay_tree_delete_key_fn)
+			    (void (*) (void)) delete_key_fn,
+			    (splay_tree_delete_value_fn)
+			    (void (*) (void)) delete_value_fn);
 }
 
 /* Destructor for typed_splay_tree <K, V>.  */
Index: libcpp/internal.h
===================================================================
--- libcpp/internal.h	(revision 253493)
+++ libcpp/internal.h	(working copy)
@@ -708,7 +708,7 @@ struct _cpp_dir_only_callbacks
 {
   /* Called to print a block of lines. */
   void (*print_lines) (int, const void *, size_t);
-  void (*maybe_print_line) (source_location);
+  bool (*maybe_print_line) (source_location);
 };
 
 extern void _cpp_preprocess_dir_only (cpp_reader *,

Re: [PATCH] Add a warning for invalid function casts

[Martin Sebor]

On 10/09/2017 04:30 PM, Bernd Edlinger wrote:
> On 10/09/17 20:34, Martin Sebor wrote:
>> On 10/09/2017 11:50 AM, Bernd Edlinger wrote:
>>> On 10/09/17 18:44, Martin Sebor wrote:
>>>> On 10/07/2017 10:48 AM, Bernd Edlinger wrote:
>>>>> Hi!
>>>>>
>>>>> I think I have now something useful, it has a few more heuristics
>>>>> added, to reduce the number of false-positives so that it
>>>>> is able to find real bugs, for instance in openssl it triggers
>>>>> at a function cast which has already a TODO on it.
>>>>>
>>>>> The heuristics are:
>>>>> - handle void (*)(void) as a wild-card function type.
>>>>> - ignore volatile, const qualifiers on parameters/return.
>>>>> - handle any pointers as equivalent.
>>>>> - handle integral types, enums, and booleans of same precision
>>>>>    and signedness as equivalent.
>>>>> - stop parameter validation at the first "...".
>>>>
>>>> These sound quite reasonable to me.  I have a reservation about
>>>> just one of them, and some comments about other aspects of the
>>>> warning.  Sorry if this seems like a lot.  I'm hoping you'll
>>>> find the feedback constructive.
>>>>
>>>> I don't think using void(*)(void) to suppress the warning is
>>>> a robust solution because it's not safe to call a function that
>>>> takes arguments through such a pointer (especially not if one
>>>> or more of the arguments is a pointer).  Depending on the ABI,
>>>> calling a function that expects arguments with none could also
>>>> mess up the stack as the callee may pop arguments that were
>>>> never passed to it.
>>>>
>>>
>>> This is of course only a heuristic, and if there is no warning
>>> that does not mean any guarantee that there can't be a problem
>>> at runtime.  The heuristic is only meant to separate the
>>> bad from the very bad type-cast.  In my personal opinion there
>>> is not a single good type cast.
>>
>> I agree.  Since the warning uses one kind of a cast as an escape
>> mechanism from the checking it should be one whose result can
>> the most likely be used to call the function without undefined
>> behavior.
>>
>> Since it's possible to call any function through a pointer to
>> a function with no arguments (simply by providing arguments of
>> matching types) it's a reasonable candidate.
>>
>> On the other hand, since it is not safe to call an arbitrary
>> function through void (*)(void), it's not as good a candidate.
>>
>> Another reason why I think a protoype-less function is a good
>> choice is because the alias and ifunc attributes already use it
>> as an escape mechanism from their type incompatibility warning.
>>
>
> I know of pre-existing code-bases where a type-cast to type:
> void (*) (void);
>
> .. is already used as a generic function pointer: libffi and
> libgo, I would not want to break these.

Why not fix them instead?  They're a part of GCC so it should
be straightforward.  It doesn't seem like a good tradeoff to
compromise the efficacy of the warning to accommodate a couple
of questionable use cases.

>
> Actually when I have a type:
> X (*) (...);
>
> I would like to make sure that the warning checks that
> only functions returning X are assigned.
>
> and for X (*) (Y, ....);
>
> I would like to check that anything returning X with
> first argument of type Y is assigned.
>
> There are code bases where such a scheme is used.
> For instance one that I myself maintain: the OPC/UA AnsiC Stack,
> where I have this type definition:
>
> typedef OpcUa_StatusCode (OpcUa_PfnInvokeService)(OpcUa_Endpoint
> hEndpoint, ...);

If GCC guarantees that a variadic function can safely be called
with a pointer to another variadic function it seems reasonable
to avoid warning on such conversions.  But I'm not sure I see
what bearing this use case has on the one with functions without
a prototype.  In C++, void (*)(...) is the equivalent of
void (*)() in C, and so any function can be cast to it with no
warning because it can be safely called by providing the right
arguments.  Ignoring the return type should likewise be safe.
This feels like a clean design, while using void (*)(void)
like an unnecessary exception.

> And this plays well together with this warning, because only
> functions are assigned that match up to the ...);
> Afterwards this pointer is cast back to the original signature,
> so everything is perfectly fine.

I tend to agree.

> Regarding the cast from pointer to member to function, I see also a
> warning without -Wpedantic:
> Warnung: converting from »void (S::*)(int*)« to »void (*)(int*)«
> [-Wpmf-conversions]
>     F *pf = (F*)&S::foo;
>                     ^~~
>
> And this one is even default-enabled, so I think that should be
> more than sufficient.

I agree.  It looks like this triggers -Wpedantic when -Wpedantic
is set and -Wpmf-conversions when it isn't.  (It seems odd but
adding it under yet another warning option wouldn't help.)

Warning on the other test case would, however, be useful:

   struct S { void foo (int*); };

   typedef void (S::*MF)(int);

   MF pmf = (MF)&S::foo;

Martin

Re: [PATCH] Add a warning for invalid function casts

[Joseph Myers]

On Tue, 10 Oct 2017, Martin Sebor wrote:

> > I know of pre-existing code-bases where a type-cast to type:
> > void (*) (void);
> > 
> > .. is already used as a generic function pointer: libffi and
> > libgo, I would not want to break these.
> 
> Why not fix them instead?  They're a part of GCC so it should
> be straightforward.  It doesn't seem like a good tradeoff to

Sometimes an interface needs to store an arbitrary function type for which 
an ABI-compliant call ends up being constructed at runtime from assembly 
language (or from non-C, in general).  That's the sort of thing libffi 
does - so it inherently needs to be able to take pointers to arbitrary 
function types, which thus need to be converted to a generic function 
type, and the de facto generic function pointer type in C is void (*) 
(void).  (C11 6.11.6 says "The use of function declarators with empty 
parentheses (not prototype-format parameter type declarators) is an 
obsolescent feature.", so void (*) () is best avoided.)  Likewise 
interfaces such as dlsym (which happens to return void * along with a 
special case in POSIX requiring conversions between void * and function 
pointers to work, but void (*) (void) is the natural type for such 
interfaces to use).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Add a warning for invalid function casts

[Martin Sebor]

On 10/10/2017 10:30 AM, Joseph Myers wrote:
> On Tue, 10 Oct 2017, Martin Sebor wrote:
>
>>> I know of pre-existing code-bases where a type-cast to type:
>>> void (*) (void);
>>>
>>> .. is already used as a generic function pointer: libffi and
>>> libgo, I would not want to break these.
>>
>> Why not fix them instead?  They're a part of GCC so it should
>> be straightforward.  It doesn't seem like a good tradeoff to
>
> Sometimes an interface needs to store an arbitrary function type for which
> an ABI-compliant call ends up being constructed at runtime from assembly
> language (or from non-C, in general).  That's the sort of thing libffi
> does - so it inherently needs to be able to take pointers to arbitrary
> function types, which thus need to be converted to a generic function
> type, and the de facto generic function pointer type in C is void (*)
> (void).  (C11 6.11.6 says "The use of function declarators with empty
> parentheses (not prototype-format parameter type declarators) is an
> obsolescent feature.", so void (*) () is best avoided.)  Likewise
> interfaces such as dlsym (which happens to return void * along with a
> special case in POSIX requiring conversions between void * and function
> pointers to work, but void (*) (void) is the natural type for such
> interfaces to use).

I agree that unprototyped functions are best avoided in cases
where type checking is needed (i.e., in most use cases).  I also
agree that "generic function pointers") are not uncommon and are
worth accommodating.

But I don't think that adopting an inferior mechanism for it when
a better alternative exists is helpful.

Calling a function that takes arguments via a void (*)(void)
is undefined not just on paper but also in practice, so the
resulting pointer from such a cast is unusable except to convert
to a compatible pointer.

On the other hand, it is well-defined or safe to use a void (*)()
to call essentially any function, so it is a superior choice for
this use case.  Even if the pointer is only used for storage and
never to call a function, the syntax is elegant and the intent
clear.

Martin

PS It would be useful if modern C provided a clean mechanism to
support generic function pointers rather that forcing users leery
of relying on obsolescent features to invent workarounds.  One
alternative is to unobsolesce unprototyped functions for this
use case (i.e., for pointers to functions).  Another might be to
invent some other syntax (e.g., void (*)(...) since it's accepted
in C++).  But using void(*)(void) feels like a hack to me.

Re: [PATCH] Add a warning for invalid function casts

[Joseph Myers]

On Tue, 10 Oct 2017, Martin Sebor wrote:

> Calling a function that takes arguments via a void (*)(void)
> is undefined not just on paper but also in practice, so the
> resulting pointer from such a cast is unusable except to convert
> to a compatible pointer.

That's the point of a generic pointer - it either gets cast to the actual 
type (or something ABI-compatible with the actual type) for calls, or 
called from non-C code which has the same effect.

> PS It would be useful if modern C provided a clean mechanism to
> support generic function pointers rather that forcing users leery
> of relying on obsolescent features to invent workarounds.  One

You can use any function pointer type whatever as your generic pointer 
(casting back to the original type for calls).  That you can cast from one 
function pointer type to another and back to the original type is 
guaranteed by ISO C.  That the result of the conversion is meaningful for 
calls from non-C languages is part of the customary practices of C 
implementations.  That the type used is void (*) (void) is a common 
convention used in C code dealing with generic function pointers (another 
convention, of course, is just using void * as in POSIX and relying on 
conversions between function pointers and void *).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Add a warning for invalid function casts

[Martin Sebor]

On 10/10/2017 03:48 PM, Joseph Myers wrote:
> On Tue, 10 Oct 2017, Martin Sebor wrote:
>
>> Calling a function that takes arguments via a void (*)(void)
>> is undefined not just on paper but also in practice, so the
>> resulting pointer from such a cast is unusable except to convert
>> to a compatible pointer.
>
> That's the point of a generic pointer - it either gets cast to the actual
> type (or something ABI-compatible with the actual type) for calls, or
> called from non-C code which has the same effect.

Sure.  But (as you note below) any function pointer can be used
as a generic function pointer.  There's nothing unique or special
about void (*)(void) to make it particularly suitable for this
purpose.

But we're conflating two use cases: 1) converting to a generic
function pointer that's never used to call a function, and 2)
converting to a function pointer that's used to call a function
not strictly compatible with it but where the ABI obviates
the incompatibility.

The ideal solution for 1) would be a function pointer that can
never be used to call a function (i.e., the void* equivalent
for functions).[X]

I argue the ideal solution for 2) is void (*)() (or something
like it).  It's superior to void(*)(void) because it makes its
intent in a cast clear: ignore the types of the target function's
formal arguments.  Contrast that with void(*)(void) which could
mean one of two things: a) follow the type system rules, or b)
treat it as special and ignore the type system.  Accepting (b)
when (a) so clearly expresses the intent of this use case seems
like a clearly inferior choice to me.

>> PS It would be useful if modern C provided a clean mechanism to
>> support generic function pointers rather that forcing users leery
>> of relying on obsolescent features to invent workarounds.  One
>
> You can use any function pointer type whatever as your generic pointer
> (casting back to the original type for calls).  That you can cast from one
> function pointer type to another and back to the original type is
> guaranteed by ISO C.  That the result of the conversion is meaningful for
> calls from non-C languages is part of the customary practices of C
> implementations.  That the type used is void (*) (void) is a common
> convention used in C code dealing with generic function pointers (another
> convention, of course, is just using void * as in POSIX and relying on
> conversions between function pointers and void *).

Right.  The problem Bernd and I are trying to solve is how to
distinguish the last one from the other two use cases on this
list:

1) a conversion of an arbitrary function to a generic pointer
    that's only used for storage but never to call the function
    directly, without converting it to the original function's
    type (see also [X] below),
2) a conversion of an arbitrary function to some strictly
    incompatible type that is nonetheless safe to use (by the
    ABI rules) to call the original function, and
3) all other conversions that are likely mistakes/bugs and
    that should be diagnosed.

My claim, again, is that using void(*)() for both (1) and (2)
above (or a moral equivalent of it, such as void(*)(...)) is
a superior solution than any other that we have seen, including
void(*)(void), because it clearly expresses the intent (no type
checking) and doesn't exclude any type from the set to check for
compatibility.

Incidentally, void(*)(void) in C++ is a poor choice for this
use case also because of the language's default function
arguments.  It's an easy mistake for a C++ programmer to make
to assume that given, say:

   void foo (const char *s = "...");

or for any other function that provides default values for all
its arguments, the function may be callable via void(*)(void):

   typedef void F (void);

   void (pf)(void) = (F*)foo;

by having the (default) function argument value(s) magically
substituted at the call site of:

    pf ();

Bu since that's not the case it would be helpful for the new
warning to detect this mistake.  By encouraging the use of

   typedef void F (...);

as the type of a pointer there is little chance of making such
a mistake.

Martin

[X] This can be function that takes an argument of an incomplete
type, such as:

   struct Incomplete;
   typedef void Uncallable (struct Incomplete);

Any function can safely be converted to Uncallable* without
the risk of being called with the wrong arguments.  The only
way to use an Uncallable* is to explicitly convert it to
a pointer to a function that can be called.

Re: [PATCH] Add a warning for invalid function casts

[Joseph Myers]

On Tue, 10 Oct 2017, Martin Sebor wrote:

> The ideal solution for 1) would be a function pointer that can
> never be used to call a function (i.e., the void* equivalent
> for functions).[X]

I don't think that's relevant.  The normal idiom for this in modern C 
code, if not just using void *, is void (*) (void), and since the warning 
is supposed to be avoiding excessive false positives and detecting the 
cases that are likely to be used for ABI-incompatible calls, the warning 
should allow void (*) (void) there.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Add a warning for invalid function casts

[Martin Sebor]

On 10/11/2017 11:26 AM, Joseph Myers wrote:
> On Tue, 10 Oct 2017, Martin Sebor wrote:
>
>> The ideal solution for 1) would be a function pointer that can
>> never be used to call a function (i.e., the void* equivalent
>> for functions).[X]
>
> I don't think that's relevant.  The normal idiom for this in modern C
> code, if not just using void *, is void (*) (void), and since the warning
> is supposed to be avoiding excessive false positives and detecting the
> cases that are likely to be used for ABI-incompatible calls, the warning
> should allow void (*) (void) there.

I think we'll just have to agree to disagree.  I'm not convinced
that using void(*)(void) for this is idiomatic or pervasive enough
to drive design decisions.  Bernd mentioned just libgo and libffi
as the code bases that do, and both you and I have noted that any
pointer type works equally well for this purpose.  The problem
with almost any type, including void(*) (void), is that they can
be misused to call the incompatible function.  Since the sole
purpose of this new warning is to help find those misuses,
excluding void(*)(void) from the checking is directly at odds
with its goal.

I would prefer not to design an unnecessary back door into
the implementation and compromise the effectiveness of the warning
for what's clearly an inferior choice made without fully considering
the risk of misusing the result.  Instead I hope the warning will
drive improvements to code to make its intent explicit.  In my view
that's a good thing even if the code works correctly today.

I don't know how much code there is out that uses void (*)(void)
as a generic function pointer that's never intended to be called.
but I wouldn't expect there to be so much of it to make my
suggestion unfeasible.  I could of course be wrong.  If I am,
we'd find out pretty quickly.

But I've exhausted my arguments and so I think it's time for
me to bow out of the discussion.

Martin

Re: [PATCH] Add a warning for invalid function casts

[Pedro Alves]

On 10/11/2017 06:58 PM, Martin Sebor wrote:
> On 10/11/2017 11:26 AM, Joseph Myers wrote:
>> On Tue, 10 Oct 2017, Martin Sebor wrote:
>>
>>> The ideal solution for 1) would be a function pointer that can
>>> never be used to call a function (i.e., the void* equivalent
>>> for functions).[X]
>>
>> I don't think that's relevant.  The normal idiom for this in modern C
>> code, if not just using void *, is void (*) (void), and since the warning
>> is supposed to be avoiding excessive false positives and detecting the
>> cases that are likely to be used for ABI-incompatible calls, the warning
>> should allow void (*) (void) there.
> 
> I think we'll just have to agree to disagree.  I'm not convinced
> that using void(*)(void) for this is idiomatic or pervasive enough
> to drive design decisions.  Bernd mentioned just libgo and libffi
> as the code bases that do, and both you and I have noted that any
> pointer type works equally well for this purpose.  The problem
> with almost any type, including void(*) (void), is that they can
> be misused to call the incompatible function.  Since the sole
> purpose of this new warning is to help find those misuses,
> excluding void(*)(void) from the checking is directly at odds
> with its goal.

Switching type-erased function pointers from "void(*)(void)"
to "void(*)()" (in C) substantially increases the risk of code
calling the type-erased pointer without casting it back by accident:

 extern void foo (int, int);
 typedef void (type_erased_func) (void);
 type_erased_func *func = (type_erased_func *) foo;

 int main ()
 {
   func (1, 2); // error: too many arguments
 }

vs:

 extern void foo (int, int);
 typedef void (type_erased_func) (); // note: void dropped
 type_erased_func *func = (type_erased_func *) foo;

 int main ()
 {
   func (1, 2); // whoops, now silently compiles
 }

I think it'd be good if this were weighed in as well.  If 'void ()'
is picked as the special type, then maybe the above could
be at least addressed in the documentation, and/or
diagnostics/notes.

Thanks,
Pedro Alves

Re: [PATCH] Add a warning for invalid function casts

[Pedro Alves]

On 10/11/2017 03:57 AM, Martin Sebor wrote:
> 
> 
> Incidentally, void(*)(void) in C++ is a poor choice for this
> use case also because of the language's default function
> arguments.  It's an easy mistake for a C++ programmer to make
> to assume that given, say:
> 
>   void foo (const char *s = "...");
> 
> or for any other function that provides default values for all
> its arguments, the function may be callable via void(*)(void):
> 
>   typedef void F (void);
> 
>   void (pf)(void) = (F*)foo;

I'd think it'd be much more common to write instead:

  typedef void F (void);

  F *pf = (F*)foo;

I.e., use the typedef on both sides of the assignment.

> 
> by having the (default) function argument value(s) magically
> substituted at the call site of:
> 
>    pf ();
> 
> Bu since that's not the case it would be helpful for the new
> warning to detect this mistake.  By encouraging the use of
> 
>   typedef void F (...);
> 
> as the type of a pointer there is little chance of making such
> a mistake.

(and then) I don't think I understand this rationale.
If users follow the advice, they'll end up with:

  void foo (const char *s = "...");
  typedef void F (...);
  F *pf = (F *)foo;
  pf ();

which still compiles silently and calls the foo
function incorrectly.

Thanks,
Pedro Alves

Re: [PATCH] Add a warning for invalid function casts

[Pedro Alves]

On 10/11/2017 03:57 AM, Martin Sebor wrote:
> 
> 
> [X] This can be function that takes an argument of an incomplete
> type, such as:
> 
>   struct Incomplete;
>   typedef void Uncallable (struct Incomplete);
> 
> Any function can safely be converted to Uncallable* without
> the risk of being called with the wrong arguments.  The only
> way to use an Uncallable* is to explicitly convert it to
> a pointer to a function that can be called.

OOC, did you consider trying to get something like that added
to C proper, to some standard header?  I don't imagine that it'd
be much objectionable, and having a standard type may help
tooling give better diagnostics and suggestions.

Thanks,
Pedro Alves

Re: [PATCH] Add a warning for invalid function casts

[Martin Sebor]

On 10/12/2017 05:52 AM, Pedro Alves wrote:
> On 10/11/2017 03:57 AM, Martin Sebor wrote:
>>
>>
>> [X] This can be function that takes an argument of an incomplete
>> type, such as:
>>
>>   struct Incomplete;
>>   typedef void Uncallable (struct Incomplete);
>>
>> Any function can safely be converted to Uncallable* without
>> the risk of being called with the wrong arguments.  The only
>> way to use an Uncallable* is to explicitly convert it to
>> a pointer to a function that can be called.
>
> OOC, did you consider trying to get something like that added
> to C proper, to some standard header?  I don't imagine that it'd
> be much objectionable, and having a standard type may help
> tooling give better diagnostics and suggestions.

Yes.  In light of this discussion I am thinking it might be
worthwhile to bring up the issue of generic function pointers
with WG14 for C2X.

Martin

Re: [PATCH] Add a warning for invalid function casts

[Joseph Myers]

On Thu, 12 Oct 2017, Martin Sebor wrote:

> Yes.  In light of this discussion I am thinking it might be
> worthwhile to bring up the issue of generic function pointers
> with WG14 for C2X.

I'm fine with the idea of having a standard solution that (unlike void (*) 
(void)) cannot be called at all without converting to another type.  I 
just maintain that void (*) (void) is the de facto idiom for this and so 
the warning should reflect this even in the future presence of such a 
standard solution (much as we e.g. handle trailing [1] arrays in structs 
as possibly being used as flexible array members in code using that as a 
C89/C++-compatible idiom rather than relying on C99 flexible array 
members).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Add a warning for invalid function casts

[Bernd Edlinger]

Ping...

for the latest version of my patch which can be found here:

https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00559.html


Thanks
Bernd.

On 10/10/17 00:30, Bernd Edlinger wrote:
> On 10/09/17 20:34, Martin Sebor wrote:
>> On 10/09/2017 11:50 AM, Bernd Edlinger wrote:
>>> On 10/09/17 18:44, Martin Sebor wrote:
>>>> On 10/07/2017 10:48 AM, Bernd Edlinger wrote:
>>>>> Hi!
>>>>>
>>>>> I think I have now something useful, it has a few more heuristics
>>>>> added, to reduce the number of false-positives so that it
>>>>> is able to find real bugs, for instance in openssl it triggers
>>>>> at a function cast which has already a TODO on it.
>>>>>
>>>>> The heuristics are:
>>>>> - handle void (*)(void) as a wild-card function type.
>>>>> - ignore volatile, const qualifiers on parameters/return.
>>>>> - handle any pointers as equivalent.
>>>>> - handle integral types, enums, and booleans of same precision
>>>>>     and signedness as equivalent.
>>>>> - stop parameter validation at the first "...".
>>>>
>>>> These sound quite reasonable to me.  I have a reservation about
>>>> just one of them, and some comments about other aspects of the
>>>> warning.  Sorry if this seems like a lot.  I'm hoping you'll
>>>> find the feedback constructive.
>>>>
>>>> I don't think using void(*)(void) to suppress the warning is
>>>> a robust solution because it's not safe to call a function that
>>>> takes arguments through such a pointer (especially not if one
>>>> or more of the arguments is a pointer).  Depending on the ABI,
>>>> calling a function that expects arguments with none could also
>>>> mess up the stack as the callee may pop arguments that were
>>>> never passed to it.
>>>>
>>>
>>> This is of course only a heuristic, and if there is no warning
>>> that does not mean any guarantee that there can't be a problem
>>> at runtime.  The heuristic is only meant to separate the
>>> bad from the very bad type-cast.  In my personal opinion there
>>> is not a single good type cast.
>>
>> I agree.  Since the warning uses one kind of a cast as an escape
>> mechanism from the checking it should be one whose result can
>> the most likely be used to call the function without undefined
>> behavior.
>>
>> Since it's possible to call any function through a pointer to
>> a function with no arguments (simply by providing arguments of
>> matching types) it's a reasonable candidate.
>>
>> On the other hand, since it is not safe to call an arbitrary
>> function through void (*)(void), it's not as good a candidate.
>>
>> Another reason why I think a protoype-less function is a good
>> choice is because the alias and ifunc attributes already use it
>> as an escape mechanism from their type incompatibility warning.
>>
> 
> I know of pre-existing code-bases where a type-cast to type:
> void (*) (void);
> 
> .. is already used as a generic function pointer: libffi and
> libgo, I would not want to break these.
> 
> Actually when I have a type:
> X (*) (...);
> 
> I would like to make sure that the warning checks that
> only functions returning X are assigned.
> 
> and for X (*) (Y, ....);
> 
> I would like to check that anything returning X with
> first argument of type Y is assigned.
> 
> There are code bases where such a scheme is used.
> For instance one that I myself maintain: the OPC/UA AnsiC Stack,
> where I have this type definition:
> 
> typedef OpcUa_StatusCode (OpcUa_PfnInvokeService)(OpcUa_Endpoint
> hEndpoint, ...);
> 
> And this plays well together with this warning, because only
> functions are assigned that match up to the ...);
> Afterwards this pointer is cast back to the original signature,
> so everything is perfectly fine.
> 
> Regarding the cast from pointer to member to function, I see also a
> warning without -Wpedantic:
> Warnung: converting from »void (S::*)(int*)« to »void (*)(int*)«
> [-Wpmf-conversions]
>      F *pf = (F*)&S::foo;
>                      ^~~
> 
> And this one is even default-enabled, so I think that should be
> more than sufficient.
> 
> I also changed the heuristic, so that your example with the enum should
> now work.  I did not add it to the test case, because it would
> break with -fshort-enums :(
> 
> Attached I have an updated patch that extends this warning to the
> pointer-to-member function cast, and relaxes the heuristic on the
> benign integral type differences a bit further.
> 
> 
> Is it OK for trunk after bootstrap and reg-testing?
> 
> 
> Thanks
> Bernd.
> 

Re: [PATCH] Add a warning for invalid function casts

[Joseph Myers]

On Mon, 9 Oct 2017, Bernd Edlinger wrote:

> +type @code{void (*) (void);} is special and matches everything, which can

The type name should not include ";".

The non-C++ parts of the patch are OK with that change.

-- 
Joseph S. Myers
joseph@codesourcery.com

[PING] [PATCH] Add a warning for invalid function casts

[Bernd Edlinger]

Ping...

for the C++ part of this patch:

https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00559.html


Thanks
Bernd.

> On 10/10/17 00:30, Bernd Edlinger wrote:
>> On 10/09/17 20:34, Martin Sebor wrote:
>>> On 10/09/2017 11:50 AM, Bernd Edlinger wrote:
>>>> On 10/09/17 18:44, Martin Sebor wrote:
>>>>> On 10/07/2017 10:48 AM, Bernd Edlinger wrote:
>>>>>> Hi!
>>>>>>
>>>>>> I think I have now something useful, it has a few more heuristics
>>>>>> added, to reduce the number of false-positives so that it
>>>>>> is able to find real bugs, for instance in openssl it triggers
>>>>>> at a function cast which has already a TODO on it.
>>>>>>
>>>>>> The heuristics are:
>>>>>> - handle void (*)(void) as a wild-card function type.
>>>>>> - ignore volatile, const qualifiers on parameters/return.
>>>>>> - handle any pointers as equivalent.
>>>>>> - handle integral types, enums, and booleans of same precision
>>>>>>     and signedness as equivalent.
>>>>>> - stop parameter validation at the first "...".
>>>>>
>>>>> These sound quite reasonable to me.  I have a reservation about
>>>>> just one of them, and some comments about other aspects of the
>>>>> warning.  Sorry if this seems like a lot.  I'm hoping you'll
>>>>> find the feedback constructive.
>>>>>
>>>>> I don't think using void(*)(void) to suppress the warning is
>>>>> a robust solution because it's not safe to call a function that
>>>>> takes arguments through such a pointer (especially not if one
>>>>> or more of the arguments is a pointer).  Depending on the ABI,
>>>>> calling a function that expects arguments with none could also
>>>>> mess up the stack as the callee may pop arguments that were
>>>>> never passed to it.
>>>>>
>>>>
>>>> This is of course only a heuristic, and if there is no warning
>>>> that does not mean any guarantee that there can't be a problem
>>>> at runtime.  The heuristic is only meant to separate the
>>>> bad from the very bad type-cast.  In my personal opinion there
>>>> is not a single good type cast.
>>>
>>> I agree.  Since the warning uses one kind of a cast as an escape
>>> mechanism from the checking it should be one whose result can
>>> the most likely be used to call the function without undefined
>>> behavior.
>>>
>>> Since it's possible to call any function through a pointer to
>>> a function with no arguments (simply by providing arguments of
>>> matching types) it's a reasonable candidate.
>>>
>>> On the other hand, since it is not safe to call an arbitrary
>>> function through void (*)(void), it's not as good a candidate.
>>>
>>> Another reason why I think a protoype-less function is a good
>>> choice is because the alias and ifunc attributes already use it
>>> as an escape mechanism from their type incompatibility warning.
>>>
>>
>> I know of pre-existing code-bases where a type-cast to type:
>> void (*) (void);
>>
>> .. is already used as a generic function pointer: libffi and
>> libgo, I would not want to break these.
>>
>> Actually when I have a type:
>> X (*) (...);
>>
>> I would like to make sure that the warning checks that
>> only functions returning X are assigned.
>>
>> and for X (*) (Y, ....);
>>
>> I would like to check that anything returning X with
>> first argument of type Y is assigned.
>>
>> There are code bases where such a scheme is used.
>> For instance one that I myself maintain: the OPC/UA AnsiC Stack,
>> where I have this type definition:
>>
>> typedef OpcUa_StatusCode (OpcUa_PfnInvokeService)(OpcUa_Endpoint
>> hEndpoint, ...);
>>
>> And this plays well together with this warning, because only
>> functions are assigned that match up to the ...);
>> Afterwards this pointer is cast back to the original signature,
>> so everything is perfectly fine.
>>
>> Regarding the cast from pointer to member to function, I see also a
>> warning without -Wpedantic:
>> Warnung: converting from »void (S::*)(int*)« to »void (*)(int*)«
>> [-Wpmf-conversions]
>>      F *pf = (F*)&S::foo;
>>                      ^~~
>>
>> And this one is even default-enabled, so I think that should be
>> more than sufficient.
>>
>> I also changed the heuristic, so that your example with the enum should
>> now work.  I did not add it to the test case, because it would
>> break with -fshort-enums :(
>>
>> Attached I have an updated patch that extends this warning to the
>> pointer-to-member function cast, and relaxes the heuristic on the
>> benign integral type differences a bit further.
>>
>>
>> Is it OK for trunk after bootstrap and reg-testing?
>>
>>
>> Thanks
>> Bernd.
>>

[PING**2] [PATCH] Add a warning for invalid function casts

[Bernd Edlinger]

Ping...

On 11/08/17 17:55, Bernd Edlinger wrote:
> Ping...
> 
> for the C++ part of this patch:
> 
> https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00559.html
> 
> 
> Thanks
> Bernd.
> 
>> On 10/10/17 00:30, Bernd Edlinger wrote:
>>> On 10/09/17 20:34, Martin Sebor wrote:
>>>> On 10/09/2017 11:50 AM, Bernd Edlinger wrote:
>>>>> On 10/09/17 18:44, Martin Sebor wrote:
>>>>>> On 10/07/2017 10:48 AM, Bernd Edlinger wrote:
>>>>>>> Hi!
>>>>>>>
>>>>>>> I think I have now something useful, it has a few more heuristics
>>>>>>> added, to reduce the number of false-positives so that it
>>>>>>> is able to find real bugs, for instance in openssl it triggers
>>>>>>> at a function cast which has already a TODO on it.
>>>>>>>
>>>>>>> The heuristics are:
>>>>>>> - handle void (*)(void) as a wild-card function type.
>>>>>>> - ignore volatile, const qualifiers on parameters/return.
>>>>>>> - handle any pointers as equivalent.
>>>>>>> - handle integral types, enums, and booleans of same precision
>>>>>>>     and signedness as equivalent.
>>>>>>> - stop parameter validation at the first "...".
>>>>>>
>>>>>> These sound quite reasonable to me.  I have a reservation about
>>>>>> just one of them, and some comments about other aspects of the
>>>>>> warning.  Sorry if this seems like a lot.  I'm hoping you'll
>>>>>> find the feedback constructive.
>>>>>>
>>>>>> I don't think using void(*)(void) to suppress the warning is
>>>>>> a robust solution because it's not safe to call a function that
>>>>>> takes arguments through such a pointer (especially not if one
>>>>>> or more of the arguments is a pointer).  Depending on the ABI,
>>>>>> calling a function that expects arguments with none could also
>>>>>> mess up the stack as the callee may pop arguments that were
>>>>>> never passed to it.
>>>>>>
>>>>>
>>>>> This is of course only a heuristic, and if there is no warning
>>>>> that does not mean any guarantee that there can't be a problem
>>>>> at runtime.  The heuristic is only meant to separate the
>>>>> bad from the very bad type-cast.  In my personal opinion there
>>>>> is not a single good type cast.
>>>>
>>>> I agree.  Since the warning uses one kind of a cast as an escape
>>>> mechanism from the checking it should be one whose result can
>>>> the most likely be used to call the function without undefined
>>>> behavior.
>>>>
>>>> Since it's possible to call any function through a pointer to
>>>> a function with no arguments (simply by providing arguments of
>>>> matching types) it's a reasonable candidate.
>>>>
>>>> On the other hand, since it is not safe to call an arbitrary
>>>> function through void (*)(void), it's not as good a candidate.
>>>>
>>>> Another reason why I think a protoype-less function is a good
>>>> choice is because the alias and ifunc attributes already use it
>>>> as an escape mechanism from their type incompatibility warning.
>>>>
>>>
>>> I know of pre-existing code-bases where a type-cast to type:
>>> void (*) (void);
>>>
>>> .. is already used as a generic function pointer: libffi and
>>> libgo, I would not want to break these.
>>>
>>> Actually when I have a type:
>>> X (*) (...);
>>>
>>> I would like to make sure that the warning checks that
>>> only functions returning X are assigned.
>>>
>>> and for X (*) (Y, ....);
>>>
>>> I would like to check that anything returning X with
>>> first argument of type Y is assigned.
>>>
>>> There are code bases where such a scheme is used.
>>> For instance one that I myself maintain: the OPC/UA AnsiC Stack,
>>> where I have this type definition:
>>>
>>> typedef OpcUa_StatusCode (OpcUa_PfnInvokeService)(OpcUa_Endpoint
>>> hEndpoint, ...);
>>>
>>> And this plays well together with this warning, because only
>>> functions are assigned that match up to the ...);
>>> Afterwards this pointer is cast back to the original signature,
>>> so everything is perfectly fine.
>>>
>>> Regarding the cast from pointer to member to function, I see also a
>>> warning without -Wpedantic:
>>> Warnung: converting from »void (S::*)(int*)« to »void (*)(int*)«
>>> [-Wpmf-conversions]
>>>      F *pf = (F*)&S::foo;
>>>                      ^~~
>>>
>>> And this one is even default-enabled, so I think that should be
>>> more than sufficient.
>>>
>>> I also changed the heuristic, so that your example with the enum should
>>> now work.  I did not add it to the test case, because it would
>>> break with -fshort-enums :(
>>>
>>> Attached I have an updated patch that extends this warning to the
>>> pointer-to-member function cast, and relaxes the heuristic on the
>>> benign integral type differences a bit further.
>>>
>>>
>>> Is it OK for trunk after bootstrap and reg-testing?
>>>
>>>
>>> Thanks
>>> Bernd.
>>>

Re: [PATCH] Add a warning for invalid function casts

[Jason Merrill]

On 10/09/2017 06:30 PM, Bernd Edlinger wrote:
> +/* Heuristic check if two parameter types can be considered ABI-equivalent.  */
> +
> +static bool
> +cxx_abi_equiv_type_p (tree t1, tree t2)

This name is too general for a function that is specifically for 
implementing a particular warning.

> +  if (INTEGRAL_TYPE_P (t1)
> +      && INTEGRAL_TYPE_P (t2)
> +      && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
> +      && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
> +	  || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
> +    return true;

This section needs a comment explaining what you're allowing and why.
> +  else if (TYPE_PTRMEMFUNC_P (type) && TYPE_PTRMEMFUNC_P (intype))
> +    {
> +      if ((complain & tf_warning)
> +	  && !same_type_p (type, intype))

Why not use cxx_safe_function_type_cast_p here, too? 
TYPE_PTRMEMFUNC_FN_TYPE will be helpful.

Jason

[PATCHv2] Add a warning for invalid function casts

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]

On 11/29/17 22:57, Jason Merrill wrote:
> On 10/09/2017 06:30 PM, Bernd Edlinger wrote:
>> +/* Heuristic check if two parameter types can be considered 
>> ABI-equivalent.  */
>> +
>> +static bool
>> +cxx_abi_equiv_type_p (tree t1, tree t2)
> 
> This name is too general for a function that is specifically for 
> implementing a particular warning.
> 

Okay, then I will name it cxx_safe_arg_type_equiv_p.

>> +  if (INTEGRAL_TYPE_P (t1)
>> +      && INTEGRAL_TYPE_P (t2)
>> +      && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
>> +      && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
>> +      || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
>> +    return true;
> 
> This section needs a comment explaining what you're allowing and why.

Okay. I will add a comment here:

   /* The signedness of the parameter matters only when an integral
      type smaller than int is promoted to int, otherwise only the
      precision of the parameter matters.
      This check should make sure that the callee does not see
      undefined values in argument registers.  */

Joseph, I assume your OK is still valid when I do these two changes
also in the C frontend.

>> +  else if (TYPE_PTRMEMFUNC_P (type) && TYPE_PTRMEMFUNC_P (intype))
>> +    {
>> +      if ((complain & tf_warning)
>> +      && !same_type_p (type, intype))
> 
> Why not use cxx_safe_function_type_cast_p here, too? 
> TYPE_PTRMEMFUNC_FN_TYPE will be helpful.
> 

Yes, that won't hurt.


So, is the attached patch OK for trunk, after the usual
bootstrap and reg-testing?


Thanks
Bernd.

[-- Attachment #2: changelog-cast-function-type.txt --]
[-- Type: text/plain, Size: 1285 bytes --]

gcc:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* doc/invoke.texi: Document -Wcast-function-type.
	* recog.h (stored_funcptr): Change signature.
	* tree-dump.c (dump_node): Avoid warning.
	* typed-splay-tree.h (typed_splay_tree): Avoid warning.

libcpp:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* internal.h (maybe_print_line): Change signature.
	
c-family:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c.opt (Wcast-function-type): New warning option.
	* c-lex.c (get_fileinfo): Avoid warning.
	* c-ppoutput.c (scan_translation_unit_directives_only): Remove cast.

c:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-typeck.c (c_safe_arg_type_equiv_p,
	c_safe_function_type_cast_p): New function.
	(build_c_cast): Implement -Wcast-function-type.

cp:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* decl2.c (start_static_storage_duration_function): Avoid warning.
	* typeck.c (cxx_safe_arg_type_equiv_p,
	cxx_safe_function_type_cast_p): New function.
	(build_reinterpret_cast_1): Implement -Wcast-function-type.

testsuite:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-c++-common/Wcast-function-type.c: New test.
	* g++.dg/Wcast-function-type.C: New test.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: patch-cast-function-type.diff --]
[-- Type: text/x-patch; name="patch-cast-function-type.diff", Size: 14676 bytes --]

Index: gcc/c/c-typeck.c
===================================================================
--- gcc/c/c-typeck.c	(revision 255270)
+++ gcc/c/c-typeck.c	(working copy)
@@ -5452,6 +5452,58 @@ handle_warn_cast_qual (location_t loc, tree type,
   while (TREE_CODE (in_type) == POINTER_TYPE);
 }
 
+/* Heuristic check if two parameter types can be considered ABI-equivalent.  */
+
+static bool
+c_safe_arg_type_equiv_p (tree t1, tree t2)
+{
+  t1 = TYPE_MAIN_VARIANT (t1);
+  t2 = TYPE_MAIN_VARIANT (t2);
+
+  if (TREE_CODE (t1) == POINTER_TYPE
+      && TREE_CODE (t2) == POINTER_TYPE)
+    return true;
+
+  /* The signedness of the parameter matters only when an integral
+     type smaller than int is promoted to int, otherwise only the
+     precision of the parameter matters.
+     This check should make sure that the callee does not see
+     undefined values in argument registers.  */
+  if (INTEGRAL_TYPE_P (t1)
+      && INTEGRAL_TYPE_P (t2)
+      && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
+      && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
+	  || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
+    return true;
+
+  return comptypes (t1, t2);
+}
+
+/* Check if a type cast between two function types can be considered safe.  */
+
+static bool
+c_safe_function_type_cast_p (tree t1, tree t2)
+{
+  if (TREE_TYPE (t1) == void_type_node &&
+      TYPE_ARG_TYPES (t1) == void_list_node)
+    return true;
+
+  if (TREE_TYPE (t2) == void_type_node &&
+      TYPE_ARG_TYPES (t2) == void_list_node)
+    return true;
+
+  if (!c_safe_arg_type_equiv_p (TREE_TYPE (t1), TREE_TYPE (t2)))
+    return false;
+
+  for (t1 = TYPE_ARG_TYPES (t1), t2 = TYPE_ARG_TYPES (t2);
+       t1 && t2;
+       t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2))
+    if (!c_safe_arg_type_equiv_p (TREE_VALUE (t1), TREE_VALUE (t2)))
+      return false;
+
+  return true;
+}
+
 /* Build an expression representing a cast to type TYPE of expression EXPR.
    LOC is the location of the cast-- typically the open paren of the cast.  */
 
@@ -5645,6 +5697,16 @@ build_c_cast (location_t loc, tree type, tree expr
 	pedwarn (loc, OPT_Wpedantic, "ISO C forbids "
 		 "conversion of object pointer to function pointer type");
 
+      if (TREE_CODE (type) == POINTER_TYPE
+	  && TREE_CODE (otype) == POINTER_TYPE
+	  && TREE_CODE (TREE_TYPE (type)) == FUNCTION_TYPE
+	  && TREE_CODE (TREE_TYPE (otype)) == FUNCTION_TYPE
+	  && !c_safe_function_type_cast_p (TREE_TYPE (type),
+					   TREE_TYPE (otype)))
+	warning_at (loc, OPT_Wcast_function_type,
+		    "cast between incompatible function types"
+		    " from %qT to %qT", otype, type);
+
       ovalue = value;
       value = convert (type, value);
 
Index: gcc/c-family/c-lex.c
===================================================================
--- gcc/c-family/c-lex.c	(revision 255270)
+++ gcc/c-family/c-lex.c	(working copy)
@@ -101,9 +101,11 @@ get_fileinfo (const char *name)
   struct c_fileinfo *fi;
 
   if (!file_info_tree)
-    file_info_tree = splay_tree_new ((splay_tree_compare_fn) strcmp,
+    file_info_tree = splay_tree_new ((splay_tree_compare_fn)
+				     (void (*) (void)) strcmp,
 				     0,
-				     (splay_tree_delete_value_fn) free);
+				     (splay_tree_delete_value_fn)
+				     (void (*) (void)) free);
 
   n = splay_tree_lookup (file_info_tree, (splay_tree_key) name);
   if (n)
Index: gcc/c-family/c-ppoutput.c
===================================================================
--- gcc/c-family/c-ppoutput.c	(revision 255270)
+++ gcc/c-family/c-ppoutput.c	(working copy)
@@ -299,7 +299,7 @@ scan_translation_unit_directives_only (cpp_reader
   struct _cpp_dir_only_callbacks cb;
 
   cb.print_lines = print_lines_directives_only;
-  cb.maybe_print_line = (void (*) (source_location)) maybe_print_line;
+  cb.maybe_print_line = maybe_print_line;
 
   _cpp_preprocess_dir_only (pfile, &cb);
 }
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 255270)
+++ gcc/c-family/c.opt	(working copy)
@@ -384,6 +384,10 @@ Wc++17-compat
 C++ ObjC++ Var(warn_cxx17_compat) Warning LangEnabledBy(C++ ObjC++,Wall)
 Warn about C++ constructs whose meaning differs between ISO C++ 2014 and ISO C++ 2017.
 
+Wcast-function-type
+C ObjC C++ ObjC++ Var(warn_cast_function_type) Warning EnabledBy(Wextra)
+Warn about casts between incompatible function types.
+
 Wcast-qual
 C ObjC C++ ObjC++ Var(warn_cast_qual) Warning
 Warn about casts which discard qualifiers.
Index: gcc/cp/decl2.c
===================================================================
--- gcc/cp/decl2.c	(revision 255270)
+++ gcc/cp/decl2.c	(working copy)
@@ -3525,7 +3525,8 @@ start_static_storage_duration_function (unsigned c
       priority_info_map = splay_tree_new (splay_tree_compare_ints,
 					  /*delete_key_fn=*/0,
 					  /*delete_value_fn=*/
-					  (splay_tree_delete_value_fn) &free);
+					  (splay_tree_delete_value_fn)
+					  (void (*) (void)) free);
 
       /* We always need to generate functions for the
 	 DEFAULT_INIT_PRIORITY so enter it now.  That way when we walk
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 255270)
+++ gcc/cp/typeck.c	(working copy)
@@ -1173,6 +1173,58 @@ comp_template_parms_position (tree t1, tree t2)
   return true;
 }
 
+/* Heuristic check if two parameter types can be considered ABI-equivalent.  */
+
+static bool
+cxx_safe_arg_type_equiv_p (tree t1, tree t2)
+{
+  t1 = TYPE_MAIN_VARIANT (t1);
+  t2 = TYPE_MAIN_VARIANT (t2);
+
+  if (TREE_CODE (t1) == POINTER_TYPE
+      && TREE_CODE (t2) == POINTER_TYPE)
+    return true;
+
+  /* The signedness of the parameter matters only when an integral
+     type smaller than int is promoted to int, otherwise only the
+     precision of the parameter matters.
+     This check should make sure that the callee does not see
+     undefined values in argument registers.  */
+  if (INTEGRAL_TYPE_P (t1)
+      && INTEGRAL_TYPE_P (t2)
+      && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
+      && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
+	  || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
+    return true;
+
+  return same_type_p (t1, t2);
+}
+
+/* Check if a type cast between two function types can be considered safe.  */
+
+static bool
+cxx_safe_function_type_cast_p (tree t1, tree t2)
+{
+  if (TREE_TYPE (t1) == void_type_node &&
+      TYPE_ARG_TYPES (t1) == void_list_node)
+    return true;
+
+  if (TREE_TYPE (t2) == void_type_node &&
+      TYPE_ARG_TYPES (t2) == void_list_node)
+    return true;
+
+  if (!cxx_safe_arg_type_equiv_p (TREE_TYPE (t1), TREE_TYPE (t2)))
+    return false;
+
+  for (t1 = TYPE_ARG_TYPES (t1), t2 = TYPE_ARG_TYPES (t2);
+       t1 && t2;
+       t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2))
+    if (!cxx_safe_arg_type_equiv_p (TREE_VALUE (t1), TREE_VALUE (t2)))
+      return false;
+
+  return true;
+}
+
 /* Subroutine in comptypes.  */
 
 static bool
@@ -7297,9 +7349,27 @@ build_reinterpret_cast_1 (tree type, tree expr, bo
 	   && same_type_p (type, intype))
     /* DR 799 */
     return rvalue (expr);
-  else if ((TYPE_PTRFN_P (type) && TYPE_PTRFN_P (intype))
-	   || (TYPE_PTRMEMFUNC_P (type) && TYPE_PTRMEMFUNC_P (intype)))
-    return build_nop (type, expr);
+  else if (TYPE_PTRFN_P (type) && TYPE_PTRFN_P (intype))
+    {
+      if ((complain & tf_warning)
+	  && !cxx_safe_function_type_cast_p (TREE_TYPE (type),
+					     TREE_TYPE (intype)))
+	warning (OPT_Wcast_function_type,
+		 "cast between incompatible function types"
+		 " from %qH to %qI", intype, type);
+      return build_nop (type, expr);
+    }
+  else if (TYPE_PTRMEMFUNC_P (type) && TYPE_PTRMEMFUNC_P (intype))
+    {
+      if ((complain & tf_warning)
+	  && !cxx_safe_function_type_cast_p
+		(TREE_TYPE (TYPE_PTRMEMFUNC_FN_TYPE_RAW (type)),
+		 TREE_TYPE (TYPE_PTRMEMFUNC_FN_TYPE_RAW (intype))))
+	warning (OPT_Wcast_function_type,
+		 "cast between incompatible pointer to member types"
+		 " from %qH to %qI", intype, type);
+      return build_nop (type, expr);
+    }
   else if ((TYPE_PTRDATAMEM_P (type) && TYPE_PTRDATAMEM_P (intype))
 	   || (TYPE_PTROBV_P (type) && TYPE_PTROBV_P (intype)))
     {
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 255270)
+++ gcc/doc/invoke.texi	(working copy)
@@ -267,7 +267,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wno-builtin-declaration-mismatch @gol
 -Wno-builtin-macro-redefined  -Wc90-c99-compat  -Wc99-c11-compat @gol
 -Wc++-compat  -Wc++11-compat  -Wc++14-compat  @gol
--Wcast-align  -Wcast-align=strict  -Wcast-qual  @gol
+-Wcast-align  -Wcast-align=strict  -Wcast-function-type  -Wcast-qual  @gol
 -Wchar-subscripts  -Wchkp  -Wcatch-value  -Wcatch-value=@var{n} @gol
 -Wclobbered  -Wcomment  -Wconditionally-supported @gol
 -Wconversion  -Wcoverage-mismatch  -Wno-cpp  -Wdangling-else  -Wdate-time @gol
@@ -3904,6 +3904,7 @@ This enables some extra warning flags that are not
 name is still supported, but the newer name is more descriptive.)
 
 @gccoptlist{-Wclobbered  @gol
+-Wcast-function-type  @gol
 -Wempty-body  @gol
 -Wignored-qualifiers @gol
 -Wimplicit-fallthrough=3 @gol
@@ -6033,6 +6034,21 @@ Warn whenever a pointer is cast such that the requ
 target is increased.  For example, warn if a @code{char *} is cast to
 an @code{int *} regardless of the target machine.
 
+@item -Wcast-function-type
+@opindex Wcast-function-type
+@opindex Wno-cast-function-type
+Warn when a function pointer is cast to an incompatible function pointer.
+In a cast involving function types with a variable argument list only
+the types of initial arguments that are provided are considered.
+Any parameter of pointer-type matches any other pointer-type.  Any benign
+differences in integral types are ignored, like @code{int} vs. @code{long}
+on ILP32 targets.  Likewise type qualifiers are ignored.  The function
+type @code{void (*) (void)} is special and matches everything, which can
+be used to suppress this warning.
+In a cast involving pointer to member types this warning warns whenever
+the type cast is changing the pointer to member type.
+This warning is enabled by @option{-Wextra}.
+
 @item -Wwrite-strings
 @opindex Wwrite-strings
 @opindex Wno-write-strings
Index: gcc/recog.h
===================================================================
--- gcc/recog.h	(revision 255270)
+++ gcc/recog.h	(working copy)
@@ -294,7 +294,7 @@ struct insn_gen_fn
   typedef rtx_insn * (*f15) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
   typedef rtx_insn * (*f16) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
 
-  typedef f0 stored_funcptr;
+  typedef void (*stored_funcptr) (void);
 
   rtx_insn * operator () (void) const { return ((f0)func) (); }
   rtx_insn * operator () (rtx a0) const { return ((f1)func) (a0); }
Index: gcc/testsuite/c-c++-common/Wcast-function-type.c
===================================================================
--- gcc/testsuite/c-c++-common/Wcast-function-type.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wcast-function-type.c	(working copy)
@@ -0,0 +1,31 @@
+/* { dg-do compile } */
+/* { dg-options "-Wcast-function-type" } */
+
+int f(long);
+
+typedef int (f1)(long);
+typedef int (f2)(void*);
+#ifdef __cplusplus
+typedef int (f3)(...);
+typedef void (f4)(...);
+#else
+typedef int (f3)();
+typedef void (f4)();
+#endif
+typedef void (f5)(void);
+
+f1 *a;
+f2 *b;
+f3 *c;
+f4 *d;
+f5 *e;
+
+void
+foo (void)
+{
+  a = (f1 *) f; /* { dg-bogus   "incompatible function types" } */
+  b = (f2 *) f; /* { dg-warning "incompatible function types" } */
+  c = (f3 *) f; /* { dg-bogus   "incompatible function types" } */
+  d = (f4 *) f; /* { dg-warning "incompatible function types" } */
+  e = (f5 *) f; /* { dg-bogus   "incompatible function types" } */
+}
Index: gcc/testsuite/g++.dg/Wcast-function-type.C
===================================================================
--- gcc/testsuite/g++.dg/Wcast-function-type.C	(revision 0)
+++ gcc/testsuite/g++.dg/Wcast-function-type.C	(working copy)
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-Wcast-function-type" } */
+
+struct S
+{
+  void foo (int*);
+  void bar (int);
+};
+
+typedef void (S::*MF)(int);
+
+void
+foo (void)
+{
+  MF p1 = (MF)&S::foo; /* { dg-warning "pointer to member" } */
+  MF p2 = (MF)&S::bar; /* { dg-bogus   "pointer to member" } */
+}
Index: gcc/tree-dump.c
===================================================================
--- gcc/tree-dump.c	(revision 255270)
+++ gcc/tree-dump.c	(working copy)
@@ -736,7 +736,8 @@ dump_node (const_tree t, dump_flags_t flags, FILE
   di.flags = flags;
   di.node = t;
   di.nodes = splay_tree_new (splay_tree_compare_pointers, 0,
-			     (splay_tree_delete_value_fn) &free);
+			     (splay_tree_delete_value_fn)
+			     (void (*) (void)) free);
 
   /* Queue up the first node.  */
   queue (&di, t, DUMP_NONE);
Index: gcc/typed-splay-tree.h
===================================================================
--- gcc/typed-splay-tree.h	(revision 255270)
+++ gcc/typed-splay-tree.h	(working copy)
@@ -75,9 +75,12 @@ inline typed_splay_tree<KEY_TYPE, VALUE_TYPE>::
 		    delete_key_fn delete_key_fn,
 		    delete_value_fn delete_value_fn)
 {
-  m_inner = splay_tree_new ((splay_tree_compare_fn)compare_fn,
-			    (splay_tree_delete_key_fn)delete_key_fn,
-			    (splay_tree_delete_value_fn)delete_value_fn);
+  m_inner = splay_tree_new ((splay_tree_compare_fn)
+			    (void (*) (void)) compare_fn,
+			    (splay_tree_delete_key_fn)
+			    (void (*) (void)) delete_key_fn,
+			    (splay_tree_delete_value_fn)
+			    (void (*) (void)) delete_value_fn);
 }
 
 /* Destructor for typed_splay_tree <K, V>.  */
Index: libcpp/internal.h
===================================================================
--- libcpp/internal.h	(revision 255270)
+++ libcpp/internal.h	(working copy)
@@ -709,7 +709,7 @@ struct _cpp_dir_only_callbacks
 {
   /* Called to print a block of lines. */
   void (*print_lines) (int, const void *, size_t);
-  void (*maybe_print_line) (source_location);
+  bool (*maybe_print_line) (source_location);
 };
 
 extern void _cpp_preprocess_dir_only (cpp_reader *,

Re: [PATCHv2] Add a warning for invalid function casts

[Jason Merrill]

On Thu, Nov 30, 2017 at 10:14 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> On 11/29/17 22:57, Jason Merrill wrote:
>> On 10/09/2017 06:30 PM, Bernd Edlinger wrote:

>>> +  if (INTEGRAL_TYPE_P (t1)
>>> +      && INTEGRAL_TYPE_P (t2)
>>> +      && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
>>> +      && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
>>> +      || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
>>> +    return true;
>>
>> This section needs a comment explaining what you're allowing and why.
>
> Okay. I will add a comment here:
>
>    /* The signedness of the parameter matters only when an integral
>       type smaller than int is promoted to int, otherwise only the
>       precision of the parameter matters.
>       This check should make sure that the callee does not see
>       undefined values in argument registers.  */

If we're thinking about argument promotion, should this use
type_passed_as rather than assume promotion to int?

Jason

Re: [PATCHv2] Add a warning for invalid function casts

[Bernd Edlinger]

On 11/30/17 16:45, Jason Merrill wrote:
> On Thu, Nov 30, 2017 at 10:14 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> On 11/29/17 22:57, Jason Merrill wrote:
>>> On 10/09/2017 06:30 PM, Bernd Edlinger wrote:
> 
>>>> +  if (INTEGRAL_TYPE_P (t1)
>>>> +      && INTEGRAL_TYPE_P (t2)
>>>> +      && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
>>>> +      && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
>>>> +      || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
>>>> +    return true;
>>>
>>> This section needs a comment explaining what you're allowing and why.
>>
>> Okay. I will add a comment here:
>>
>>     /* The signedness of the parameter matters only when an integral
>>        type smaller than int is promoted to int, otherwise only the
>>        precision of the parameter matters.
>>        This check should make sure that the callee does not see
>>        undefined values in argument registers.  */
> 
> If we're thinking about argument promotion, should this use
> type_passed_as rather than assume promotion to int?
> 

I don't know, it is only a heuristic after all, and even if there is no
warning for a bogus type cast that does not mean any
correctness-guarantee at all.

Would type_passed_as make any difference for integral types?


Bernd.

Re: [PATCHv2] Add a warning for invalid function casts

[Jason Merrill]

On Thu, Nov 30, 2017 at 11:07 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> On 11/30/17 16:45, Jason Merrill wrote:
>> On Thu, Nov 30, 2017 at 10:14 AM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> On 11/29/17 22:57, Jason Merrill wrote:
>>>> On 10/09/2017 06:30 PM, Bernd Edlinger wrote:
>>
>>>>> +  if (INTEGRAL_TYPE_P (t1)
>>>>> +      && INTEGRAL_TYPE_P (t2)
>>>>> +      && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
>>>>> +      && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
>>>>> +      || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
>>>>> +    return true;
>>>>
>>>> This section needs a comment explaining what you're allowing and why.
>>>
>>> Okay. I will add a comment here:
>>>
>>>     /* The signedness of the parameter matters only when an integral
>>>        type smaller than int is promoted to int, otherwise only the
>>>        precision of the parameter matters.
>>>        This check should make sure that the callee does not see
>>>        undefined values in argument registers.  */
>>
>> If we're thinking about argument promotion, should this use
>> type_passed_as rather than assume promotion to int?
>
> I don't know, it is only a heuristic after all, and even if there is no
> warning for a bogus type cast that does not mean any
> correctness-guarantee at all.
>
> Would type_passed_as make any difference for integral types?

Yes, type_passed_as expresses promotion to int on targets that want
that behavior.

Jason

Re: [PATCHv2] Add a warning for invalid function casts

[Bernd Edlinger]

On 11/30/17 18:29, Jason Merrill wrote:
> On Thu, Nov 30, 2017 at 11:07 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> On 11/30/17 16:45, Jason Merrill wrote:
>>> On Thu, Nov 30, 2017 at 10:14 AM, Bernd Edlinger
>>> <bernd.edlinger@hotmail.de> wrote:
>>>> On 11/29/17 22:57, Jason Merrill wrote:
>>>>> On 10/09/2017 06:30 PM, Bernd Edlinger wrote:
>>>
>>>>>> +  if (INTEGRAL_TYPE_P (t1)
>>>>>> +      && INTEGRAL_TYPE_P (t2)
>>>>>> +      && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
>>>>>> +      && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
>>>>>> +      || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
>>>>>> +    return true;
>>>>>
>>>>> This section needs a comment explaining what you're allowing and why.
>>>>
>>>> Okay. I will add a comment here:
>>>>
>>>>      /* The signedness of the parameter matters only when an integral
>>>>         type smaller than int is promoted to int, otherwise only the
>>>>         precision of the parameter matters.
>>>>         This check should make sure that the callee does not see
>>>>         undefined values in argument registers.  */
>>>
>>> If we're thinking about argument promotion, should this use
>>> type_passed_as rather than assume promotion to int?
>>
>> I don't know, it is only a heuristic after all, and even if there is no
>> warning for a bogus type cast that does not mean any
>> correctness-guarantee at all.
>>
>> Would type_passed_as make any difference for integral types?
> 
> Yes, type_passed_as expresses promotion to int on targets that want
> that behavior.
> 

Hmm, I see, mostly arm, sh and msp430 (whatever that may be).

So how would you like this:

   if (INTEGRAL_TYPE_P (t1)
       && INTEGRAL_TYPE_P (t2)
       && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
       && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
           || !targetm.calls.promote_prototypes (t1)
           || !targetm.calls.promote_prototypes (t2)
           || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))


Thanks
Bernd.

Re: [PATCHv2] Add a warning for invalid function casts

[Jason Merrill]

On Thu, Nov 30, 2017 at 12:55 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> On 11/30/17 18:29, Jason Merrill wrote:
>> On Thu, Nov 30, 2017 at 11:07 AM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> On 11/30/17 16:45, Jason Merrill wrote:
>>>> On Thu, Nov 30, 2017 at 10:14 AM, Bernd Edlinger
>>>> <bernd.edlinger@hotmail.de> wrote:
>>>>> On 11/29/17 22:57, Jason Merrill wrote:
>>>>>> On 10/09/2017 06:30 PM, Bernd Edlinger wrote:
>>>>
>>>>>>> +  if (INTEGRAL_TYPE_P (t1)
>>>>>>> +      && INTEGRAL_TYPE_P (t2)
>>>>>>> +      && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
>>>>>>> +      && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
>>>>>>> +      || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
>>>>>>> +    return true;
>>>>>>
>>>>>> This section needs a comment explaining what you're allowing and why.
>>>>>
>>>>> Okay. I will add a comment here:
>>>>>
>>>>>      /* The signedness of the parameter matters only when an integral
>>>>>         type smaller than int is promoted to int, otherwise only the
>>>>>         precision of the parameter matters.
>>>>>         This check should make sure that the callee does not see
>>>>>         undefined values in argument registers.  */
>>>>
>>>> If we're thinking about argument promotion, should this use
>>>> type_passed_as rather than assume promotion to int?
>>>
>>> I don't know, it is only a heuristic after all, and even if there is no
>>> warning for a bogus type cast that does not mean any
>>> correctness-guarantee at all.
>>>
>>> Would type_passed_as make any difference for integral types?
>>
>> Yes, type_passed_as expresses promotion to int on targets that want
>> that behavior.
>>
>
> Hmm, I see, mostly arm, sh and msp430 (whatever that may be).
>
> So how would you like this:
>
>    if (INTEGRAL_TYPE_P (t1)
>        && INTEGRAL_TYPE_P (t2)
>        && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
>        && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
>            || !targetm.calls.promote_prototypes (t1)
>            || !targetm.calls.promote_prototypes (t2)
>            || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))

I was thinking

        && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
            || type_passed_as (t1) == t1))

Jason

Re: [PATCHv2] Add a warning for invalid function casts

[Bernd Edlinger]

On 11/30/17 19:05, Jason Merrill wrote:
> On Thu, Nov 30, 2017 at 12:55 PM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> On 11/30/17 18:29, Jason Merrill wrote:
>>> On Thu, Nov 30, 2017 at 11:07 AM, Bernd Edlinger
>>> <bernd.edlinger@hotmail.de> wrote:
>>>> On 11/30/17 16:45, Jason Merrill wrote:
>>>>> On Thu, Nov 30, 2017 at 10:14 AM, Bernd Edlinger
>>>>> <bernd.edlinger@hotmail.de> wrote:
>>>>>> On 11/29/17 22:57, Jason Merrill wrote:
>>>>>>> On 10/09/2017 06:30 PM, Bernd Edlinger wrote:
>>>>>
>>>>>>>> +  if (INTEGRAL_TYPE_P (t1)
>>>>>>>> +      && INTEGRAL_TYPE_P (t2)
>>>>>>>> +      && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
>>>>>>>> +      && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
>>>>>>>> +      || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
>>>>>>>> +    return true;
>>>>>>>
>>>>>>> This section needs a comment explaining what you're allowing and why.
>>>>>>
>>>>>> Okay. I will add a comment here:
>>>>>>
>>>>>>       /* The signedness of the parameter matters only when an integral
>>>>>>          type smaller than int is promoted to int, otherwise only the
>>>>>>          precision of the parameter matters.
>>>>>>          This check should make sure that the callee does not see
>>>>>>          undefined values in argument registers.  */
>>>>>
>>>>> If we're thinking about argument promotion, should this use
>>>>> type_passed_as rather than assume promotion to int?
>>>>
>>>> I don't know, it is only a heuristic after all, and even if there is no
>>>> warning for a bogus type cast that does not mean any
>>>> correctness-guarantee at all.
>>>>
>>>> Would type_passed_as make any difference for integral types?
>>>
>>> Yes, type_passed_as expresses promotion to int on targets that want
>>> that behavior.
>>>
>>
>> Hmm, I see, mostly arm, sh and msp430 (whatever that may be).
>>
>> So how would you like this:
>>
>>     if (INTEGRAL_TYPE_P (t1)
>>         && INTEGRAL_TYPE_P (t2)
>>         && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
>>         && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
>>             || !targetm.calls.promote_prototypes (t1)
>>             || !targetm.calls.promote_prototypes (t2)
>>             || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
> 
> I was thinking
> 
>          && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
>              || type_passed_as (t1) == t1))
> 

Which should be hopefully equivalent, since type_passed_as also compares
size(t1) with sizeof(int), and calls the same hook.

It is unfortunate that I don't see an equivalent function to use in the
C FE.

But they just have to agree on this target dependency.

So in the C front-end I would have to use the target hook as above,
unless Joseph can point out a more appropriate way.

I personally would like to keep the symmetry between C and C++ here,
but if you strongly prefer to use type_passed_as, I can use your
suggestion in the C++ FE instead.


Thanks
Bernd.

Re: [PATCHv2] Add a warning for invalid function casts

[Jason Merrill]

On Thu, Nov 30, 2017 at 1:23 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> On 11/30/17 19:05, Jason Merrill wrote:
>> On Thu, Nov 30, 2017 at 12:55 PM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> On 11/30/17 18:29, Jason Merrill wrote:
>>>> On Thu, Nov 30, 2017 at 11:07 AM, Bernd Edlinger
>>>> <bernd.edlinger@hotmail.de> wrote:
>>>>> On 11/30/17 16:45, Jason Merrill wrote:
>>>>>> On Thu, Nov 30, 2017 at 10:14 AM, Bernd Edlinger
>>>>>> <bernd.edlinger@hotmail.de> wrote:
>>>>>>> On 11/29/17 22:57, Jason Merrill wrote:
>>>>>>>> On 10/09/2017 06:30 PM, Bernd Edlinger wrote:
>>>>>>
>>>>>>>>> +  if (INTEGRAL_TYPE_P (t1)
>>>>>>>>> +      && INTEGRAL_TYPE_P (t2)
>>>>>>>>> +      && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
>>>>>>>>> +      && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
>>>>>>>>> +      || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
>>>>>>>>> +    return true;
>>>>>>>>
>>>>>>>> This section needs a comment explaining what you're allowing and why.
>>>>>>>
>>>>>>> Okay. I will add a comment here:
>>>>>>>
>>>>>>>       /* The signedness of the parameter matters only when an integral
>>>>>>>          type smaller than int is promoted to int, otherwise only the
>>>>>>>          precision of the parameter matters.
>>>>>>>          This check should make sure that the callee does not see
>>>>>>>          undefined values in argument registers.  */
>>>>>>
>>>>>> If we're thinking about argument promotion, should this use
>>>>>> type_passed_as rather than assume promotion to int?
>>>>>
>>>>> I don't know, it is only a heuristic after all, and even if there is no
>>>>> warning for a bogus type cast that does not mean any
>>>>> correctness-guarantee at all.
>>>>>
>>>>> Would type_passed_as make any difference for integral types?
>>>>
>>>> Yes, type_passed_as expresses promotion to int on targets that want
>>>> that behavior.
>>>>
>>>
>>> Hmm, I see, mostly arm, sh and msp430 (whatever that may be).
>>>
>>> So how would you like this:
>>>
>>>     if (INTEGRAL_TYPE_P (t1)
>>>         && INTEGRAL_TYPE_P (t2)
>>>         && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
>>>         && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
>>>             || !targetm.calls.promote_prototypes (t1)
>>>             || !targetm.calls.promote_prototypes (t2)
>>>             || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
>>
>> I was thinking
>>
>>          && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
>>              || type_passed_as (t1) == t1))
>>
>
> Which should be hopefully equivalent, since type_passed_as also compares
> size(t1) with sizeof(int), and calls the same hook.
>
> It is unfortunate that I don't see an equivalent function to use in the
> C FE.
>
> But they just have to agree on this target dependency.
>
> So in the C front-end I would have to use the target hook as above,
> unless Joseph can point out a more appropriate way.
>
> I personally would like to keep the symmetry between C and C++ here,

OK, that makes sense.

Jason

[PATCHv3] Add a warning for invalid function casts

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 348 bytes --]

Hi,

this version of the patch improves the heuristic check to take the
target hook into account, to handle cases correctly when both or only
one parameter is _not_ promoted to int.

Both C and C++ FE should of course use the same logic here.


Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.

[-- Attachment #2: changelog-cast-function-type.txt --]
[-- Type: text/plain, Size: 1245 bytes --]

gcc:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* doc/invoke.texi: Document -Wcast-function-type.
	* recog.h (stored_funcptr): Change signature.
	* tree-dump.c (dump_node): Avoid warning.
	* typed-splay-tree.h (typed_splay_tree): Avoid warning.

libcpp:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* internal.h (maybe_print_line): Change signature.
	
c-family:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c.opt (Wcast-function-type): New warning option.
	* c-lex.c (get_fileinfo): Avoid warning.
	* c-ppoutput.c (scan_translation_unit_directives_only): Remove cast.

c:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-typeck.c (c_safe_arg_type_equiv_p,
	c_safe_function_type_cast_p): New function.
	(build_c_cast): Implement -Wcast-function-type.

cp:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* decl2.c (start_static_storage_duration_function): Avoid warning.
	* typeck.c (cxx_safe_arg_type_equiv_p,
	cxx_safe_function_type_cast_p): New function.
	(build_reinterpret_cast_1): Implement -Wcast-function-type.

testsuite:
2017-10-06  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-c++-common/Wcast-function-type.c: New test.
	* g++.dg/Wcast-function-type.C: New test.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: patch-cast-function-type.diff --]
[-- Type: text/x-patch; name="patch-cast-function-type.diff", Size: 15096 bytes --]

Index: gcc/c/c-typeck.c
===================================================================
--- gcc/c/c-typeck.c	(revision 255270)
+++ gcc/c/c-typeck.c	(working copy)
@@ -5452,6 +5452,62 @@ handle_warn_cast_qual (location_t loc, tree type,
   while (TREE_CODE (in_type) == POINTER_TYPE);
 }
 
+/* Heuristic check if two parameter types can be considered ABI-equivalent.  */
+
+static bool
+c_safe_arg_type_equiv_p (tree t1, tree t2)
+{
+  t1 = TYPE_MAIN_VARIANT (t1);
+  t2 = TYPE_MAIN_VARIANT (t2);
+
+  if (TREE_CODE (t1) == POINTER_TYPE
+      && TREE_CODE (t2) == POINTER_TYPE)
+    return true;
+
+  /* The signedness of the parameter matters only when an integral
+     type smaller than int is promoted to int, otherwise only the
+     precision of the parameter matters.
+     This check should make sure that the callee does not see
+     undefined values in argument registers.  */
+  if (INTEGRAL_TYPE_P (t1)
+      && INTEGRAL_TYPE_P (t2)
+      && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
+      && (targetm.calls.promote_prototypes (t1)
+	  == targetm.calls.promote_prototypes (t2)
+	  || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node))
+      && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
+	  || !targetm.calls.promote_prototypes (t1)
+	  || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
+    return true;
+
+  return comptypes (t1, t2);
+}
+
+/* Check if a type cast between two function types can be considered safe.  */
+
+static bool
+c_safe_function_type_cast_p (tree t1, tree t2)
+{
+  if (TREE_TYPE (t1) == void_type_node &&
+      TYPE_ARG_TYPES (t1) == void_list_node)
+    return true;
+
+  if (TREE_TYPE (t2) == void_type_node &&
+      TYPE_ARG_TYPES (t2) == void_list_node)
+    return true;
+
+  if (!c_safe_arg_type_equiv_p (TREE_TYPE (t1), TREE_TYPE (t2)))
+    return false;
+
+  for (t1 = TYPE_ARG_TYPES (t1), t2 = TYPE_ARG_TYPES (t2);
+       t1 && t2;
+       t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2))
+    if (!c_safe_arg_type_equiv_p (TREE_VALUE (t1), TREE_VALUE (t2)))
+      return false;
+
+  return true;
+}
+
 /* Build an expression representing a cast to type TYPE of expression EXPR.
    LOC is the location of the cast-- typically the open paren of the cast.  */
 
@@ -5645,6 +5701,16 @@ build_c_cast (location_t loc, tree type, tree expr
 	pedwarn (loc, OPT_Wpedantic, "ISO C forbids "
 		 "conversion of object pointer to function pointer type");
 
+      if (TREE_CODE (type) == POINTER_TYPE
+	  && TREE_CODE (otype) == POINTER_TYPE
+	  && TREE_CODE (TREE_TYPE (type)) == FUNCTION_TYPE
+	  && TREE_CODE (TREE_TYPE (otype)) == FUNCTION_TYPE
+	  && !c_safe_function_type_cast_p (TREE_TYPE (type),
+					   TREE_TYPE (otype)))
+	warning_at (loc, OPT_Wcast_function_type,
+		    "cast between incompatible function types"
+		    " from %qT to %qT", otype, type);
+
       ovalue = value;
       value = convert (type, value);
 
Index: gcc/c-family/c-lex.c
===================================================================
--- gcc/c-family/c-lex.c	(revision 255270)
+++ gcc/c-family/c-lex.c	(working copy)
@@ -101,9 +101,11 @@ get_fileinfo (const char *name)
   struct c_fileinfo *fi;
 
   if (!file_info_tree)
-    file_info_tree = splay_tree_new ((splay_tree_compare_fn) strcmp,
+    file_info_tree = splay_tree_new ((splay_tree_compare_fn)
+				     (void (*) (void)) strcmp,
 				     0,
-				     (splay_tree_delete_value_fn) free);
+				     (splay_tree_delete_value_fn)
+				     (void (*) (void)) free);
 
   n = splay_tree_lookup (file_info_tree, (splay_tree_key) name);
   if (n)
Index: gcc/c-family/c-ppoutput.c
===================================================================
--- gcc/c-family/c-ppoutput.c	(revision 255270)
+++ gcc/c-family/c-ppoutput.c	(working copy)
@@ -299,7 +299,7 @@ scan_translation_unit_directives_only (cpp_reader
   struct _cpp_dir_only_callbacks cb;
 
   cb.print_lines = print_lines_directives_only;
-  cb.maybe_print_line = (void (*) (source_location)) maybe_print_line;
+  cb.maybe_print_line = maybe_print_line;
 
   _cpp_preprocess_dir_only (pfile, &cb);
 }
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 255270)
+++ gcc/c-family/c.opt	(working copy)
@@ -384,6 +384,10 @@ Wc++17-compat
 C++ ObjC++ Var(warn_cxx17_compat) Warning LangEnabledBy(C++ ObjC++,Wall)
 Warn about C++ constructs whose meaning differs between ISO C++ 2014 and ISO C++ 2017.
 
+Wcast-function-type
+C ObjC C++ ObjC++ Var(warn_cast_function_type) Warning EnabledBy(Wextra)
+Warn about casts between incompatible function types.
+
 Wcast-qual
 C ObjC C++ ObjC++ Var(warn_cast_qual) Warning
 Warn about casts which discard qualifiers.
Index: gcc/cp/decl2.c
===================================================================
--- gcc/cp/decl2.c	(revision 255270)
+++ gcc/cp/decl2.c	(working copy)
@@ -3525,7 +3525,8 @@ start_static_storage_duration_function (unsigned c
       priority_info_map = splay_tree_new (splay_tree_compare_ints,
 					  /*delete_key_fn=*/0,
 					  /*delete_value_fn=*/
-					  (splay_tree_delete_value_fn) &free);
+					  (splay_tree_delete_value_fn)
+					  (void (*) (void)) free);
 
       /* We always need to generate functions for the
 	 DEFAULT_INIT_PRIORITY so enter it now.  That way when we walk
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 255270)
+++ gcc/cp/typeck.c	(working copy)
@@ -1173,6 +1173,62 @@ comp_template_parms_position (tree t1, tree t2)
   return true;
 }
 
+/* Heuristic check if two parameter types can be considered ABI-equivalent.  */
+
+static bool
+cxx_safe_arg_type_equiv_p (tree t1, tree t2)
+{
+  t1 = TYPE_MAIN_VARIANT (t1);
+  t2 = TYPE_MAIN_VARIANT (t2);
+
+  if (TREE_CODE (t1) == POINTER_TYPE
+      && TREE_CODE (t2) == POINTER_TYPE)
+    return true;
+
+  /* The signedness of the parameter matters only when an integral
+     type smaller than int is promoted to int, otherwise only the
+     precision of the parameter matters.
+     This check should make sure that the callee does not see
+     undefined values in argument registers.  */
+  if (INTEGRAL_TYPE_P (t1)
+      && INTEGRAL_TYPE_P (t2)
+      && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
+      && (targetm.calls.promote_prototypes (t1)
+	  == targetm.calls.promote_prototypes (t2)
+	  || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node))
+      && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
+	  || !targetm.calls.promote_prototypes (t1)
+	  || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
+    return true;
+
+  return same_type_p (t1, t2);
+}
+
+/* Check if a type cast between two function types can be considered safe.  */
+
+static bool
+cxx_safe_function_type_cast_p (tree t1, tree t2)
+{
+  if (TREE_TYPE (t1) == void_type_node &&
+      TYPE_ARG_TYPES (t1) == void_list_node)
+    return true;
+
+  if (TREE_TYPE (t2) == void_type_node &&
+      TYPE_ARG_TYPES (t2) == void_list_node)
+    return true;
+
+  if (!cxx_safe_arg_type_equiv_p (TREE_TYPE (t1), TREE_TYPE (t2)))
+    return false;
+
+  for (t1 = TYPE_ARG_TYPES (t1), t2 = TYPE_ARG_TYPES (t2);
+       t1 && t2;
+       t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2))
+    if (!cxx_safe_arg_type_equiv_p (TREE_VALUE (t1), TREE_VALUE (t2)))
+      return false;
+
+  return true;
+}
+
 /* Subroutine in comptypes.  */
 
 static bool
@@ -7297,9 +7353,27 @@ build_reinterpret_cast_1 (tree type, tree expr, bo
 	   && same_type_p (type, intype))
     /* DR 799 */
     return rvalue (expr);
-  else if ((TYPE_PTRFN_P (type) && TYPE_PTRFN_P (intype))
-	   || (TYPE_PTRMEMFUNC_P (type) && TYPE_PTRMEMFUNC_P (intype)))
-    return build_nop (type, expr);
+  else if (TYPE_PTRFN_P (type) && TYPE_PTRFN_P (intype))
+    {
+      if ((complain & tf_warning)
+	  && !cxx_safe_function_type_cast_p (TREE_TYPE (type),
+					     TREE_TYPE (intype)))
+	warning (OPT_Wcast_function_type,
+		 "cast between incompatible function types"
+		 " from %qH to %qI", intype, type);
+      return build_nop (type, expr);
+    }
+  else if (TYPE_PTRMEMFUNC_P (type) && TYPE_PTRMEMFUNC_P (intype))
+    {
+      if ((complain & tf_warning)
+	  && !cxx_safe_function_type_cast_p
+		(TREE_TYPE (TYPE_PTRMEMFUNC_FN_TYPE_RAW (type)),
+		 TREE_TYPE (TYPE_PTRMEMFUNC_FN_TYPE_RAW (intype))))
+	warning (OPT_Wcast_function_type,
+		 "cast between incompatible pointer to member types"
+		 " from %qH to %qI", intype, type);
+      return build_nop (type, expr);
+    }
   else if ((TYPE_PTRDATAMEM_P (type) && TYPE_PTRDATAMEM_P (intype))
 	   || (TYPE_PTROBV_P (type) && TYPE_PTROBV_P (intype)))
     {
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 255270)
+++ gcc/doc/invoke.texi	(working copy)
@@ -267,7 +267,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wno-builtin-declaration-mismatch @gol
 -Wno-builtin-macro-redefined  -Wc90-c99-compat  -Wc99-c11-compat @gol
 -Wc++-compat  -Wc++11-compat  -Wc++14-compat  @gol
--Wcast-align  -Wcast-align=strict  -Wcast-qual  @gol
+-Wcast-align  -Wcast-align=strict  -Wcast-function-type  -Wcast-qual  @gol
 -Wchar-subscripts  -Wchkp  -Wcatch-value  -Wcatch-value=@var{n} @gol
 -Wclobbered  -Wcomment  -Wconditionally-supported @gol
 -Wconversion  -Wcoverage-mismatch  -Wno-cpp  -Wdangling-else  -Wdate-time @gol
@@ -3904,6 +3904,7 @@ This enables some extra warning flags that are not
 name is still supported, but the newer name is more descriptive.)
 
 @gccoptlist{-Wclobbered  @gol
+-Wcast-function-type  @gol
 -Wempty-body  @gol
 -Wignored-qualifiers @gol
 -Wimplicit-fallthrough=3 @gol
@@ -6033,6 +6034,21 @@ Warn whenever a pointer is cast such that the requ
 target is increased.  For example, warn if a @code{char *} is cast to
 an @code{int *} regardless of the target machine.
 
+@item -Wcast-function-type
+@opindex Wcast-function-type
+@opindex Wno-cast-function-type
+Warn when a function pointer is cast to an incompatible function pointer.
+In a cast involving function types with a variable argument list only
+the types of initial arguments that are provided are considered.
+Any parameter of pointer-type matches any other pointer-type.  Any benign
+differences in integral types are ignored, like @code{int} vs. @code{long}
+on ILP32 targets.  Likewise type qualifiers are ignored.  The function
+type @code{void (*) (void)} is special and matches everything, which can
+be used to suppress this warning.
+In a cast involving pointer to member types this warning warns whenever
+the type cast is changing the pointer to member type.
+This warning is enabled by @option{-Wextra}.
+
 @item -Wwrite-strings
 @opindex Wwrite-strings
 @opindex Wno-write-strings
Index: gcc/recog.h
===================================================================
--- gcc/recog.h	(revision 255270)
+++ gcc/recog.h	(working copy)
@@ -294,7 +294,7 @@ struct insn_gen_fn
   typedef rtx_insn * (*f15) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
   typedef rtx_insn * (*f16) (rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx, rtx);
 
-  typedef f0 stored_funcptr;
+  typedef void (*stored_funcptr) (void);
 
   rtx_insn * operator () (void) const { return ((f0)func) (); }
   rtx_insn * operator () (rtx a0) const { return ((f1)func) (a0); }
Index: gcc/testsuite/c-c++-common/Wcast-function-type.c
===================================================================
--- gcc/testsuite/c-c++-common/Wcast-function-type.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wcast-function-type.c	(working copy)
@@ -0,0 +1,31 @@
+/* { dg-do compile } */
+/* { dg-options "-Wcast-function-type" } */
+
+int f(long);
+
+typedef int (f1)(long);
+typedef int (f2)(void*);
+#ifdef __cplusplus
+typedef int (f3)(...);
+typedef void (f4)(...);
+#else
+typedef int (f3)();
+typedef void (f4)();
+#endif
+typedef void (f5)(void);
+
+f1 *a;
+f2 *b;
+f3 *c;
+f4 *d;
+f5 *e;
+
+void
+foo (void)
+{
+  a = (f1 *) f; /* { dg-bogus   "incompatible function types" } */
+  b = (f2 *) f; /* { dg-warning "incompatible function types" } */
+  c = (f3 *) f; /* { dg-bogus   "incompatible function types" } */
+  d = (f4 *) f; /* { dg-warning "incompatible function types" } */
+  e = (f5 *) f; /* { dg-bogus   "incompatible function types" } */
+}
Index: gcc/testsuite/g++.dg/Wcast-function-type.C
===================================================================
--- gcc/testsuite/g++.dg/Wcast-function-type.C	(revision 0)
+++ gcc/testsuite/g++.dg/Wcast-function-type.C	(working copy)
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-Wcast-function-type" } */
+
+struct S
+{
+  void foo (int*);
+  void bar (int);
+};
+
+typedef void (S::*MF)(int);
+
+void
+foo (void)
+{
+  MF p1 = (MF)&S::foo; /* { dg-warning "pointer to member" } */
+  MF p2 = (MF)&S::bar; /* { dg-bogus   "pointer to member" } */
+}
Index: gcc/tree-dump.c
===================================================================
--- gcc/tree-dump.c	(revision 255270)
+++ gcc/tree-dump.c	(working copy)
@@ -736,7 +736,8 @@ dump_node (const_tree t, dump_flags_t flags, FILE
   di.flags = flags;
   di.node = t;
   di.nodes = splay_tree_new (splay_tree_compare_pointers, 0,
-			     (splay_tree_delete_value_fn) &free);
+			     (splay_tree_delete_value_fn)
+			     (void (*) (void)) free);
 
   /* Queue up the first node.  */
   queue (&di, t, DUMP_NONE);
Index: gcc/typed-splay-tree.h
===================================================================
--- gcc/typed-splay-tree.h	(revision 255270)
+++ gcc/typed-splay-tree.h	(working copy)
@@ -75,9 +75,12 @@ inline typed_splay_tree<KEY_TYPE, VALUE_TYPE>::
 		    delete_key_fn delete_key_fn,
 		    delete_value_fn delete_value_fn)
 {
-  m_inner = splay_tree_new ((splay_tree_compare_fn)compare_fn,
-			    (splay_tree_delete_key_fn)delete_key_fn,
-			    (splay_tree_delete_value_fn)delete_value_fn);
+  m_inner = splay_tree_new ((splay_tree_compare_fn)
+			    (void (*) (void)) compare_fn,
+			    (splay_tree_delete_key_fn)
+			    (void (*) (void)) delete_key_fn,
+			    (splay_tree_delete_value_fn)
+			    (void (*) (void)) delete_value_fn);
 }
 
 /* Destructor for typed_splay_tree <K, V>.  */
Index: libcpp/internal.h
===================================================================
--- libcpp/internal.h	(revision 255270)
+++ libcpp/internal.h	(working copy)
@@ -709,7 +709,7 @@ struct _cpp_dir_only_callbacks
 {
   /* Called to print a block of lines. */
   void (*print_lines) (int, const void *, size_t);
-  void (*maybe_print_line) (source_location);
+  bool (*maybe_print_line) (source_location);
 };
 
 extern void _cpp_preprocess_dir_only (cpp_reader *,

Re: [PATCHv3] Add a warning for invalid function casts

[Jason Merrill]

On Fri, Dec 1, 2017 at 7:42 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> this version of the patch improves the heuristic check to take the
> target hook into account, to handle cases correctly when both or only
> one parameter is _not_ promoted to int.

In looking at this, I discovered that the argument to
promote_prototypes should be the function type, not the parameter
type; the existing uses in the C++ front end were wrong.

Jason

Re: [PATCHv3] Add a warning for invalid function casts

[Bernd Edlinger]

On 12/06/17 23:35, Jason Merrill wrote:
> On Fri, Dec 1, 2017 at 7:42 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> this version of the patch improves the heuristic check to take the
>> target hook into account, to handle cases correctly when both or only
>> one parameter is _not_ promoted to int.
> 
> In looking at this, I discovered that the argument to
> promote_prototypes should be the function type, not the parameter
> type; the existing uses in the C++ front end were wrong.
> 

Bah, sorry.

Yes, it looks like there are at least two more target hooks that change
the type promotion rules later-on: targetm.calls.promote_function_mode
and PROMOTE_MODE.

In the ada FE we pass NULL_TREE to promote_prototypes which seems to
mean if the target wants type promotion in principle.  So it returns
true if some function may promote, and false if no function promotes.
At least this is how the sh-target handles this parameter.
This is BTW the only target that uses the argument of this callback.

So I would think for the purpose of this warning the following check
should be sufficient:

   if (INTEGRAL_TYPE_P (t1)
       && INTEGRAL_TYPE_P (t2)
       && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
       && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
           || !targetm.calls.promote_prototypes (NULL_TREE)
           || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))


What do you think?
Is the patch OK with this change?


Thanks
Bernd.

Re: [PATCHv3] Add a warning for invalid function casts

[Jason Merrill]

On Thu, Dec 7, 2017 at 3:48 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> On 12/06/17 23:35, Jason Merrill wrote:
>> On Fri, Dec 1, 2017 at 7:42 AM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> this version of the patch improves the heuristic check to take the
>>> target hook into account, to handle cases correctly when both or only
>>> one parameter is _not_ promoted to int.
>>
>> In looking at this, I discovered that the argument to
>> promote_prototypes should be the function type, not the parameter
>> type; the existing uses in the C++ front end were wrong.
>>
>
> Bah, sorry.
>
> Yes, it looks like there are at least two more target hooks that change
> the type promotion rules later-on: targetm.calls.promote_function_mode
> and PROMOTE_MODE.
>
> In the ada FE we pass NULL_TREE to promote_prototypes which seems to
> mean if the target wants type promotion in principle.  So it returns
> true if some function may promote, and false if no function promotes.
> At least this is how the sh-target handles this parameter.
> This is BTW the only target that uses the argument of this callback.
>
> So I would think for the purpose of this warning the following check
> should be sufficient:
>
>    if (INTEGRAL_TYPE_P (t1)
>        && INTEGRAL_TYPE_P (t2)
>        && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
>        && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
>            || !targetm.calls.promote_prototypes (NULL_TREE)
>            || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
>
>
> What do you think?
> Is the patch OK with this change?

Yes.

Jason

Re: [PATCHv3] Add a warning for invalid function casts

[Bernd Edlinger]

On 12/07/17 21:48, Bernd Edlinger wrote:
> On 12/06/17 23:35, Jason Merrill wrote:
>> On Fri, Dec 1, 2017 at 7:42 AM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> this version of the patch improves the heuristic check to take the
>>> target hook into account, to handle cases correctly when both or only
>>> one parameter is _not_ promoted to int.
>>
>> In looking at this, I discovered that the argument to
>> promote_prototypes should be the function type, not the parameter
>> type; the existing uses in the C++ front end were wrong.
>>
> 
> Bah, sorry.
> 
> Yes, it looks like there are at least two more target hooks that change
> the type promotion rules later-on: targetm.calls.promote_function_mode
> and PROMOTE_MODE.
> 
> In the ada FE we pass NULL_TREE to promote_prototypes which seems to
> mean if the target wants type promotion in principle.  So it returns
> true if some function may promote, and false if no function promotes.
> At least this is how the sh-target handles this parameter.
> This is BTW the only target that uses the argument of this callback.
> 
> So I would think for the purpose of this warning the following check
> should be sufficient:
> 
>    if (INTEGRAL_TYPE_P (t1)
>        && INTEGRAL_TYPE_P (t2)
>        && TYPE_PRECISION (t1) == TYPE_PRECISION (t2)
>        && (TYPE_UNSIGNED (t1) == TYPE_UNSIGNED (t2)
>            || !targetm.calls.promote_prototypes (NULL_TREE)
>            || TYPE_PRECISION (t1) >= TYPE_PRECISION (integer_type_node)))
> 
> 
> What do you think?
> Is the patch OK with this change?
> 

So, is the simplified heuristic using only promote_prototypes (NULL)
OK?


Thanks
Bernd.