From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 129347 invoked by alias); 9 Oct 2017 17:50:53 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 129332 invoked by uid 89); 9 Oct 2017 17:50:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=HX-OriginatorOrg:outlook.com, HTo:U*msebor, crap, Hauthentication-results:gmail.com X-HELO: EUR01-VE1-obe.outbound.protection.outlook.com Received: from mail-oln040092066024.outbound.protection.outlook.com (HELO EUR01-VE1-obe.outbound.protection.outlook.com) (40.92.66.24) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 09 Oct 2017 17:50:50 +0000 Received: from VE1EUR01FT050.eop-EUR01.prod.protection.outlook.com (10.152.2.52) by VE1EUR01HT082.eop-EUR01.prod.protection.outlook.com (10.152.3.74) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P384) id 15.20.77.10; Mon, 9 Oct 2017 17:50:47 +0000 Received: from AM5PR0701MB2657.eurprd07.prod.outlook.com (10.152.2.58) by VE1EUR01FT050.mail.protection.outlook.com (10.152.3.32) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.20.77.10 via Frontend Transport; Mon, 9 Oct 2017 17:50:47 +0000 Received: from AM5PR0701MB2657.eurprd07.prod.outlook.com ([fe80::8c96:1341:5db1:7f8c]) by AM5PR0701MB2657.eurprd07.prod.outlook.com ([fe80::8c96:1341:5db1:7f8c%18]) with mapi id 15.20.0077.020; Mon, 9 Oct 2017 17:50:47 +0000 From: Bernd Edlinger To: Martin Sebor , Jeff Law , "gcc-patches@gcc.gnu.org" , Joseph Myers , Jason Merrill Subject: Re: [PATCH] Add a warning for invalid function casts Date: Mon, 09 Oct 2017 18:19:00 -0000 Message-ID: References: <1644e85e-6d3a-a114-32b5-a9b49de24407@gmail.com> <72167c17-48f5-d5d6-d9cd-87004e3c1d11@gmail.com> <476f5e91-3768-ef69-331a-ce0c00d38277@hotmail.de> <6f4b4385-52af-6b41-14bf-7e0a06895a8f@hotmail.de> <43dcb212-0c40-f55c-61a0-84a5c149695e@gmail.com> In-Reply-To: <43dcb212-0c40-f55c-61a0-84a5c149695e@gmail.com> authentication-results: gmail.com; dkim=none (message not signed) header.d=none;gmail.com; dmarc=none action=none header.from=hotmail.de; x-incomingtopheadermarker: OriginalChecksum:59917AB40B5B0CCC75A70238BF388C0C82E17663061F3F9AFE46789AF7AC75B3;UpperCasedChecksum:DD2E2A319EB8E089523D883457B2E69325B374DE216CEA8F570A6D1BABA16E3C;SizeAsReceived:7647;Count:46 x-ms-exchange-messagesentrepresentingtype: 1 x-tmn: [qsqbc34Xy6cm8PiHh7OyqNmS9zJGtF8g] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1;VE1EUR01HT082;6:+Av7ooEMQ/+K2AEOcJOrdDDl2sjAIWPdE8JUqZ1RUQRiA9hlCpbqJJMoPN05eyuMGNsqakDzMpeJfcVI5K8ocJnojVfwyRO14LhWCyhCY048ELi3Ivsfj3L8jAuB6wm4IWO0xs24rCzlpYmQr3M3fLjwUiFSn/+xseCJDLKcEHEgQvq011kjZuizr02IULUpUCdRZ4mpn/k/6fWUPg7vabt/94utmBU3QfXNVsxNHoF9tNA8Mn1DxiuaQIOiGM162ifpN0iaFcDklcgVnWDtuldqQIWCqevhWb8ayd5qGUG+PDXQmvDpjQFxYQbo6jrFIr7lltnzxq2Nm5wfIelMKg==;5:lCu2ziFZZZrK+cRayrk78WCVRHi/Ctdm7oMvhx4wEJaC1pJ4j4k0vz2C8YvQqtg8NZw2qOPY5e7meJ+M4OOm1QG0SjxG02NoYRbc7npHDnMrbX1I3/Ydjt1j06cIPpPNvgOHj2kNdTf+fJCzMiSO9A==;24:unWFwt/suAsuxFSMhzgfOXsmdCq+Yk5Ja6PvWDqpL/r0PfabqwfEz8t/B0p6ViLLZRlH8zRQu+isGW5WNLyIreglJiFB+HqrVagCwEn9uys=;7:QreL8kilcCaT2r5Kuur0WBEgEb4KO+/GAOibPSOjAu/gx5xg6LAfpEi2WqHvjf/Wyq8GUJmXmM24Tswd5D4L1PaWadPX8oJhsEDQM13hf5m60dlaE8Hx6Ih47EMs0HPBvR2rm4qOmytw28fgYjP9Anf2XVxaFTTFvRC8ScjLsfLRSl1mmBLlHxiznd5oU/8kaFpD29J5Ugyv7F8k4UWXzlsLDYP6S6NRpMEgOOL/Fh4= x-incomingheadercount: 46 x-eopattributedmessage: 0 x-ms-office365-filtering-correlation-id: 80452b2a-9bf9-40dc-1067-08d50f3e4543 x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(201702061074)(5061506573)(5061507331)(1603103135)(2017031320274)(2017031324274)(2017031323274)(2017031322404)(1603101448)(1601125374)(1701031045);SRVR:VE1EUR01HT082; x-ms-traffictypediagnostic: VE1EUR01HT082: x-exchange-antispam-report-test: UriScan:; x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(444000031);SRVR:VE1EUR01HT082;BCL:0;PCL:0;RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095);SRVR:VE1EUR01HT082; x-forefront-prvs: 045584D28C x-forefront-antispam-report: SFV:NSPM;SFS:(7070007)(98901004);DIR:OUT;SFP:1901;SCL:1;SRVR:VE1EUR01HT082;H:AM5PR0701MB2657.eurprd07.prod.outlook.com;FPR:;SPF:None;LANG:; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="Windows-1252" Content-ID: Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Oct 2017 17:50:47.3924 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Internet X-MS-Exchange-CrossTenant-id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1EUR01HT082 X-SW-Source: 2017-10/txt/msg00517.txt.bz2 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 >> =A0=A0 and signedness as equivalent. >> - stop parameter validation at the first "...". >=20 > These sound quite reasonable to me.=A0 I have a reservation about > just one of them, and some comments about other aspects of the > warning.=A0 Sorry if this seems like a lot.=A0 I'm hoping you'll > find the feedback constructive. >=20 > 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).=A0 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. >=20 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.=A0 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. >=20 > 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. > =A0E.g.: >=20 > =A0 typedef enum E { e } E; > =A0 E f (void); >=20 > =A0 typedef int F (void); >=20 > =A0 F *pf =3D (F *)f;=A0=A0 // -Wcast-function-type >=20 > Is the warning here intended?=A0 (My reading of the documentation > suggests it's not.) >=20 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 >=20 > =A0 struct S { void foo (int*); }; >=20 > =A0 typedef void (S::*MF)(int); > =A0 typedef void F (int*); >=20 > =A0 MF pmf =3D (MF)&S::foo;=A0=A0 // no warning > =A0 F *pf =3D (F*)&S::foo;=A0=A0=A0 // -Wpedantic only >=20 > 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). >=20 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.=A0 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.=A0 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.=A0 Note, linux does not enable -Wextra. >> >> >> Patch was bootstrapped and reg-tested on x86_64-pc-linux-gnu. >> Is it OK for trunk? >=20 > A few comments on the documentation: >=20 > =A0 When one of the function types uses variable arguments like this > =A0 @code{int f(...);}, then only the return value and the parameters > =A0 before the @code{...} are checked, >=20 > 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."=A0=A0 Also, it's the > types of the arguments that are considered (not the value).=A0 With > that I would suggest rewording the sentence along these lines: > > =A0 In a cast involving a function types with a variable argument > =A0 list only the types of initial arguments that are provided are > =A0 considered. >=20 > The sentence >=20 > =A0 Any benign differences in integral types are ignored... >=20 > leaves open the question of what's considered benign.=A0 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. > =A0The 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. >=20 > 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. >=20 Suggested doc changes sound good, I will update the doc accordingly. Thanks Bernd.