| Message ID | 6f4b4385-52af-6b41-14bf-7e0a06895a8f@hotmail.de |
|---|---|
| State | New |
| Headers | show |
| Series | Add a warning for invalid function casts | expand |
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
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.
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
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. 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. 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 *,
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
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).
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.
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 *).
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.
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.
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
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
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
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
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
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).
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. >
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.
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... 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. >>>
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
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 *,