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.
> 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.)
>
Aehm no, that is unintentional, thanks for testing...
It looks like the warning does not trigger with
typedef unsigned int F (void);
But this enum should promote to int, likewise for _Bool,
So I think this should be fixable.
> 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).
>
Yes I agree.
I think all pointer-to member function casts where the types
are different should warn, although I don't have seen any of that
crap so far.
>> 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.
I am not sure if the sign is relevant for int/unsigned int.
What I heard from the linux people is, the callee expects the
inputs to be correctly promoted according to the type of the
parameter, the example was an unsigned char which is used
as an array index, but the actual register may be negative
for instance, which results in undefined behavior.
So I am not sure if that applies only to values with
precision smaller than int, or also for int64_t
i.e. if the target has 64bit registers.
>
> 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.
>
Suggested doc changes sound good, I will update the doc accordingly.
Thanks
Bernd.
