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

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

On 10/05/17 18:16, Martin Sebor wrote:
> On 10/03/2017 01:33 PM, Bernd Edlinger wrote:
>>
>> 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.

Thanks for your advice.

I did look into openssl and linux, both have plenty of
-Wcast-function-type warnings.

In the case of openssl it is lots of similar stuff
crypto/aes/aes_cfb.c:25:27: warning: cast between incompatible function 
types from 'void (*)(const unsigned char *, unsigned char *, const
AES_KEY *) {aka void (*)(const unsigned char *, unsigned char *, const 
struct aes_key_st *)}' to 'void (*)(const unsigned char *, unsigned
char *, const void *)' [-Wcast-function-type]
                            (block128_f) AES_encrypt);

The problem is the function is of course called with a different
signature than what is declared.  They take it somehow for granted,
that "void*" or "const void*" parameter are an alias for
any pointer or any const pointer.  Either as parameter or as return
code.

I would believe this is not well-defined by the c-standard.

But it makes the warning less useful because it would be impossible
to spot the few places where the call will actually abort at
runtime.

Then I tried to compile linux, I noticed that there is a new
warning for the alias to incompatible function.

I saw it very often, and it is always when a system call
is defined:

./include/linux/compat.h:50:18: Warnung: »compat_sys_ptrace« alias 
between functions of incompatible types »long int(compat_long_t, 
compat_long_t,  compat_long_t,  compat_long_t) {alias long int(int, 
int,  int,  int)}« and »long int(long int,  long int,  long int,  long 
int)« [-Wattributes]
   asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))\

./include/linux/syscalls.h:211:18: Warnung: »sys_rt_sigprocmask« alias 
between functions of incompatible types »long int(int,  sigset_t *, 
sigset_t *, size_t) {alias long int(int,  struct <anonym> *, struct 
<anonym> *, long unsigned int)}« and »long int(long int,  long int, 
long int,  long int)« [-Wattributes]
   asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \


Needless to say that even more Wcast-function-type warning
happen.

./include/linux/timer.h:178:23: Warnung: cast between incompatible 
function types from »void (*)(struct timer_list *)« to »void (*)(long 
unsigned int)« [-Wcast-function-type]
   __setup_timer(timer, (TIMER_FUNC_TYPE)callback,

So they assume obviously that any int / long / pointer value
are compatible with uintptr_t and intptr_t.

If the Wattribute stays this way, I can garantee that Linus will simply
add -Wno-attribute to the linux makefiles, which is bad because
-Wattribute is more than one warning alone.

At the least we should rename this particular warning into
something else, so that it is not necessary to disable everything
when only this specific warning is too hard to fix.

Maybe it would be good to not warn in type-casts, when they can be
assumed to be safe, for instance
void* <-> any pointer (parameter or result),
uintptr_t <-> any int, any pointer (parameter or result),
void (*) (void) and void (*) (...) <-> any function pointer.

I think that applies to both warnings.

What do you think?


Bernd.