From: Eric Gallager <egall@gwmail.gwu.edu>
To: Martin Sebor <msebor@gmail.com>
Cc: Bernd Edlinger <bernd.edlinger@hotmail.de>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>
Subject: Re: [RFA] [PATCH] Add a warning for invalid function casts
Date: Sun, 08 Oct 2017 16:47:00 -0000 [thread overview]
Message-ID: <CAMfHzOs4KAs61XrfZm8CczpSTUSn+S-oVXbuMHBXsPMkgE=eVw@mail.gmail.com> (raw)
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.
next reply other threads:[~2017-10-08 16:38 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-08 16:47 Eric Gallager [this message]
-- strict thread matches above, loose matches on Subject: below --
2017-10-08 18:05 Eric Gallager
2017-10-03 19:33 Bernd Edlinger
2017-10-03 21:34 ` Joseph Myers
2017-10-05 14:03 ` Bernd Edlinger
2017-10-05 14:10 ` Joseph Myers
2017-10-05 0:25 ` Eric Gallager
2017-10-05 13:39 ` Bernd Edlinger
2017-10-05 14:06 ` Andreas Schwab
2017-10-05 18:22 ` Eric Gallager
2017-10-05 16:16 ` Martin Sebor
2017-10-05 21:04 ` Bernd Edlinger
2017-10-05 21:47 ` Joseph Myers
2017-10-06 20:50 ` Jeff Law
2017-10-05 22:17 ` Martin Sebor
2017-10-06 13:26 ` Bernd Edlinger
2017-10-06 15:43 ` Martin Sebor
2017-10-06 18:25 ` Bernd Edlinger
2017-10-06 20:43 ` Martin Sebor
2017-10-06 21:01 ` Jeff Law
2017-10-06 22:23 ` Bernd Edlinger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMfHzOs4KAs61XrfZm8CczpSTUSn+S-oVXbuMHBXsPMkgE=eVw@mail.gmail.com' \
--to=egall@gwmail.gwu.edu \
--cc=bernd.edlinger@hotmail.de \
--cc=gcc-patches@gcc.gnu.org \
--cc=msebor@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).