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.
Instead, I think the warning should be suppressed for casts to
function types without a prototype. Calling those is (or should
for the most part be) safe and they are the natural way to express
that we don't care about type safety.
Let me also clarify that I understand bullet 4 correctly.
In my tests the warning triggers on enum/integral/boolean
incompatibilities that the text above suggests should be accepted.
E.g.:
typedef enum E { e } E;
E f (void);
typedef int F (void);
F *pf = (F *)f; // -Wcast-function-type
Is the warning here intended? (My reading of the documentation
suggests it's not.)
In testing the warning in C++, I noticed it's not issued for
casts between incompatible pointers to member functions, or for
casts between member functions to ordinary functions (those are
diagnosed with -Wpedantic
struct S { void foo (int*); };
typedef void (S::*MF)(int);
typedef void F (int*);
MF pmf = (MF)&S::foo; // no warning
F *pf = (F*)&S::foo; // -Wpedantic only
These both look like they would be worth diagnosing under the new
option (the second one looks like an opportunity to provide
a dedicated option to control the existing pedantic warning).
I cannot convince myself to handle uintptr_t and pointers as
equivalent. It is possible that targets like m68k have
an ABI where uintptr_t and void* are different as return values
but are identical as parameter values.
IMHO adding an exception for uintptr_t vs. pointer as parameters
could easily prevent detection of real bugs. Even if it is safe
for all targets.
However it happens: Examples are the libiberty splay-tree functions,
and also one single place in linux, where an argument of type
"long int" is used to call a timer function with a pointer
argument. Note, linux does not enable -Wextra.
Patch was bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?
A few comments on the documentation:
When one of the function types uses variable arguments like this
@code{int f(...);}, then only the return value and the parameters
before the @code{...} are checked,
Although G++ accepts 'int f(...)' since GCC does not I would suggest
to avoid showing the prototype in the descriptive text and instead
refer to "functions taking variable arguments." Also, it's the
types of the arguments that are considered (not the value). With
that I would suggest rewording the sentence along these lines:
In a cast involving a function types with a variable argument
list only the types of initial arguments that are provided are
considered.
The sentence
Any benign differences in integral types are ignored...
leaves open the question of what's considered benign. In my view,
if pointer types are ignored (which is reasonable as we discussed
but which can lead to serious problems) it seems that that
signed/unsigned differences should also be considered benign.
The consequences of those differences seem considerably less
dangerous than calling a function that takes an char* with an int*
argument.
Regarding qualifiers, unless some types qualifiers are not ignored
(e.g., _Atomic and restrict) I would suggest to correct the sentence
that mentions const and volatile to simply refer to "type qualifiers"
instead to make it clear that no qualifiers are considered.
Martin
