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.
gcc:
2017-10-06 Bernd Edlinger <bernd.edlin...@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.edlin...@hotmail.de>
* internal.h (maybe_print_line): Change signature.
c-family:
2017-10-06 Bernd Edlinger <bernd.edlin...@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.edlin...@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.edlin...@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.edlin...@hotmail.de>
* c-c++-common/Wcast-function-type.c: New test.
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 *,
