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

[Martin Sebor <msebor at gmail dot com>]

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

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

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

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

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

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

Likewise by:

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

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

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

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

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

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

Martin

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