From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20371 invoked by alias); 28 Sep 2017 14:26:02 -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 20362 invoked by uid 89); 28 Sep 2017 14:26:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy= X-HELO: mail-io0-f177.google.com Received: from mail-io0-f177.google.com (HELO mail-io0-f177.google.com) (209.85.223.177) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 28 Sep 2017 14:25:59 +0000 Received: by mail-io0-f177.google.com with SMTP id v36so1713502ioi.1 for ; Thu, 28 Sep 2017 07:25:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:subject:to:references:from:message-id :date:user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=+M0ER4Zw5a0cGGVa9Gc2hExGAeUSm1A4XC1Iat918j8=; b=XJ1eGj9WvoDf2qW6ayzBqjN1vWDYtIv00vlsaEZku70+2AcziEgI3ubSHrFlASNcqb Rz8e8QT7yeg/VFExQMKubRM9GG5vzY1o+F1RJppdnMkWpt/daiRsr0GJU4IxnSvjJc8m hYWThjK8r8BJvJ3XYourtuu/JI3l28kedBIdZUR92Wygj9MV2GlZs58tN3Mp9cSLu9yl DEa3f0zhJ7q34Cdd1mxuZgqBl1NMcsLltUXDIJQAiMRunUutgvsTgW357qDKGUhnrA34 Ol+YMOpVpVIyHGOjzZamo2CI4ImxtetcUDRxtjCHIlHVmxUZCuN8uan3VhhTD+n/GpBb qMdQ== X-Gm-Message-State: AMCzsaXr2KLnsciAlgqlvaTFdE74mQqCbQiw1iXehrI5TuNeo3P/ph1w QMscLpyv2LBwwBi4wwJINgM= X-Google-Smtp-Source: AOwi7QAb5f6Rc1l0r7W/iZbDWv8NsOaDqOdB//wdVyJgx0cG2Ai5sZ+2JLwLvwyHbNKG7rLjUAEOSw== X-Received: by 10.107.181.196 with SMTP id e187mr7469859iof.178.1506608757272; Thu, 28 Sep 2017 07:25:57 -0700 (PDT) Received: from [10.196.112.115] ([216.133.211.222]) by smtp.googlemail.com with ESMTPSA id k76sm712625ita.4.2017.09.28.07.25.56 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 28 Sep 2017 07:25:56 -0700 (PDT) Subject: Re: correct attribute ifunc C++ type safety (PR 82301) To: Martin Sebor , Gcc Patch List References: <42ebe275-53db-35c9-4199-85b7423cd711@gmail.com> From: Nathan Sidwell Message-ID: Date: Thu, 28 Sep 2017 14:26:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <42ebe275-53db-35c9-4199-85b7423cd711@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-SW-Source: 2017-09/txt/msg01883.txt.bz2 On 09/24/2017 06:03 PM, Martin Sebor wrote: > r253041 enhanced type checking for alias and ifunc attributes to > detect declarations of incompatible aliases, or ifunc resolvers > that return pointers to functions of an incompatible type.  More > extensive testing exposed a bug in the implementation of the ifunc > attribute handling in C++ where the checker expected the ifunc > resolver to return a pointer to a member function when the > implementation actually expects it return a pointer to a non- > member function. > > In a discussion of the test suite failures, Jakub also suggested > to break the enhanced warning out of -Wattributes and issue it > under a different option. > > The attached patch corrects the C++ problem and moves the warning > under -Wincompatible-pointer-types.  Since this is a C-only option, > the patch also enables for it C++.  Since the option is enabled by > default, the patch further requires -Wextra to issue the warning > for ifunc resolvers returning void*.  However, the patched checker > diagnoses other incompatibilities without it. > > Martin I find the maybe_diag_incompatible_alias function confusing. > +/* Check declaration of the type of ALIAS for compatibility with its TARGET > + (which may be an ifunc resolver) and issue a diagnostic when they are > + not compatible according to language rules (plus a C++ extension for > + non-static member functions). */ > + > +static void > +maybe_diag_incompatible_alias (tree alias, tree target) > +{ > + tree altype = TREE_TYPE (alias); > + tree targtype = TREE_TYPE (target); > + > + bool ifunc = lookup_attribute ("ifunc", DECL_ATTRIBUTES (alias)); > + if (ifunc) I think it might be clearer if this was broken out into a diag_ifunc function? But see below ... > + { > + /* Handle attribute ifunc first. */ > + > + tree funcptr = altype; > + > + /* Set FUNCPTR to the type of the alias target. If the type > + is a non-static member function of class C, construct a type > + of an ordinary function taking C* as the first argument, > + followed by the member function argument list, and use it > + instead to check for compatibilties. FUNCPTR is used only > + in diagnostics. */ This comment is self-contradictory. 1 Set FUNCPTR 2 Do some method-type shenanigans 3 Use it to check for incompatibilites 4 FUNCPTR is only used in diags Which of #3 and #4 is true? > + > + if (TREE_CODE (altype) == METHOD_TYPE) > + { IMHO put the description of the METHOD_TYPE chicanery inside the block doing it? FWIW, although the change being made works on many (most?) ABIs, it's not formally correct and I think fails on some where 'this' is passed specially. You might want to note that? > + tree rettype = TREE_TYPE (altype); > + tree args = TYPE_ARG_TYPES (altype); > + altype = build_function_type (rettype, args); > + funcptr = altype; > + } > + > + if ((!FUNC_OR_METHOD_TYPE_P (targtype) > + || (prototype_p (altype) > + && prototype_p (targtype) > + && !types_compatible_p (altype, targtype)))) > + { > + funcptr = build_pointer_type (funcptr); > + > + if (warning_at (DECL_SOURCE_LOCATION (target), > + OPT_Wincompatible_pointer_types, > + "% resolver for %qD should return %qT", > + alias, funcptr)) > + inform (DECL_SOURCE_LOCATION (alias), > + "resolver indirect function declared here"); > + } this block is almost the same as the non-ifunc block. Surely they can be the same code? (by generalizing one of the cases until it turns into the other?) > + /* Deal with static member function pointers. */ I do not understand this comment or condition. We seem to have dealt with pointers already and the conditions seem confused. > + if (TREE_CODE (targtype) != RECORD_TYPE > + || TYPE_FIELDS (targtype) > + || TREE_CODE (TREE_TYPE (TYPE_FIELDS (targtype))) != POINTER_TYPE > + || (TREE_CODE (TREE_TYPE (TREE_TYPE (TYPE_FIELDS (targtype)))) > + != METHOD_TYPE)) if not a record, or has TYPE_FIELDS non-NULL or the first field doesn't have pointer type (we can't get here) or something else about the first field oh, I think it's trying to spot the pointer to NON-static member function internal record type. But brokenly. I think pmf record_types have DECL_ARTIFICIAL and BUILTIN_LOCATION, that might be useful. > + { > + funcptr = build_pointer_type (funcptr); > + > + error ("% resolver for %qD must return %qT", > + alias, funcptr); > + > + inform (DECL_SOURCE_LOCATION (alias), > + "resolver indirect function declared here"); > + } > + } > + > + if ((!FUNC_OR_METHOD_TYPE_P (targtype) > + || (prototype_p (altype) > + && prototype_p (targtype) > + && !types_compatible_p (altype, targtype)))) Similar to above, already noted. Is this function called before we know whether we've enabled the appropriate warnings? It would be better to bail out sooner if the warnings are disabled. nathan -- Nathan Sidwell