This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Add a warning for invalid function casts
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