From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1188 invoked by alias); 7 Nov 2018 21:59:39 -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 1161 invoked by uid 89); 7 Nov 2018 21:59:38 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 07 Nov 2018 21:59:37 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DB1DC3082B4C; Wed, 7 Nov 2018 21:59:35 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-54.rdu2.redhat.com [10.10.112.54]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4AE8918E3D; Wed, 7 Nov 2018 21:59:34 +0000 (UTC) Subject: Re: [PATCH] detect attribute mismatches in alias declarations (PR 81824) To: Martin Sebor , Joseph Myers Cc: Jason Merrill , Gcc Patch List References: <42f8ec31-b967-08e7-ff7a-67a63a1a2a64@gmail.com> <21833fc0-d4ca-d239-7194-49fe90a2f6d4@gmail.com> From: Jeff Law Openpgp: preference=signencrypt Message-ID: <833ca271-6675-0748-800f-13d6295bb5ea@redhat.com> Date: Wed, 07 Nov 2018 21:59:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <21833fc0-d4ca-d239-7194-49fe90a2f6d4@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2018-11/txt/msg00491.txt.bz2 On 10/23/18 7:50 PM, Martin Sebor wrote: > On 10/23/2018 03:53 PM, Joseph Myers wrote: >> On Mon, 22 Oct 2018, Martin Sebor wrote: >> >>> between aliases and ifunc resolvers.  With -Wattribute-alias=1 >>> that reduced the number of unique instances of the warnings for >>> a Glibc build to just 27.  Of those, all but one of >>> the -Wattributes instances are of the form: >>> >>>   warning: ‘leaf’ attribute has no effect on unit local functions >> >> What do the macro expansions look like there?  All the places where you're >> >> adding "copy" attributes are for extern declarations, not static ones, >> whereas your list of warnings seems to indicate this is appearing for >> ifunc resolvers (which are static, but should not be copying attributes >> from anywhere). > > These must have been caused by the bug in the patch (below). > They have cleared up with it fixed.  I'm down to just 18 > instances of a -Wmissing-attributes warning, all for string > functions.  The cause of those is described below. > >> >>> All the -Wmissing-attributes instances are due to a missing >>> nonnull attribute on the __EI__ kinds of functions, like: >>> >>>   warning: ‘__EI_vfprintf’ specifies less restrictive attribute than its >>> target ‘vfprintf’: ‘nonnull’ >> >> That looks like a bug in the GCC patch to me; you appear to be adding copy >> >> attributes in the correct place.  Note that __EI_* gets declared twice >> (first with __asm__, second with an alias attribute), so anything related >> to handling of such duplicate declarations might be a cause for such a >> bug (and an indication of what you need to add a test for when fixing such >> >> a bug). > > There was a bug in the patch, but there is also an issue in Glibc > that made it tricky to see the problem. > > The tests I had in place were too simple to catch the GCC bug: > the problem there was that when the decl didn't have an attribute > the type of the "template" did the check would fail without also > considering the decl's type.  Tricky stuff!  I've added tests to > exercise this. > > The Glibc issue has to do with the use of __hidden_ver1 macro > to declare string functions.  sysdeps/x86_64/multiarch/strcmp.c > for instance has: > >   __hidden_ver1 (strcmp, __GI_strcmp, __redirect_strcmp) >     __attribute__ ((visibility ("hidden"))); > > and __redirect_strcmp is missing the nonnull attribute because > it's #undefined in include/sys/cdefs.h.  An example of one of > these warnings is attached. > > Using strcmp instead of __redirect_strcmp would solve this but > __redirect_strcmp should have all the same attributes as strcmp. > But nonnull is removed from the declaration because the __nonnull > macro that controls it is undefined in include/sys/cdefs.h.  There > is a comment above the #undef in the header that reads: > > /* The compiler will optimize based on the knowledge the parameter is >    not NULL.  This will omit tests.  A robust implementation cannot allow >    this so when compiling glibc itself we ignore this attribute.  */ > # undef __nonnull > # define __nonnull(params) > > I don't think this is actually true for recent versions of GCC. > The nonnull optimization is controlled by > -fisolate-erroneous-paths-attribute and according to the manual > and common.opt the option is disabled by default. > > But if you do want to avoid the attribute on declarations of > these functions regardless it should be safe to add it after > the declaration in the .c file, like so: > > __hidden_ver1 (strcmp, __GI_strcmp, __redirect_strcmp) >   __attribute__ ((visibility ("hidden"), copy (strcmp))); > > That should make it straightforward to adopt the enhancement > and experiment with -Wattribute-alias=2 to see if it does what > you had  in mind. > > The latest GCC patch with the fix mentioned above is attached. > > Martin > > gcc-81824.diff > > PR middle-end/81824 - Warn for missing attributes with function aliases > > gcc/c-family/ChangeLog: > > PR middle-end/81824 > * c-attribs.c (handle_copy_attribute_impl): New function. > (handle_copy_attribute): Same. > > gcc/cp/ChangeLog: > > PR middle-end/81824 > * pt.c (warn_spec_missing_attributes): Move code to attribs.c. > Call decls_mismatched_attributes. > > gcc/ChangeLog: > > PR middle-end/81824 > * attribs.c (has_attribute): New helper function. > (decls_mismatched_attributes, maybe_diag_alias_attributes): Same. > * attribs.h (decls_mismatched_attributes): Declare. > * cgraphunit.c (handle_alias_pairs): Call maybe_diag_alias_attributes. > (maybe_diag_incompatible_alias): Use OPT_Wattribute_alias_. > * common.opt (-Wattribute-alias): Take an argument. > (-Wno-attribute-alias): New option. > * doc/extend.texi (Common Function Attributes): Document copy. > (Common Variable Attributes): Same. > * doc/invoke.texi (-Wmissing-attributes): Document enhancement. > (-Wattribute-alias): Document new option argument. > > libgomp/ChangeLog: > > PR c/81824 > * libgomp.h (strong_alias, ialias, ialias_redirect): Use attribute > copy. > > gcc/testsuite/ChangeLog: > > PR middle-end/81824 > * gcc.dg/Wattribute-alias.c: New test. > * gcc.dg/Wmissing-attributes.c: New test. > * gcc.dg/attr-copy.c: New test. > * gcc.dg/attr-copy-2.c: New test. > * gcc.dg/attr-copy-3.c: New test. > * gcc.dg/attr-copy-4.c: New test. > [ snip ] > +} > + > +/* Handle the "copy" attribute by copying the set of attributes > + from the symbol referenced by ARGS to the declaration of *NODE. */ > + > +static tree > +handle_copy_attribute (tree *node, tree name, tree args, > + int flags, bool *no_add_attrs) > +{ > + /* Break cycles in circular references. */ > + static hash_set attr_copy_visited; Does this really need to be static? > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi > index cfe6a8e..8ffb0cd 100644 > --- a/gcc/doc/extend.texi > +++ b/gcc/doc/extend.texi > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index 5c95f67..c027acd 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi [ ... ] > + > +In C++, the warning is issued when an explicitcspecialization of a primary "explicitcspecialization" ? :-) Looks pretty good. There's the explicit specialization nit and the static vs auto question for attr_copy_visited. Otherwise it's OK. Jeff