From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 86919 invoked by alias); 9 Oct 2017 16:44:20 -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 86561 invoked by uid 89); 9 Oct 2017 16:44:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy=Depending X-HELO: mail-qk0-f170.google.com Received: from mail-qk0-f170.google.com (HELO mail-qk0-f170.google.com) (209.85.220.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 09 Oct 2017 16:44:18 +0000 Received: by mail-qk0-f170.google.com with SMTP id s14so10886679qks.6 for ; Mon, 09 Oct 2017 09:44:17 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:subject:to:references:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=2hBfLLFRXpidEFrHwPy/K+SGYvIh44hiZDYU/5Ie130=; b=ec7oyOAHpucNPoDmXWKeEoRSGfRBSjI9qVhfeAgAxHvpJwY+vjep+YVT10BzbNWjuR wapDhyweJuGWJel1jSdvmHEzItTg+NwjlSGRiGF4VyWJTujVl+DwguK3e7cdfxS+XANk psmr790mA1iywYEa9OtOdlpbvUNij4o4tTfx1+7KC3eioicvrMdT2aVl2gqQTo04IvBD bJj0hGNwdHxB3jgmU/miH/v+cyu3V3tTyTYPeR6uWvwvVzYm8LmmnaQbsfMqfADYscR1 d6ID09BlQsKuQOgEI61c+v7aw2krzCidLo8/Ib7dt2AVSPxuEbWb8NixJwVs83tDv8V4 RwOg== X-Gm-Message-State: AMCzsaVWKnSyPXR05AnjMxUZJR3ECkRBfk83Dlg84qVfX4nE+9OlWU2h ybpMt89zXyoHPH6MxMBTMKQ= X-Google-Smtp-Source: AOwi7QAiZeiJvt4jWHXvf4BBSCeMa+e/y2ELhKUJff3tB9QlT+sO+b4lbI9jsyFzokVT4EvhI1Zh+A== X-Received: by 10.55.21.153 with SMTP id 25mr9476322qkv.6.1507567456109; Mon, 09 Oct 2017 09:44:16 -0700 (PDT) Received: from localhost.localdomain (71-218-5-6.hlrn.qwest.net. [71.218.5.6]) by smtp.gmail.com with ESMTPSA id e7sm5016165qkf.73.2017.10.09.09.44.15 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 09 Oct 2017 09:44:15 -0700 (PDT) From: Martin Sebor Subject: Re: [PATCH] Add a warning for invalid function casts To: Bernd Edlinger , Jeff Law , "gcc-patches@gcc.gnu.org" , Joseph Myers , Jason Merrill 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> Message-ID: <43dcb212-0c40-f55c-61a0-84a5c149695e@gmail.com> Date: Mon, 09 Oct 2017 16:48:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <6f4b4385-52af-6b41-14bf-7e0a06895a8f@hotmail.de> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg00510.txt.bz2 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. 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.) 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). > 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. 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. Martin