[PATCH] Fix bogus function cast warning for functions with common arg subset

[PATCH] Fix bogus function cast warning for functions with common arg subset

[Siddhesh Poyarekar]

Libraries like gtk/glib[1] and python[2] use functions with common
argument subsets to register callbacks.  The working idea behind it is
to have a flag in the structure (or some other pre-determined method)
that specifies how the callback is cast and called.

Fix this by not throwing a warning when functions with different
argument list lengths (of M and N where M < N) have compatible
argument types for the first M arguments.

Tested and bootstrapped on x86_64.

Siddhesh

[1] I haven't checked gtk/glib lately, but I remember the G_CALLBACK
    interface does similar things

[2] python has the PyCFunction type member ml_meth in PyMethodDef
    which is designed to accept a PyCFunctionWithKeywords (3 void *
    arguments instead of 2 in PyCFunction) and ml_meth is then cast
    internally to either based on flags in the ml_flags member in
    PyMethodDef.

gcc/c:
	* c-typcheck.c (c_safe_function_type_cast_p): Don't warn on
	unequal number of arguments as long as the common argument
	types match.

gcc/cp:
	* c-typcheck.c (c_safe_function_type_cast_p): Don't warn on
	unequal number of arguments as long as the common argument
	types match.

gcc/testsuite:
	* c-c++-common/Wcast-function-type.c: New test cases.
---
 gcc/c/c-typeck.c                                 |  9 +++++++-
 gcc/cp/typeck.c                                  |  9 +++++++-
 gcc/testsuite/c-c++-common/Wcast-function-type.c | 29 ++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 1eae4ea849c..16887cd3ac4 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -5520,7 +5520,14 @@ c_safe_function_type_cast_p (tree t1, tree t2)
        t1 && t2;
        t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2))
     if (!c_safe_arg_type_equiv_p (TREE_VALUE (t1), TREE_VALUE (t2)))
-      return false;
+      {
+	/* Don't warn on unequal number of arguments as long as the common
+	   argument types match.  This is a common idiom for callbacks.  */
+	if (t1 == void_list_node || t2 == void_list_node)
+	  return true;
+
+	return false;
+      }
 
   return true;
 }
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 0e7c63dd197..f35dca3a05e 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -1221,7 +1221,14 @@ cxx_safe_function_type_cast_p (tree t1, tree t2)
        t1 && t2;
        t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2))
     if (!cxx_safe_arg_type_equiv_p (TREE_VALUE (t1), TREE_VALUE (t2)))
-      return false;
+      {
+	/* Don't warn on unequal number of arguments as long as the common
+	   argument types match.  This is a common idiom for callbacks.  */
+	if (t1 == void_list_node || t2 == void_list_node)
+	  return true;
+
+	return false;
+      }
 
   return true;
 }
diff --git a/gcc/testsuite/c-c++-common/Wcast-function-type.c b/gcc/testsuite/c-c++-common/Wcast-function-type.c
index 81105762ef7..f38ad3fe73d 100644
--- a/gcc/testsuite/c-c++-common/Wcast-function-type.c
+++ b/gcc/testsuite/c-c++-common/Wcast-function-type.c
@@ -2,6 +2,8 @@
 /* { dg-options "-Wcast-function-type" } */
 
 int f(long);
+int g(long, double);
+int h(long, double, double, int);
 
 typedef int (f1)(long);
 typedef int (f2)(void*);
@@ -14,11 +16,26 @@ typedef void (f4)();
 #endif
 typedef void (f5)(void);
 
+typedef int (f6)(long, void *);
+typedef int (f7)(long, double, double);
+typedef int (f8)(long, void *, double);
+
 f1 *a;
 f2 *b;
 f3 *c;
 f4 *d;
 f5 *e;
+f6 *i;
+f7 *j;
+f8 *k;
+
+f6 *l;
+f7 *m;
+f8 *n;
+
+f6 *o;
+f7 *p;
+f8 *q;
 
 void
 foo (void)
@@ -28,4 +45,16 @@ foo (void)
   c = (f3 *) f; /* { dg-bogus   "incompatible function types" } */
   d = (f4 *) f; /* { dg-warning "incompatible function types" } */
   e = (f5 *) f; /* { dg-bogus   "incompatible function types" } */
