Re: [PATCH] detect attribute mismatches in alias declarations (PR 81824)
On 11/12/2018 11:29 AM, Matthew Malcomson wrote:
Hello Martin,
The new testcase Wattribute-alias.c fails on targets without ifunc
support (e.g. aarch64-none-elf cross-build).
It seems that just adding a directive `{ dg-require-ifunc "" }` to the
test file changes the test to unsupported instead of having a fail.
I don't know much about this patch so I don't know if the non-ifunc
checks would still be useful on such targets.
Would the simple change be OK? or would it be best to split the test
file into multiple parts to still run the other checks?
I just committed the former change earlier today but splitting
the test would have probably been a better way to go. Thanks
for reporting it just the same! If you would prefer to split
the test that would be fine with me.
Martin
Regards,
Matthew
On 09/11/18 17:33, Martin Sebor wrote:
+/* 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?
The variable was intended to break cycles in recursive calls to
the function for self-referential applications of attribute copy
but since the attribute itself is not applied (anymore) such cycles
can no longer form. I have removed the variable and simplified
the handlers (there are tests to verify this works correctly).
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" ? :-)
Fixed.
Looks pretty good. There's the explicit specialization nit and the
static vs auto question for attr_copy_visited. Otherwise it's OK.
Thanks. I've retested a revision with the changes discussed here
and committed it as r265980.
Martin
Re: [PATCH] detect attribute mismatches in alias declarations (PR 81824)
Hello Martin,
The new testcase Wattribute-alias.c fails on targets without ifunc
support (e.g. aarch64-none-elf cross-build).
It seems that just adding a directive `{ dg-require-ifunc "" }` to the
test file changes the test to unsupported instead of having a fail.
I don't know much about this patch so I don't know if the non-ifunc
checks would still be useful on such targets.
Would the simple change be OK? or would it be best to split the test
file into multiple parts to still run the other checks?
Regards,
Matthew
On 09/11/18 17:33, Martin Sebor wrote:
>>> +/* 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?
>
> The variable was intended to break cycles in recursive calls to
> the function for self-referential applications of attribute copy
> but since the attribute itself is not applied (anymore) such cycles
> can no longer form. I have removed the variable and simplified
> the handlers (there are tests to verify this works correctly).
>
>>> 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" ? :-)
>>
>
> Fixed.
>
>>
>> Looks pretty good. There's the explicit specialization nit and the
>> static vs auto question for attr_copy_visited. Otherwise it's OK.
>
> Thanks. I've retested a revision with the changes discussed here
> and committed it as r265980.
>
> Martin
Re: [PATCH] detect attribute mismatches in alias declarations (PR 81824)
+/* 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?
The variable was intended to break cycles in recursive calls to
the function for self-referential applications of attribute copy
but since the attribute itself is not applied (anymore) such cycles
can no longer form. I have removed the variable and simplified
the handlers (there are tests to verify this works correctly).
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" ? :-)
Fixed.
Looks pretty good. There's the explicit specialization nit and the
static vs auto question for attr_copy_visited. Otherwise it's OK.
Thanks. I've retested a revision with the changes discussed here
and committed it as r265980.
Martin
Re: [PATCH] detect attribute mismatches in alias declarations (PR 81824)
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 ar
Re: [PATCH] detect attribute mismatches in alias declarations (PR 81824)
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01481.html
There was quite a bit of discussion between Joseph and me about
the Glibc changes needed to take advantage of the solution but
the GCC patch itself (above) still needs reviewing/approval.
Other than some (minor) changes to the C++ front end the bulk
of the changes for review are to the attribute machinery in
c-family and in the middle-end (attribs.c).
With the Glibc patch below applied, the GCC patch builds Glibc
with no warnings by default. To find the alias attribute
mismatches as requested, Glibc would enable -Wattribute-alias=2.
Once the GCC patch is committed I will also submit the Glibc
patch.
Glibc patch for reference:
https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01538.html
On 10/24/2018 02:38 PM, Martin Sebor wrote:
On 10/24/2018 11:27 AM, Joseph Myers wrote:
On Wed, 24 Oct 2018, Martin Sebor wrote:
On 10/24/2018 06:22 AM, Joseph Myers wrote:
On Wed, 24 Oct 2018, Martin Sebor wrote:
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)));
The obvious question there is whether the glibc patch should use copy
(local) as well as copy (name) in the definition of __hidden_ver1.
I tried copy(local) but it breaks where local isn't declared.
I think errno_location was the first one I saw where it referred
to __GI__something_or_other that was previously only defined via
an asm.
In that case maybe it should go in the .c files (the patch should define
some common attribute_copy macro in some internal header, to avoid
lots of
places needing to duplicate conditionals for whether it's supported).
I defined __attribute_copy__ in cdefs.h like other attributes
and used it in libc-symbols.h and in the string .c files but
it turns out that cdefs.h isn't always included when the
libc-symbols.h macros are used. For instance,
sysdeps/unix/sysv/linux/umount.c is compiled without it which
results in errors. So the way I worked around it pretty hacky.
Attached is what I have. I'm sure there's a better way that
you will want to adopt for Glibc but this works as a proof of
concept and results in no warnings by default. With
-Wattribute-alias=2 it gives the attached warnings for aliases
with more restrictive attributes than their targets (alloc_size,
const, leaf, malloc, nonnull, noreturn, nothrow, pure, and
returns_nonnull).
The GCC patch is the same so please let me know if there's
something to change there.
Whether nonnull attributes should be disabled when building glibc is a
separate question which would involve reviewing lots of functions with
such attributes against
to see whether there are checks for NULL that are actually appropriate
under current glibc conventions but might be optimized away given such
attributes. So it should clearly be kept separate from fixes to use copy
attributes to get better attribute consistency fot aliases.
Agreed. I certainly don't plan to undertake this project as
part of the GCC attribute enhancement.
Martin
Re: [PATCH] detect attribute mismatches in alias declarations (PR 81824)
On 10/24/2018 11:27 AM, Joseph Myers wrote:
On Wed, 24 Oct 2018, Martin Sebor wrote:
On 10/24/2018 06:22 AM, Joseph Myers wrote:
On Wed, 24 Oct 2018, Martin Sebor wrote:
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)));
The obvious question there is whether the glibc patch should use copy
(local) as well as copy (name) in the definition of __hidden_ver1.
I tried copy(local) but it breaks where local isn't declared.
I think errno_location was the first one I saw where it referred
to __GI__something_or_other that was previously only defined via
an asm.
In that case maybe it should go in the .c files (the patch should define
some common attribute_copy macro in some internal header, to avoid lots of
places needing to duplicate conditionals for whether it's supported).
I defined __attribute_copy__ in cdefs.h like other attributes
and used it in libc-symbols.h and in the string .c files but
it turns out that cdefs.h isn't always included when the
libc-symbols.h macros are used. For instance,
sysdeps/unix/sysv/linux/umount.c is compiled without it which
results in errors. So the way I worked around it pretty hacky.
Attached is what I have. I'm sure there's a better way that
you will want to adopt for Glibc but this works as a proof of
concept and results in no warnings by default. With
-Wattribute-alias=2 it gives the attached warnings for aliases
with more restrictive attributes than their targets (alloc_size,
const, leaf, malloc, nonnull, noreturn, nothrow, pure, and
returns_nonnull).
The GCC patch is the same so please let me know if there's
something to change there.
Whether nonnull attributes should be disabled when building glibc is a
separate question which would involve reviewing lots of functions with
such attributes against
to see whether there are checks for NULL that are actually appropriate
under current glibc conventions but might be optimized away given such
attributes. So it should clearly be kept separate from fixes to use copy
attributes to get better attribute consistency fot aliases.
Agreed. I certainly don't plan to undertake this project as
part of the GCC attribute enhancement.
Martin
diff --git a/include/libc-symbols.h b/include/libc-symbols.h
index 8b9273c..e71a479 100644
--- a/include/libc-symbols.h
+++ b/include/libc-symbols.h
@@ -125,6 +125,11 @@
# define ASM_LINE_SEP ;
#endif
+#ifndef __attribute_copy__
+/* Provide an empty definition when cdefs.h is not included. */
+# define __attribute_copy__(arg)
+#endif
+
#ifndef __ASSEMBLER__
/* GCC understands weak symbols and aliases; use its interface where
possible, instead of embedded assembly language. */
@@ -132,7 +137,8 @@
/* Define ALIASNAME as a strong alias for NAME. */
# define strong_alias(name, aliasname) _strong_alias(name, aliasname)
# define _strong_alias(name, aliasname) \
- extern __typeof (name) aliasname __attribute__ ((alias (#name)));
+ extern __typeof (name) aliasname __attribute__ ((alias (#name))) \
+__attribute_copy__ (name);
/* This comes between the return type and function name in
a function definition to make that definition weak. */
@@ -143,14 +149,16 @@
If weak aliases are not available, this defines a strong alias. */
# define weak_alias(name, aliasname) _weak_alias (name, aliasname)
# define _weak_alias(name, aliasname) \
- extern __typeof (name) aliasname __attribute__ ((weak, alias (#name)));
+ extern __typeof (name) aliasname __attribute__ ((weak, alias (#name))) \
+__attribute_copy__ (name);
/* Same as WEAK_ALIAS, but mark symbol as hidden. */
# define weak_hidden_alias(name, aliasname) \
_weak_hidden_alias (name, aliasname)
# define _weak_hidden_alias(name, aliasname) \
extern __typeof (name) aliasname \
-__attribute__ ((weak, alias (#name), __visibility__ ("hidden")));
+__attribute__ ((weak, alias (#name), __visibility__ ("hidden"))) \
+__attribute_copy__ (name);
/* Declare SYMBOL as weak undefined symbol (resolved to 0 if not defined). */
# define weak_extern(symbol) _weak_extern (weak symbol)
@@ -532,7 +540,8 @@ for linking")
# define __hidden_ver1(local, internal, name) \
extern __typeof (name) __EI_##name __asm__(__hidden_asmname (#internal)); \
extern __typeof (name) __EI_##name \
- __attribute__((alias (__hidden_asmname (#local
+__attribute__((alias (__hidden_asmname (#local \
+__attribute_copy__ (name)
# define hidden_ver(local, name) __hidden_ver1(local, __GI_##name, name);
# define hidden_data_ver(local, name) hidden_ver(local, name)
# define hidden_def(name) __hidden_ver1(__GI_##name, name, name);
@@ -545,7 +554,8 @@ for linking")
# define __hidden_nolink1(local, inte
Re: [PATCH] detect attribute mismatches in alias declarations (PR 81824)
On Wed, 24 Oct 2018, Martin Sebor wrote:
> On 10/24/2018 06:22 AM, Joseph Myers wrote:
> > On Wed, 24 Oct 2018, Martin Sebor wrote:
> >
> > > 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)));
> >
> > The obvious question there is whether the glibc patch should use copy
> > (local) as well as copy (name) in the definition of __hidden_ver1.
>
> I tried copy(local) but it breaks where local isn't declared.
> I think errno_location was the first one I saw where it referred
> to __GI__something_or_other that was previously only defined via
> an asm.
In that case maybe it should go in the .c files (the patch should define
some common attribute_copy macro in some internal header, to avoid lots of
places needing to duplicate conditionals for whether it's supported).
Whether nonnull attributes should be disabled when building glibc is a
separate question which would involve reviewing lots of functions with
such attributes against
to see whether there are checks for NULL that are actually appropriate
under current glibc conventions but might be optimized away given such
attributes. So it should clearly be kept separate from fixes to use copy
attributes to get better attribute consistency fot aliases.
--
Joseph S. Myers
jos...@codesourcery.com
Re: [PATCH] detect attribute mismatches in alias declarations (PR 81824)
On 10/24/2018 06:24 AM, Joseph Myers wrote: On Wed, 24 Oct 2018, Martin Sebor wrote: /* 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. I think -fisolate-erroneous-paths-attribute controls something different (generating tests and traps rather than simply optimizing on the basis of a parameter not being NULL). You're right, the option that does what I was thinking of is -fdelete-null-pointer-checks and that one is enabled by default. I should update the description of the attribute to mention this. Martin
Re: [PATCH] detect attribute mismatches in alias declarations (PR 81824)
On 10/24/2018 06:22 AM, Joseph Myers wrote:
On Wed, 24 Oct 2018, Martin Sebor wrote:
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)));
The obvious question there is whether the glibc patch should use copy
(local) as well as copy (name) in the definition of __hidden_ver1.
I tried copy(local) but it breaks where local isn't declared.
I think errno_location was the first one I saw where it referred
to __GI__something_or_other that was previously only defined via
an asm.
Martin
Re: [PATCH] detect attribute mismatches in alias declarations (PR 81824)
On Wed, 24 Oct 2018, Martin Sebor wrote: > /* 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. I think -fisolate-erroneous-paths-attribute controls something different (generating tests and traps rather than simply optimizing on the basis of a parameter not being NULL). -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH] detect attribute mismatches in alias declarations (PR 81824)
On Wed, 24 Oct 2018, Martin Sebor wrote:
> 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)));
The obvious question there is whether the glibc patch should use copy
(local) as well as copy (name) in the definition of __hidden_ver1.
--
Joseph S. Myers
jos...@codesourcery.com
Re: [PATCH] detect attribute mismatches in alias declarations (PR 81824)
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
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.
diff --git a/gcc/attribs.c b
Re: [PATCH] detect attribute mismatches in alias declarations (PR 81824)
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). > 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). -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH] detect attribute mismatches in alias declarations (PR 81824)
On 10/01/2018 05:00 PM, Joseph Myers wrote: On Mon, 1 Oct 2018, Martin Sebor wrote: Testing the patch with Glibc triggers thousands of warnings of both kinds. After reviewing a small subset it became apparent Thousands of warnings suggests initially having the warning outside -Wall (though one might hope to move it into -Wall at some point, depending on how hard the warnings are to address and to what extent they appear at all for other packages - most don't make heavy use of aliases like that - or failing that, to enable it explicitly for glibc once all the warnings are fixed, since this is certainly a useful warning for glibc showing issues we want to fix) - it's not like the typical case of a new warning where you can quickly and easily fix all the instances in glibc, for all architectures, to keep it building with mainline GCC. I have improved the Glibc patch to avoid many of the warnings. To avoid most of the rest I have adjusted the GCC patch to make -Wattribute-alias a two-level warning, and to disregard mismatches 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 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’ I think both sets might be caused either by a bug/missing handling in my Glibc patch, or by a bug in the GCC patch, but I'm a little lost in the maze of Glibc ifunc macro to tell which just yet. I can keep looking into it but it would make it easier if you could apply it and see if the Glibc macros need tweaking. Enabling -Wattribute-alias=2 shows the remaining attribute mismatches (those highlighting "potential bugs," although I suspect few, if any, are real bugs; more than likely they are benign.) attribute called copy. The attribute copies attributes from one declaration (or type) to another. The attribute doesn't resolve all the warnings but it helps. (For actual use in glibc that use would of course need to be conditional on a GCC version supporting the attribute.) Sure. The patch is just to show what I did to get the warnings. (Attached is an update with the changes I mentioned above and the Glibc warning breakdown with it.) The class of warnings I noticed that can't be so easily handled are due to inconsistencies between ifuncs and their resolvers. One way to solve it might be to have resolvers automatically "inherit" all the attributes of their targets (and enhance GCC to warn for violations). Another way might be to expect resolvers to be explicitly declared with attribute copy to copy the attributes of all the targets (and also warn for violations). I'm not sure we should care about the attributes on IFUNC resolvers at all; no normal code will see a declaration of the resolver and also call the function, whereas lots of code calls functions under internal alias names that currently lack the same attributes as the public declaration has. It's also not obvious whether there might be more cases of attributes for a function that are inapplicable to IFUNC resolvers than just the attributes relating to a symbol rather than the function itself which are hardcoded as excluded. I agree that checking attributes on ifunc resolvers probably doesn't make much sense. It would be helpful to check their targets against the aliases, but that's a whole other project. Attached is the latest update along with the Glibc warning breakdown. Tested on x86_64-linux. Martin 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
Re: [PATCH] detect attribute mismatches in alias declarations (PR 81824)
On Mon, 1 Oct 2018, Martin Sebor wrote: > Testing the patch with Glibc triggers thousands of warnings of > both kinds. After reviewing a small subset it became apparent Thousands of warnings suggests initially having the warning outside -Wall (though one might hope to move it into -Wall at some point, depending on how hard the warnings are to address and to what extent they appear at all for other packages - most don't make heavy use of aliases like that - or failing that, to enable it explicitly for glibc once all the warnings are fixed, since this is certainly a useful warning for glibc showing issues we want to fix) - it's not like the typical case of a new warning where you can quickly and easily fix all the instances in glibc, for all architectures, to keep it building with mainline GCC. > attribute called copy. The attribute copies attributes from > one declaration (or type) to another. The attribute doesn't > resolve all the warnings but it helps. (For actual use in glibc that use would of course need to be conditional on a GCC version supporting the attribute.) > The class of warnings I noticed that can't be so easily handled > are due to inconsistencies between ifuncs and their resolvers. > One way to solve it might be to have resolvers automatically > "inherit" all the attributes of their targets (and enhance > GCC to warn for violations). Another way might be to expect > resolvers to be explicitly declared with attribute copy to copy > the attributes of all the targets (and also warn for violations). I'm not sure we should care about the attributes on IFUNC resolvers at all; no normal code will see a declaration of the resolver and also call the function, whereas lots of code calls functions under internal alias names that currently lack the same attributes as the public declaration has. It's also not obvious whether there might be more cases of attributes for a function that are inapplicable to IFUNC resolvers than just the attributes relating to a symbol rather than the function itself which are hardcoded as excluded. -- Joseph S. Myers jos...@codesourcery.com
[PATCH] detect attribute mismatches in alias declarations (PR 81824)
PR 81824 is a request to detect and diagnose alias declarations
with less restrictive attributes than those of their targets.
I promised I'd implement this for GCC 9 so with the end of
stage 1 approaching I figured it was about time to post my
attempt at this enhancement. I expect it to need tweaking
to make it easier to adopt.
The solution reuses for this purpose the -Wmissing-attributes
warning introduced in GCC 8 for C++. It goes beyond the C++
warning and also beyond what Joseph asked for and detects both
less and more restrictive attributes. (The latter triggers
-Wattributes but with the growing number of distinct checkes
in -Wattributes it might be worth thinking about splitting
some out into new options.)
Testing the patch with Glibc triggers thousands of warnings of
both kinds. After reviewing a small subset it became apparent
that dealing with the inconsistencies on such a scale calls for
a convenient mechanism to (at at a minimum) automatically copy
attributes between declarations, similar to how __typeof__ makes
it possible to use the type of an existing declaration as that
of a new one. The patch helps with this by introducing a new
attribute called copy. The attribute copies attributes from
one declaration (or type) to another. The attribute doesn't
resolve all the warnings but it helps.
The class of warnings I noticed that can't be so easily handled
are due to inconsistencies between ifuncs and their resolvers.
One way to solve it might be to have resolvers automatically
"inherit" all the attributes of their targets (and enhance
GCC to warn for violations). Another way might be to expect
resolvers to be explicitly declared with attribute copy to copy
the attributes of all the targets (and also warn for violations).
In the patch I have hardcoded a few attributes that don't get
copied I call those linkage and visibility attributes. I'm not
too happy about hardcoding things like this but the only other
alternative I could think of was parameterizing the copy
attribute on the set of other attributes not to copy, but since
those would almost always be the same as the harcoded ones, it
didn't seem worthwhile.
Martin
PS With the attached GCC and Glibc patches I get the following
breakdown of warnings in Glibc (the numbers are the total count,
the number of unique instances, and the number of files they are
in):
DiagnosticCount UniqueFiles
-Wattributes 1743 724 599
-Wmissing-attributes 90 24 22
The -Wattributes are of the sort:
version.c:54:37: warning: ‘gnu_get_libc_release’ specifies more
restrictive attribute than its target ‘__gnu_get_libc_release’:
‘nothrow’ [-Wattributes]
(The absence of the nothrow attribute accounts for the majority
of the warnings.)
An example of the -Wmissing-attributes instance is:
./../include/libc-symbols.h:534:26: warning: ‘__EI___redirect_strcat’
specifies less restrictive attributes than its target ‘strcat’: ‘leaf’,
‘nonnull’ [-Wmissing-attributes]
This is the ifunc resolver mismatch I mention above.
diff --git a/include/libc-symbols.h b/include/libc-symbols.h
index 8b9273c..b0fb728 100644
--- a/include/libc-symbols.h
+++ b/include/libc-symbols.h
@@ -143,7 +143,7 @@
If weak aliases are not available, this defines a strong alias. */
# define weak_alias(name, aliasname) _weak_alias (name, aliasname)
# define _weak_alias(name, aliasname) \
- extern __typeof (name) aliasname __attribute__ ((weak, alias (#name)));
+ extern __typeof (name) aliasname __attribute__ ((weak, alias (#name), copy (name)));
/* Same as WEAK_ALIAS, but mark symbol as hidden. */
# define weak_hidden_alias(name, aliasname) \
@@ -532,7 +532,7 @@ for linking")
# define __hidden_ver1(local, internal, name) \
extern __typeof (name) __EI_##name __asm__(__hidden_asmname (#internal)); \
extern __typeof (name) __EI_##name \
- __attribute__((alias (__hidden_asmname (#local
+__attribute__((alias (__hidden_asmname (#local)), copy (name)))
# define hidden_ver(local, name) __hidden_ver1(local, __GI_##name, name);
# define hidden_data_ver(local, name) hidden_ver(local, name)
# define hidden_def(name) __hidden_ver1(__GI_##name, name, name);
@@ -545,7 +545,8 @@ for linking")
# define __hidden_nolink1(local, internal, name, version) \
__hidden_nolink2 (local, internal, name, version)
# define __hidden_nolink2(local, internal, name, version) \
- extern __typeof (name) internal __attribute__ ((alias (#local))); \
+ extern __typeof (name) internal __attribute__ ((alias (#local), \
+ copy (name))); \
__hidden_nolink3 (local, internal, #name "@" #version)
# define __hidden_nolink3(local, internal, vername) \
__asm__ (".symver " #internal ", " vername);
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.
* c-attribs
