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

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

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: changelog-cast-function-type.txt
Description: changelog-cast-function-type.txt

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 *,