+
+  i = (f6 *) f; /* { dg-bogus   "incompatible function types" } */
+  j = (f7 *) f; /* { dg-bogus   "incompatible function types" } */
+  k = (f8 *) f; /* { dg-bogus   "incompatible function types" } */
+
+  l = (f6 *) g; /* { dg-warning "incompatible function types" } */
+  m = (f7 *) g; /* { dg-bogus   "incompatible function types" } */
+  n = (f8 *) g; /* { dg-warning "incompatible function types" } */
+
+  o = (f6 *) h; /* { dg-warning "incompatible function types" } */
+  p = (f7 *) h; /* { dg-bogus   "incompatible function types" } */
+  q = (f8 *) h; /* { dg-warning "incompatible function types" } */
 }
-- 
2.14.3

Re: [PATCH] Fix bogus function cast warning for functions with common arg subset

[David Malcolm]

On Fri, 2018-02-23 at 09:10 +0530, Siddhesh Poyarekar wrote:
> Libraries like gtk/glib[1] and python[2] use functions with common
> argument subsets to register callbacks.  The working idea behind it
> is
> to have a flag in the structure (or some other pre-determined method)
> that specifies how the callback is cast and called.
> 
> Fix this by not throwing a warning when functions with different
> argument list lengths (of M and N where M < N) have compatible
> argument types for the first M arguments.

Do we have a PR open for this yet?

I believe this is an example of where this bit (for the Python case):
  https://github.com/imageworks/OpenColorIO/pull/518

[...snip...]


Dave

Re: [PATCH] Fix bogus function cast warning for functions with common arg subset

[Siddhesh Poyarekar]

On Friday 23 February 2018 09:20 PM, David Malcolm wrote:
> Do we have a PR open for this yet?
> 
> I believe this is an example of where this bit (for the Python case):
>   https://github.com/imageworks/OpenColorIO/pull/518

There is now:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84531

Siddhesh

Re: [PATCH] Fix bogus function cast warning for functions with common arg subset

[Richard Biener]

On February 23, 2018 5:12:23 PM GMT+01:00, Siddhesh Poyarekar <siddhesh@sourceware.org> wrote:
>On Friday 23 February 2018 09:20 PM, David Malcolm wrote:
>> Do we have a PR open for this yet?
>> 
>> I believe this is an example of where this bit (for the Python case):
>>   https://github.com/imageworks/OpenColorIO/pull/518
>
>There is now:
>
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84531
>
>Siddhesh

I don't see how the function cast is valid. 

I've argued for void (*) () to/from void (*) (int), etc. In the past and that was shot down similarly. This looks like exactly the same thing. 

Richard. 

Re: [PATCH] Fix bogus function cast warning for functions with common arg subset

[Siddhesh Poyarekar]

On Saturday 24 February 2018 12:01 AM, Richard Biener wrote:
> I don't see how the function cast is valid. 
> 
> I've argued for void (*) () to/from void (*) (int), etc. In the past and that was shot down similarly. This looks like exactly the same thing. 

That should not throw a warning because void (*) (void) is used as a
wildcard to match all functions.  My understanding from the discussions
around the patch implementation was that these are heuristics and are
not meant to catch all cases anyway.  In such a scenario it might be
prudent to avoid breaking behaviour that many programs seem to assume.

Siddhesh

Re: [PATCH] Fix bogus function cast warning for functions with common arg subset

[Martin Sebor]

On 02/23/2018 11:52 AM, Siddhesh Poyarekar wrote:
> On Saturday 24 February 2018 12:01 AM, Richard Biener wrote:
>> I don't see how the function cast is valid.
>>
>> I've argued for void (*) () to/from void (*) (int), etc. In the past and that was shot down similarly. This looks like exactly the same thing.
>
> That should not throw a warning because void (*) (void) is used as a
> wildcard to match all functions.  My understanding from the discussions
> around the patch implementation was that these are heuristics and are
> not meant to catch all cases anyway.  In such a scenario it might be
> prudent to avoid breaking behaviour that many programs seem to assume.

Casting the address of a function that takes one or more arguments
to one that takes fewer is unsafe because when the pointer is used
to call the function the extra arguments have indeterminate values.
(This is also why void(*)(void) as a wildcard was a poor choice:
because it's only safe when it's an exact match.)

Casting in the opposite direction (fewer arguments to more) can
also lead to bugs under ABIs where the callee is responsible for
restoring the frame pointer.

