[PATCH] detect attribute mismatches in alias declarations (PR 81824)

[Jeff Law]

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<tree> 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