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

[Eric Gallager <egall at gwmail dot gwu dot edu>]

On Fri, Oct 6, 2017 at 4:37 PM, Martin Sebor <msebor@gmail.com> wrote:
> On 10/06/2017 12:06 PM, Bernd Edlinger wrote:
>>
>> On 10/06/17 17:43, Martin Sebor wrote:
>>>
>>> On 10/06/2017 07:25 AM, Bernd Edlinger wrote:
>>>>
>>>> On 10/05/17 18:16, Martin Sebor wrote:
>>>>>
>>>>> 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.
>>>>>
>>>>
>>>> Well, while the first example should be safe,
>>>> the second one is probably not safe:
>>>>
>>>> Because the signed and unsigned char are promoted to int,
>>>> by the ABI but one is in the range -128..127 while the
>>>> other is in the range 0..255, right?
>>>
>>>
>>> Right.  The cast is always safe but whether or not a call to such
>>> a function through the incompatible pointer is also safe depends
>>> on the definition of the function (and on the caller).  If the
>>> function uses just the low bits of the argument then it's most
>>> likely fine for any argument.  If the caller only calls it with
>>> values in the 7-bit range (e.g., the ASCII subset) then it's also
>>> fine.  Otherwise there's the potential for the problem you pointed
>>> out.  (Similarly, if in the first example I gave the cast added
>>> constness to the argument rather than removing it and the function
>>> modified the pointed-to object calling it through the incompatible
>>> pointer on a constant object would also be unsafe.)
>>>
>>> Another class of cases to consider are casts between functions
>>> taking pointers to different but related structs.  Code like this
>>> could be written to mimic C++ calling a base class function on
>>> a derived object.
>>>
>>>    struct Base { ... };
>>>    struct Derived { Base b; ... };
>>>
>>>    typedef void F (Derived*);
>>>
>>>    void foo (Base*);
>>>
>>>    F* pf = (F*)foo;
>>>
>>
>> Hmm, yes.
>>
>> I start to believe, that this warning should treat all pointers
>> as equivalent, but everything else need to be of the same type.
>> That would at least cover the majority of all "safe" use cases.
>
>
> Perhaps basing the warning on some form of structural equivalence
> between function arguments might be useful.  For instance, in
> ILP32, casting between 'int foo (int)' and 'long foo (long)' and
> calling the function is probably generally considered safe (even
> though it's undefined by the language) and works as people expect
> so avoiding the warning there would help keep the false positive
> rate down. (Something like this might also work for the kernel
> alias macros.)
>
> Similarly, casting between a function that returns a scalar smaller
> than int and int and then calling it is probably safe (or maybe
> even long, depending on the ABI).
>
> Casting a function returning a value to one returning void and
> calling it through the result should always be safe.
>
> I would also suggest to consider disregarding qualifiers as those
> are often among the reasons for intentional casts (e.g., when
> mixing a legacy API and a more modern const-correct one).
>
> Casts are also not uncommon between variadic and ordinary function
> types so some heuristic might be appropriate there.
>
>>
>> And I need a way to by-pass the warning with a generic function
>> pointer type.  uintptr_t is not the right choice, as you pointed
>> out already.
>>
>> But I also see problems with "int (*) ()" as a escape mechanism
>> because this declaration creates a warning in C with
>> -Wstrict-prototypes, and in C++ this syntax means "int (*) (void)"
>> while the C++ type "int (*) (...)" is rejected by the C front end.
>
>
> I wouldn't consider it a problem if the suppression mechanism were
> different between languages.  Most such casts are going to be in
> source files (as opposed to C headers) so it should be fine to use
> each language's unique form of function without a prototype.
>
> Martin

Some codebases attempt to be compilable as both C and C++, whether it
be by using -Wc++-compat, or having options to compile with either the
C compiler or C++ compiler. In such cases, I'd prefer to keep the
amounts of "#ifdef __cplusplus" required to a minimum; a suppression
mechanic that works the same between languages would be much
preferred.