The intent behind the warning is to help find instances of these
conversions that are unsafe and to drive improvements to code and
get it to adopt a single common wildcard.  The current choice
isn't ideal but expanding it even further would compromise
the goal of the warning even more.

Martin

Re: [PATCH] Fix bogus function cast warning for functions with common arg subset

[Siddhesh Poyarekar]

On Saturday 24 February 2018 01:32 AM, Martin Sebor wrote:
> Casting the address of a function that takes one or more arguments
> to one that takes fewer is unsafe because when the pointer is used
> to call the function the extra arguments have indeterminate values.
> (This is also why void(*)(void) as a wildcard was a poor choice:
> because it's only safe when it's an exact match.)
>
> Casting in the opposite direction (fewer arguments to more) can
> also lead to bugs under ABIs where the callee is responsible for
> restoring the frame pointer.

I completely agree about the safety aspect of it, but my argument is
about user experience, not safety.  We are after all talking about
explicit casts, i.e. cast decisions that users have consciously made.

> The intent behind the warning is to help find instances of these
> conversions that are unsafe and to drive improvements to code and
> get it to adopt a single common wildcard.  The current choice
> isn't ideal but expanding it even further would compromise
> the goal of the warning even more.

While varargs may solve a lot of these problems, the best viable
solution or cases where such casts are necessary seems to be to switch
off the warning, which kinda defeats the goal anyway.  IMO we're better
off making the warnings as less intrusive as possible to begin with and
then gradually make them more aggressive.

Siddhesh

Re: [PATCH] Fix bogus function cast warning for functions with common arg subset

[Martin Sebor]

On 02/23/2018 01:32 PM, Siddhesh Poyarekar wrote:
> On Saturday 24 February 2018 01:32 AM, Martin Sebor wrote:
>> Casting the address of a function that takes one or more arguments
>> to one that takes fewer is unsafe because when the pointer is used
>> to call the function the extra arguments have indeterminate values.
>> (This is also why void(*)(void) as a wildcard was a poor choice:
>> because it's only safe when it's an exact match.)
>>
>> Casting in the opposite direction (fewer arguments to more) can
>> also lead to bugs under ABIs where the callee is responsible for
>> restoring the frame pointer.
>
> I completely agree about the safety aspect of it, but my argument is
> about user experience, not safety.  We are after all talking about
> explicit casts, i.e. cast decisions that users have consciously made.

I agree.  That was also my first comment on the feature when
it was first proposed:
https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00275.html

>> The intent behind the warning is to help find instances of these
>> conversions that are unsafe and to drive improvements to code and
>> get it to adopt a single common wildcard.  The current choice
>> isn't ideal but expanding it even further would compromise
>> the goal of the warning even more.
>
> While varargs may solve a lot of these problems, the best viable
> solution or cases where such casts are necessary seems to be to switch
> off the warning, which kinda defeats the goal anyway.  IMO we're better
> off making the warnings as less intrusive as possible to begin with and
> then gradually make them more aggressive.

In my mind that would be a perfectly reasonable approach.
A variation on it might be to leave a new warning disabled
in the first release, then include it in -Wextra the next
release, and finally put it in -Wall.

Unfortunately, in reality this rarely happens.  Most warnings
stay wherever they land when they're first added and only few
are ever tightened up.  Most also stay the same for many
releases.  (IME, it's not a fun or glamorous job to do the
work it takes to turn on a disabled warning, or to tighten
up an existing one and deal with the fallout.)

Martin

Re: [PATCH] Fix bogus function cast warning for functions with common arg subset

[Siddhesh Poyarekar]

On Saturday 24 February 2018 03:28 AM, Martin Sebor wrote:
> In my mind that would be a perfectly reasonable approach.
> A variation on it might be to leave a new warning disabled
> in the first release, then include it in -Wextra the next
> release, and finally put it in -Wall.
> 
> Unfortunately, in reality this rarely happens.  Most warnings
> stay wherever they land when they're first added and only few
> are ever tightened up.  Most also stay the same for many
> releases.  (IME, it's not a fun or glamorous job to do the
> work it takes to turn on a disabled warning, or to tighten
> up an existing one and deal with the fallout.)

Yeah, I've thought about this a bit over the weekend and I've come to
the conclusion that silencing the warning will only hold us back
indefinitely.  I'll try to fix this in the modules I'm interested in.  I
withdraw this patch for now.

Thanks,
Siddhesh