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
