This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 10/05/2017 03:04 PM, Bernd Edlinger wrote:
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?

The attribute alias/ifunc incompatibilities are conflicts between
declarations of the same symbol.  Such conflicts are invalid in C
and ordinarily rejected with a hard error.  With the exception of
ifunc resolvers returning void*, I see the alias/ifunc conflicts
as signs of bugs or, at best, benign mistakes.  I also can think
of no reason why they shouldn't in most cases be corrected to
conform to the C requirements.  Where they can't be fixed it's
usually easy to suppress the warnings by declaring the functions
without a prototype.  That also makes it clear to the reader that
type checking has been suppressed.

On the other hand, explicit casts between incompatible pointers
are a well-defined feature of the language and their use a clear
indication that the programmer was aware of the incompatibility
and intentionally suppressed it.  There no doubt are bugs among
these that would be helpful to find so I think the challenge
here is to distinguish the intentional and benign conversions
from the others while minimizing false positives.  Since there
are likely to be a large proportion of the intentional and benign
cases, unless the heuristic to find the others is very good, there
should also be an easy way to suppress the false positives without
compromising readability.

Martin

PS We are discussing the kernel warnings in bug 82435.  I've
already moved them under -Wincompatible-pointer-types but it
sounds like a new option altogether might be preferable.
Ideally, though, if it's possible it would be best to fix them
like we did for Glibc so the kernel too would benefit from the
better type checking.  I should look into that.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]