[PING**2] [PATCH] Add a warning for invalid function casts

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

Ping...

On 11/08/17 17:55, Bernd Edlinger wrote:
> Ping...
> 
> for the C++ part of this patch:
> 
> https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00559.html
> 
> 
> Thanks
> Bernd.
> 
>> On 10/10/17 00:30, 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.
>>>
>>> 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, ...);
>>>
>>> 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.
>>>
>>> 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 also changed the heuristic, so that your example with the enum should
>>> now work.  I did not add it to the test case, because it would
>>> break with -fshort-enums :(
>>>
>>> Attached I have an updated patch that extends this warning to the
>>> pointer-to-member function cast, and relaxes the heuristic on the
>>> benign integral type differences a bit further.
>>>
>>>
>>> Is it OK for trunk after bootstrap and reg-testing?
>>>
>>>
>>> Thanks
>>> Bernd.
>>>