add support for GCC 9 attribute copy

Message ID c36d3219-c54b-962d-645b-04f5db4df6d4@gmail.com
State New, archived
Headers

Commit Message

Martin Sebor Nov. 9, 2018, 6:13 p.m. UTC
  GCC 9 has just gained an enhancement to help detect attribute
mismatches between alias declarations and their targets.  It
consists of a new warning, -Wattribute-alias, an enhancement to
an existing warning, -Wmissing-attributes, and a new attribute
called copy.

The purpose of the warnings is to help identify either possible
bugs (an alias declared with more restrictive attributes than
its target promises) or optimization or diagnostic opportunities
(an alias target missing some attributes that it could be declared
with that might benefit analysis and code generation).  The purpose
of the new attribute is to easily apply (almost) the same set of
attributes to one declaration as those already present on another.

As expected (and intended) the enhancement triggers warnings for
many alias declarations in Glibc code.  Attached is a patch I
tested with a recent Glibc to avoid all instances of the new
warnings (I did no testing beyond recompiling Glibc with it).
I post the patch here to help prevent failures in Glibc nightly
builds due to -Werror.  To fully benefit from the enhancement
Glibc will need to be compiled with -Wattribute-alias=2 and
remaining warnings reviewed and dealt with (IIRC, there are
a couple of thousand but most should be straightforward to deal
with).

If there's something I can do to help make the GCC enhancement
more useful please let me know.

Martin

PS For the background on this feature see GCC pr81824:
   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81824
  

Comments

Joseph Myers Nov. 9, 2018, 6:24 p.m. UTC | #1
On Fri, 9 Nov 2018, Martin Sebor wrote:

> include/ChangeLog:

We have one ChangeLog at top-level, so the reference should be to 
include/libc-symbols.h.

> diff --git a/sysdeps/x86_64/multiarch/memchr.c b/sysdeps/x86_64/multiarch/memchr.c
> index 016f578..ce2e69c 100644
> --- a/sysdeps/x86_64/multiarch/memchr.c
> +++ b/sysdeps/x86_64/multiarch/memchr.c
> @@ -30,6 +30,6 @@ libc_ifunc_redirected (__redirect_memchr, memchr, IFUNC_SELECTOR ());
>  strong_alias (memchr, __memchr)
>  # ifdef SHARED
>  __hidden_ver1 (memchr, __GI_memchr, __redirect_memchr)
> -  __attribute__((visibility ("hidden")));
> +__attribute__((visibility ("hidden"))) __attribute_copy__ (memchr);

Should not lose the indentation here.

> diff --git a/sysdeps/x86_64/multiarch/memset.c b/sysdeps/x86_64/multiarch/memset.c
> index 064841d..87246dd 100644
> --- a/sysdeps/x86_64/multiarch/memset.c
> +++ b/sysdeps/x86_64/multiarch/memset.c
> @@ -30,6 +30,6 @@ libc_ifunc_redirected (__redirect_memset, memset, IFUNC_SELECTOR ());
>  
>  # ifdef SHARED
>  __hidden_ver1 (memset, __GI_memset, __redirect_memset)
> -  __attribute__ ((visibility ("hidden")));
> +  __attribute__ ((visibility ("hidden")))  __attribute_copy__ (memset);

Should only have one space not two before __attribute_copy__.

> diff --git a/sysdeps/x86_64/multiarch/strcpy.c b/sysdeps/x86_64/multiarch/strcpy.c
> index 12e0e3f..838d916 100644
> --- a/sysdeps/x86_64/multiarch/strcpy.c
> +++ b/sysdeps/x86_64/multiarch/strcpy.c
> @@ -30,6 +30,6 @@ libc_ifunc_redirected (__redirect_strcpy, strcpy, IFUNC_SELECTOR ());
>  
>  # ifdef SHARED
>  __hidden_ver1 (strcpy, __GI_strcpy, __redirect_strcpy)
> -  __attribute__ ((visibility ("hidden")));
> +  __attribute__ ((visibility ("hidden")))  __attribute_copy__ (strcpy);

Likewise.

> diff --git a/sysdeps/x86_64/multiarch/strlen.c b/sysdeps/x86_64/multiarch/strlen.c
> index 1758d22..0860083 100644
> --- a/sysdeps/x86_64/multiarch/strlen.c
> +++ b/sysdeps/x86_64/multiarch/strlen.c
> @@ -29,6 +29,6 @@
>  libc_ifunc_redirected (__redirect_strlen, strlen, IFUNC_SELECTOR ());
>  # ifdef SHARED
>  __hidden_ver1 (strlen, __GI_strlen, __redirect_strlen)
> -  __attribute__((visibility ("hidden")));
> +  __attribute__((visibility ("hidden")))  __attribute_copy__ (strlen);

Likewise.

> diff --git a/sysdeps/x86_64/multiarch/strncpy.c b/sysdeps/x86_64/multiarch/strncpy.c
> index 3c3de8b..3201f0f 100644
> --- a/sysdeps/x86_64/multiarch/strncpy.c
> +++ b/sysdeps/x86_64/multiarch/strncpy.c
> @@ -30,6 +30,6 @@ libc_ifunc_redirected (__redirect_strncpy, strncpy, IFUNC_SELECTOR ());
>  
>  # ifdef SHARED
>  __hidden_ver1 (strncpy, __GI_strncpy, __redirect_strncpy)
> -  __attribute__ ((visibility ("hidden")));
> +  __attribute__ ((visibility ("hidden")))  __attribute_copy__ (strncpy);

Likewise.

> diff --git a/sysdeps/x86_64/multiarch/strspn.c b/sysdeps/x86_64/multiarch/strspn.c
> index 56ab4d9..8b80bdc 100644
> --- a/sysdeps/x86_64/multiarch/strspn.c
> +++ b/sysdeps/x86_64/multiarch/strspn.c
> @@ -30,6 +30,6 @@ libc_ifunc_redirected (__redirect_strspn, strspn, IFUNC_SELECTOR ());
>  
>  # ifdef SHARED
>  __hidden_ver1 (strspn, __GI_strspn, __redirect_strspn)
> -  __attribute__ ((visibility ("hidden")));
> +  __attribute__ ((visibility ("hidden")))  __attribute_copy__ (strspn);

Likewise.

OK with those fixes.
  
Martin Sebor Nov. 10, 2018, 12:35 a.m. UTC | #2
On 11/09/2018 11:24 AM, Joseph Myers wrote:
> On Fri, 9 Nov 2018, Martin Sebor wrote:
>
>> include/ChangeLog:
>
> We have one ChangeLog at top-level, so the reference should be to
> include/libc-symbols.h.
>
>> diff --git a/sysdeps/x86_64/multiarch/memchr.c b/sysdeps/x86_64/multiarch/memchr.c
>> index 016f578..ce2e69c 100644
>> --- a/sysdeps/x86_64/multiarch/memchr.c
>> +++ b/sysdeps/x86_64/multiarch/memchr.c
>> @@ -30,6 +30,6 @@ libc_ifunc_redirected (__redirect_memchr, memchr, IFUNC_SELECTOR ());
>>  strong_alias (memchr, __memchr)
>>  # ifdef SHARED
>>  __hidden_ver1 (memchr, __GI_memchr, __redirect_memchr)
>> -  __attribute__((visibility ("hidden")));
>> +__attribute__((visibility ("hidden"))) __attribute_copy__ (memchr);
>
> Should not lose the indentation here.
>
>> diff --git a/sysdeps/x86_64/multiarch/memset.c b/sysdeps/x86_64/multiarch/memset.c
>> index 064841d..87246dd 100644
>> --- a/sysdeps/x86_64/multiarch/memset.c
>> +++ b/sysdeps/x86_64/multiarch/memset.c
>> @@ -30,6 +30,6 @@ libc_ifunc_redirected (__redirect_memset, memset, IFUNC_SELECTOR ());
>>
>>  # ifdef SHARED
>>  __hidden_ver1 (memset, __GI_memset, __redirect_memset)
>> -  __attribute__ ((visibility ("hidden")));
>> +  __attribute__ ((visibility ("hidden")))  __attribute_copy__ (memset);
>
> Should only have one space not two before __attribute_copy__.
>
>> diff --git a/sysdeps/x86_64/multiarch/strcpy.c b/sysdeps/x86_64/multiarch/strcpy.c
>> index 12e0e3f..838d916 100644
>> --- a/sysdeps/x86_64/multiarch/strcpy.c
>> +++ b/sysdeps/x86_64/multiarch/strcpy.c
>> @@ -30,6 +30,6 @@ libc_ifunc_redirected (__redirect_strcpy, strcpy, IFUNC_SELECTOR ());
>>
>>  # ifdef SHARED
>>  __hidden_ver1 (strcpy, __GI_strcpy, __redirect_strcpy)
>> -  __attribute__ ((visibility ("hidden")));
>> +  __attribute__ ((visibility ("hidden")))  __attribute_copy__ (strcpy);
>
> Likewise.
>
>> diff --git a/sysdeps/x86_64/multiarch/strlen.c b/sysdeps/x86_64/multiarch/strlen.c
>> index 1758d22..0860083 100644
>> --- a/sysdeps/x86_64/multiarch/strlen.c
>> +++ b/sysdeps/x86_64/multiarch/strlen.c
>> @@ -29,6 +29,6 @@
>>  libc_ifunc_redirected (__redirect_strlen, strlen, IFUNC_SELECTOR ());
>>  # ifdef SHARED
>>  __hidden_ver1 (strlen, __GI_strlen, __redirect_strlen)
>> -  __attribute__((visibility ("hidden")));
>> +  __attribute__((visibility ("hidden")))  __attribute_copy__ (strlen);
>
> Likewise.
>
>> diff --git a/sysdeps/x86_64/multiarch/strncpy.c b/sysdeps/x86_64/multiarch/strncpy.c
>> index 3c3de8b..3201f0f 100644
>> --- a/sysdeps/x86_64/multiarch/strncpy.c
>> +++ b/sysdeps/x86_64/multiarch/strncpy.c
>> @@ -30,6 +30,6 @@ libc_ifunc_redirected (__redirect_strncpy, strncpy, IFUNC_SELECTOR ());
>>
>>  # ifdef SHARED
>>  __hidden_ver1 (strncpy, __GI_strncpy, __redirect_strncpy)
>> -  __attribute__ ((visibility ("hidden")));
>> +  __attribute__ ((visibility ("hidden")))  __attribute_copy__ (strncpy);
>
> Likewise.
>
>> diff --git a/sysdeps/x86_64/multiarch/strspn.c b/sysdeps/x86_64/multiarch/strspn.c
>> index 56ab4d9..8b80bdc 100644
>> --- a/sysdeps/x86_64/multiarch/strspn.c
>> +++ b/sysdeps/x86_64/multiarch/strspn.c
>> @@ -30,6 +30,6 @@ libc_ifunc_redirected (__redirect_strspn, strspn, IFUNC_SELECTOR ());
>>
>>  # ifdef SHARED
>>  __hidden_ver1 (strspn, __GI_strspn, __redirect_strspn)
>> -  __attribute__ ((visibility ("hidden")));
>> +  __attribute__ ((visibility ("hidden")))  __attribute_copy__ (strspn);
>
> Likewise.
>
> OK with those fixes.

I committed the updated patch as 1626a1c.

Martin
  
Martin Sebor Nov. 11, 2018, 9:19 p.m. UTC | #3
On 11/09/2018 05:35 PM, Martin Sebor wrote:
...
>
> I committed the updated patch as 1626a1c.

I should have mentioned this up front: the patch only avoids warnings
in the x86_64 files (and was only tested there). It doesn't touch
files for any other targets and (as Jeff just noted to me privately)
there are warnings in builds for some non-x86_64 targets, including
i686.  This is not unexpected and those targets will need tweaks
similar to those in the patch.  Joseph, let me know if you need my
help with any of it.

Martin
  
Joseph Myers Nov. 12, 2018, 1:49 p.m. UTC | #4
On Sun, 11 Nov 2018, Martin Sebor wrote:

> I should have mentioned this up front: the patch only avoids warnings
> in the x86_64 files (and was only tested there). It doesn't touch
> files for any other targets and (as Jeff just noted to me privately)
> there are warnings in builds for some non-x86_64 targets, including
> i686.  This is not unexpected and those targets will need tweaks
> similar to those in the patch.  Joseph, let me know if you need my
> help with any of it.

Could you look at the powerpc soft-float, s390x and mips failures, which 
look somewhat different (and thus could indicate issues with the details 
of how this warning / attribute are specified, as opposed to simply 
needing a few more copy attributes somewhere)?


powerpc soft-float:

../sysdeps/powerpc/nofpu/sim-full.c:26:1: error: 'tls_model' attribute ignored [-Werror=attributes]
   26 | libc_hidden_data_def (__sim_exceptions_thread);
      | ^~~~~~~~~~~~~~~~~~~~
../sysdeps/powerpc/nofpu/sim-full.c:30:1: error: 'tls_model' attribute ignored [-Werror=attributes]
   30 | libc_hidden_data_def (__sim_disabled_exceptions_thread);
      | ^~~~~~~~~~~~~~~~~~~~
../sysdeps/powerpc/nofpu/sim-full.c:33:1: error: 'tls_model' attribute ignored [-Werror=attributes]
   33 | libc_hidden_data_def (__sim_round_mode_thread);
      | ^~~~~~~~~~~~~~~~~~~~


s390x produces a long series of errors starting with:

../sysdeps/s390/multiarch/utf8-utf16-z9.c:26:18: error: always_inline function might not be inlinable [-Werror=attributes]


mips (o32) produces:

../sysdeps/mips/__longjmp.c:84:1: error: '__longjmp' redeclared with conflicting 'nomips16' attributes
   84 | strong_alias (____longjmp, __longjmp);
      | ^~~~~~~~~~~~

and in this case I think it's clearly correct for ____longjmp 
(ABI-specific definition) to have the attribute, but the alias __longjmp 
(declared in an architecture-independent header) not to have it in that 
header (and so shouldn't get it copied either).
  
Martin Sebor Nov. 12, 2018, 8:18 p.m. UTC | #5
On 11/12/2018 06:49 AM, Joseph Myers wrote:
> On Sun, 11 Nov 2018, Martin Sebor wrote:
>
>> I should have mentioned this up front: the patch only avoids warnings
>> in the x86_64 files (and was only tested there). It doesn't touch
>> files for any other targets and (as Jeff just noted to me privately)
>> there are warnings in builds for some non-x86_64 targets, including
>> i686.  This is not unexpected and those targets will need tweaks
>> similar to those in the patch.  Joseph, let me know if you need my
>> help with any of it.
>
> Could you look at the powerpc soft-float, s390x and mips failures, which
> look somewhat different (and thus could indicate issues with the details
> of how this warning / attribute are specified, as opposed to simply
> needing a few more copy attributes somewhere)?

Do you by chance have an easy way to get the preprocessed sources
for these files so I can easily verify that my quick test cases
match the real problems?

> powerpc soft-float:
>
> ../sysdeps/powerpc/nofpu/sim-full.c:26:1: error: 'tls_model' attribute ignored [-Werror=attributes]
>    26 | libc_hidden_data_def (__sim_exceptions_thread);
>       | ^~~~~~~~~~~~~~~~~~~~
> ../sysdeps/powerpc/nofpu/sim-full.c:30:1: error: 'tls_model' attribute ignored [-Werror=attributes]
>    30 | libc_hidden_data_def (__sim_disabled_exceptions_thread);
>       | ^~~~~~~~~~~~~~~~~~~~
> ../sysdeps/powerpc/nofpu/sim-full.c:33:1: error: 'tls_model' attribute ignored [-Werror=attributes]
>    33 | libc_hidden_data_def (__sim_round_mode_thread);
>       | ^~~~~~~~~~~~~~~~~~~~

Based on the warning alone I think the test case below shows
the problem:

   __thread int i __attribute__ ((tls_model ("local-exec")));
   extern __attribute__ ((alias ("i"), copy (i))) int j;

tls_model is one of the few attributes I didn't test.  The warning
suggests the copy attribute code might need to filter out some more
attributes.  Let me work on that.

>
> s390x produces a long series of errors starting with:
>
> ../sysdeps/s390/multiarch/utf8-utf16-z9.c:26:18: error: always_inline function might not be inlinable [-Werror=attributes]

I think the following might be it:

   static inline __attribute__ ((always_inline)) void f (void) { }

   extern __attribute__ ((alias ("f"), copy (f))) void g (void);

Does it look close to what's going on in the file?  If so, does
it make sense to define an alias target inline/always_inline?
(I can filter attribute always_inline out of copy but I wonder
if changing the target to avoid always_inline would be more
appropriate.)

>
> mips (o32) produces:
>
> ../sysdeps/mips/__longjmp.c:84:1: error: '__longjmp' redeclared with conflicting 'nomips16' attributes
>    84 | strong_alias (____longjmp, __longjmp);
>       | ^~~~~~~~~~~~

I think this is it:

   extern void g (void);

   static __attribute__ ((nomips16)) void f (void) { }

   extern __attribute__ ((alias ("f"), copy (f))) __typeof__ (f) g;

> and in this case I think it's clearly correct for ____longjmp
> (ABI-specific definition) to have the attribute, but the alias __longjmp
> (declared in an architecture-independent header) not to have it in that
> header (and so shouldn't get it copied either).

So it sounds like the mips16 attribute is meaningful on definitions
but doesn't impact callers, correct?  I think it would be useful to
reflect this distinction in GCC's struct attribute_spec.  That way
attribute copy could simply check a bit in the struct rather than
the handler having to hardcode knowledge of these (sometimes target
specific) details.

Martin
  
Joseph Myers Nov. 12, 2018, 11:10 p.m. UTC | #6
On Mon, 12 Nov 2018, Martin Sebor wrote:

> > Could you look at the powerpc soft-float, s390x and mips failures, which
> > look somewhat different (and thus could indicate issues with the details
> > of how this warning / attribute are specified, as opposed to simply
> > needing a few more copy attributes somewhere)?
> 
> Do you by chance have an easy way to get the preprocessed sources
> for these files so I can easily verify that my quick test cases
> match the real problems?

Attached.

> Based on the warning alone I think the test case below shows
> the problem:
> 
>   __thread int i __attribute__ ((tls_model ("local-exec")));
>   extern __attribute__ ((alias ("i"), copy (i))) int j;
> 
> tls_model is one of the few attributes I didn't test.  The warning
> suggests the copy attribute code might need to filter out some more
> attributes.  Let me work on that.

Or maybe some variable of libc_hidden_data_def is needed that uses 
__thread when defining aliases, in which case tls_model attributes might 
not be considered to be ignored?  (The declarations in soft-supp.h use 
libc_hidden_tls_proto which does use __thread when defining aliases.)

> Does it look close to what's going on in the file?  If so, does
> it make sense to define an alias target inline/always_inline?
> (I can filter attribute always_inline out of copy but I wonder
> if changing the target to avoid always_inline would be more
> appropriate.)

I can imagine there might be uses for having some aliases that should be 
inlined and others that shouldn't be (provided there is actually an 
external definition for the function - if it's GNU extern inline with no 
separate definition provided, aliases aren't exactly useful).

> So it sounds like the mips16 attribute is meaningful on definitions
> but doesn't impact callers, correct?  I think it would be useful to

Strictly speaking I think it *can* impact callers by telling them whether 
they might need to generate code compatible with interlinking mips16 and 
non-mips16 for that call (in the absence of the attribute, the caller 
needs to make safe assumptions about not knowing what instruction set the 
function is built for).

The purpose of the alias, however, is definitely to avoid problems with 
declarations of __longjmp being inconsistent as to whether they have the 
attribute; see 
<https://sourceware.org/ml/libc-ports/2013-01/msg00047.html>.
  
Martin Sebor Nov. 13, 2018, 12:55 a.m. UTC | #7
On 11/12/2018 04:10 PM, Joseph Myers wrote:
> On Mon, 12 Nov 2018, Martin Sebor wrote:
>
>>> Could you look at the powerpc soft-float, s390x and mips failures, which
>>> look somewhat different (and thus could indicate issues with the details
>>> of how this warning / attribute are specified, as opposed to simply
>>> needing a few more copy attributes somewhere)?
>>
>> Do you by chance have an easy way to get the preprocessed sources
>> for these files so I can easily verify that my quick test cases
>> match the real problems?
>
> Attached.

Thanks!

>
>> Based on the warning alone I think the test case below shows
>> the problem:
>>
>>   __thread int i __attribute__ ((tls_model ("local-exec")));
>>   extern __attribute__ ((alias ("i"), copy (i))) int j;
>>
>> tls_model is one of the few attributes I didn't test.  The warning
>> suggests the copy attribute code might need to filter out some more
>> attributes.  Let me work on that.
>
> Or maybe some variable of libc_hidden_data_def is needed that uses
> __thread when defining aliases, in which case tls_model attributes might
> not be considered to be ignored?  (The declarations in soft-supp.h use
> libc_hidden_tls_proto which does use __thread when defining aliases.)

Attached is a small test case reduced from the TU.

Attribute copy calls decl_attributes to copy tls_model from
__sim_exceptions_thread to __EI___sim_exceptions_thread.  That
triggers a warning because
DECL_THREAD_LOCAL_P (__EI___sim_exceptions_thread) is false.
I think __EI___sim_exceptions_thread needs to be declared
thread-local (just like __sim_exceptions_thread) for this
to work.  Can you think of something better?

>> Does it look close to what's going on in the file?  If so, does
>> it make sense to define an alias target inline/always_inline?
>> (I can filter attribute always_inline out of copy but I wonder
>> if changing the target to avoid always_inline would be more
>> appropriate.)
>
> I can imagine there might be uses for having some aliases that should be
> inlined and others that shouldn't be (provided there is actually an
> external definition for the function - if it's GNU extern inline with no
> separate definition provided, aliases aren't exactly useful).

Okay, let me just exclude always_inline and gnu_inline from
the attributes to copy and not worry about this as a potential
problem for now.

>> So it sounds like the mips16 attribute is meaningful on definitions
>> but doesn't impact callers, correct?  I think it would be useful to
>
> Strictly speaking I think it *can* impact callers by telling them whether
> they might need to generate code compatible with interlinking mips16 and
> non-mips16 for that call (in the absence of the attribute, the caller
> needs to make safe assumptions about not knowing what instruction set the
> function is built for).
>
> The purpose of the alias, however, is definitely to avoid problems with
> declarations of __longjmp being inconsistent as to whether they have the
> attribute; see
> <https://sourceware.org/ml/libc-ports/2013-01/msg00047.html>.

Hmm.  I confess I'm confused by this and by the referenced
discussions.  Most of it sounds like the alias in this case
suppresses a valid error: the mismatch between the caller and
the callee.  But then the conclusion seems to be that
declaring a function one way and defining it another is (or
can be, perhaps with some care) okay.  So either issuing
an error for the mismatch between declarations of the same
function is overly strict (in which case a warning for it
should be appropriate, as would be the attribute warning) or
the error does makes sense in which case so should the attribute
warning.

I'm open to filtering out these two attributes since it doesn't
seem that important one way or the other but unless I'm even
more confused than I think I am I would prefer to keep things
as they are and change Glibc to suppress the warning.  Partly
because of the rationale I just gave and in part also because
I don't want to hardcode a target attribute into c-attribs.c
and I'm not convinced yet we need to introduce some sort of
a hook for this (though ultimately we might end up going that
route).

Let me know if you see a problem with suppressing the warning
in Glibc.

Martin
extern __thread int
__sim_exceptions_thread
__attribute__ ((tls_model ("initial-exec")));

extern __thread __typeof (__sim_exceptions_thread)
  __sim_exceptions_thread __asm__ ("__GI___sim_exceptions_thread")
  __attribute__ ((visibility ("hidden"), tls_model ("initial-exec")));;

extern __thread int
__sim_exceptions_thread __attribute__ ((tls_model ("initial-exec")));

extern __thread __typeof (__sim_exceptions_thread)
  __sim_exceptions_thread __asm__ ("__GI___sim_exceptions_thread")
  __attribute__ ((visibility ("hidden"), tls_model ("initial-exec")));;

__thread int __sim_exceptions_thread __attribute__ ((nocommon));

extern __thread __typeof (__sim_exceptions_thread)
  __EI___sim_exceptions_thread __asm__ ("__sim_exceptions_thread");

extern __thread __typeof (__sim_exceptions_thread)
  __EI___sim_exceptions_thread
  __attribute__((alias ("__GI___sim_exceptions_thread")))
  __attribute__ ((__copy__ (__sim_exceptions_thread)));
  
Joseph Myers Nov. 13, 2018, 1:57 a.m. UTC | #8
On Mon, 12 Nov 2018, Martin Sebor wrote:

> Attribute copy calls decl_attributes to copy tls_model from
> __sim_exceptions_thread to __EI___sim_exceptions_thread.  That
> triggers a warning because
> DECL_THREAD_LOCAL_P (__EI___sim_exceptions_thread) is false.
> I think __EI___sim_exceptions_thread needs to be declared
> thread-local (just like __sim_exceptions_thread) for this
> to work.  Can you think of something better?

I've now applied a patch to add hidden_tls_def macros to deal with this.

> Let me know if you see a problem with suppressing the warning
> in Glibc.

I've now applied a patch to stop the MIPS case using strong_alias at all 
so that it doesn't get the copy attribute used.
  
Joseph Myers Nov. 13, 2018, 10:38 p.m. UTC | #9
Samuel, could you look at the build failures for i686-gnu with mainline 
GCC after this GCC change?

../sysdeps/mach/hurd/dl-sysdep.c:286:26: error: '__check_abort_no_hidden' specifies less restrictive attributes than its target 'abort': 'cold', 'leaf', 'noreturn', 'nothrow' [-Werror=missing-attributes]

(and a series of further such errors).

Supposing that the latest GCC change fixes the s390x-linux-gnu build 
failure, it's possible the i686-gnu failure is the last one remaining from 
this issue.  (ia64-linux-gnu still fails to build with GCC mainline in the 
build-many-glibcs.py logs, but that's a separate ICE building libstdc++, 
reported at <https://gcc.gnu.org/ml/gcc-patches/2018-09/msg01052.html> and 
remaining unfixed.)
  
Jeff Law Nov. 13, 2018, 10:42 p.m. UTC | #10
On 11/13/18 3:38 PM, Joseph Myers wrote:
> Samuel, could you look at the build failures for i686-gnu with mainline 
> GCC after this GCC change?
> 
> ../sysdeps/mach/hurd/dl-sysdep.c:286:26: error: '__check_abort_no_hidden' specifies less restrictive attributes than its target 'abort': 'cold', 'leaf', 'noreturn', 'nothrow' [-Werror=missing-attributes]
> 
> (and a series of further such errors).
> 
> Supposing that the latest GCC change fixes the s390x-linux-gnu build 
> failure, it's possible the i686-gnu failure is the last one remaining from 
> this issue.  (ia64-linux-gnu still fails to build with GCC mainline in the 
> build-many-glibcs.py logs, but that's a separate ICE building libstdc++, 
> reported at <https://gcc.gnu.org/ml/gcc-patches/2018-09/msg01052.html> and 
> remaining unfixed.)
ia-64 has been dead for about 2 months according to my tester...  Even
if the most eggregious problems are fixed (ie, gcc bootstrap), it'll
still trip over the qsort checking stuff building either glibc or the
kernel.

I'm tempted to ask for it to be deprecated ;-)

jeff
  
Samuel Thibault Nov. 14, 2018, 12:39 a.m. UTC | #11
Joseph Myers, le mar. 13 nov. 2018 22:38:13 +0000, a ecrit:
> Samuel, could you look at the build failures for i686-gnu with mainline 
> GCC after this GCC change?
> 
> ../sysdeps/mach/hurd/dl-sysdep.c:286:26: error: '__check_abort_no_hidden' specifies less restrictive attributes than its target 'abort': 'cold', 'leaf', 'noreturn', 'nothrow' [-Werror=missing-attributes]
> 
> (and a series of further such errors).

Now fixed :)

Samuel
  

Patch

include/ChangeLog:

	* libc-symbols.h (__attribute_copy__): Define macro unless
	it's already defined.
	(_strong_alias): Use __attribute_copy__.
	(_weak_alias,  __hidden_ver1,  __hidden_nolink2): Same.
	* misc/sys/cdefs.h (__attribute_copy__): New macro.
	* sysdeps/x86_64/multiarch/memchr.c (memchr): Use __attribute_copy__.
	* sysdeps/x86_64/multiarch/memcmp.c (memcmp): Same.
	* sysdeps/x86_64/multiarch/mempcpy.c (mempcpy): Same.
	* sysdeps/x86_64/multiarch/memset.c (memset): Same.
	* sysdeps/x86_64/multiarch/stpcpy.c (stpcpy): Same.
	* sysdeps/x86_64/multiarch/strcat.c (strcat): Same.
	* sysdeps/x86_64/multiarch/strchr.c (strchr): Same.
	* sysdeps/x86_64/multiarch/strcmp.c (strcmp): Same.
	* sysdeps/x86_64/multiarch/strcpy.c (strcpy): Same.
	* sysdeps/x86_64/multiarch/strcspn.c (strcspn): Same.
	* sysdeps/x86_64/multiarch/strlen.c (strlen): Same.
	* sysdeps/x86_64/multiarch/strncmp.c (strncmp): Same.
	* sysdeps/x86_64/multiarch/strncpy.c (strncpy): Same.
	* sysdeps/x86_64/multiarch/strnlen.c (strnlen): Same.
	* sysdeps/x86_64/multiarch/strpbrk.c (strpbrk): Same.
	* sysdeps/x86_64/multiarch/strrchr.c (strrchr): Same.
	* sysdeps/x86_64/multiarch/strspn.c (strspn): Same.


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, 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)))	\
+    __attribute_copy__ (name);						\
   __hidden_nolink3 (local, internal, #name "@" #version)
 #  define __hidden_nolink3(local, internal, vername) \
   __asm__ (".symver " #internal ", " vername);
diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
index 3f6fe3c..8d58568 100644
--- a/misc/sys/cdefs.h
+++ b/misc/sys/cdefs.h
@@ -431,6 +431,16 @@ 
 # define __attribute_nonstring__
 #endif
 
+/* Undefine (also defined in libc-symbols.h).  */
+#undef __attribute_copy__
+#if __GNUC_PREREQ (9, 0)
+/* Copies attributes from the declaration or type referenced by
+   the argument.  */
+# define __attribute_copy__(arg) __attribute__ ((__copy__ (arg)))
+#else
+# define __attribute_copy__(arg)
+#endif
+
 #if (!defined _Static_assert && !defined __cplusplus \
      && (defined __STDC_VERSION__ ? __STDC_VERSION__ : 0) < 201112 \
      && (!__GNUC_PREREQ (4, 6) || defined __STRICT_ANSI__))
diff --git a/sysdeps/x86_64/multiarch/memchr.c b/sysdeps/x86_64/multiarch/memchr.c
index 016f578..ce2e69c 100644
--- a/sysdeps/x86_64/multiarch/memchr.c
+++ b/sysdeps/x86_64/multiarch/memchr.c
@@ -30,6 +30,6 @@  libc_ifunc_redirected (__redirect_memchr, memchr, IFUNC_SELECTOR ());
 strong_alias (memchr, __memchr)
 # ifdef SHARED
 __hidden_ver1 (memchr, __GI_memchr, __redirect_memchr)
-  __attribute__((visibility ("hidden")));
+__attribute__((visibility ("hidden"))) __attribute_copy__ (memchr);
 # endif
 #endif
diff --git a/sysdeps/x86_64/multiarch/memcmp.c b/sysdeps/x86_64/multiarch/memcmp.c
index 6f3ca43..bbd3b01 100644
--- a/sysdeps/x86_64/multiarch/memcmp.c
+++ b/sysdeps/x86_64/multiarch/memcmp.c
@@ -32,6 +32,6 @@  weak_alias (memcmp, bcmp)
 
 # ifdef SHARED
 __hidden_ver1 (memcmp, __GI_memcmp, __redirect_memcmp)
-  __attribute__ ((visibility ("hidden")));
+  __attribute__ ((visibility ("hidden"))) __attribute_copy__ (memcmp);
 # endif
 #endif
diff --git a/sysdeps/x86_64/multiarch/mempcpy.c b/sysdeps/x86_64/multiarch/mempcpy.c
index 9fe41dd..d2f7928 100644
--- a/sysdeps/x86_64/multiarch/mempcpy.c
+++ b/sysdeps/x86_64/multiarch/mempcpy.c
@@ -35,8 +35,8 @@  libc_ifunc_redirected (__redirect_mempcpy, __mempcpy, IFUNC_SELECTOR ());
 weak_alias (__mempcpy, mempcpy)
 # ifdef SHARED
 __hidden_ver1 (__mempcpy, __GI___mempcpy, __redirect___mempcpy)
-  __attribute__ ((visibility ("hidden")));
+  __attribute__ ((visibility ("hidden"))) __attribute_copy__ (mempcpy);
 __hidden_ver1 (mempcpy, __GI_mempcpy, __redirect_mempcpy)
-  __attribute__ ((visibility ("hidden")));
+  __attribute__ ((visibility ("hidden"))) __attribute_copy__ (mempcpy);
 # endif
 #endif
diff --git a/sysdeps/x86_64/multiarch/memset.c b/sysdeps/x86_64/multiarch/memset.c
index 064841d..87246dd 100644
--- a/sysdeps/x86_64/multiarch/memset.c
+++ b/sysdeps/x86_64/multiarch/memset.c
@@ -30,6 +30,6 @@  libc_ifunc_redirected (__redirect_memset, memset, IFUNC_SELECTOR ());
 
 # ifdef SHARED
 __hidden_ver1 (memset, __GI_memset, __redirect_memset)
-  __attribute__ ((visibility ("hidden")));
+  __attribute__ ((visibility ("hidden")))  __attribute_copy__ (memset);
 # endif
 #endif
diff --git a/sysdeps/x86_64/multiarch/stpcpy.c b/sysdeps/x86_64/multiarch/stpcpy.c
index 1e340fc..f74a54b 100644
--- a/sysdeps/x86_64/multiarch/stpcpy.c
+++ b/sysdeps/x86_64/multiarch/stpcpy.c
@@ -35,8 +35,8 @@  libc_ifunc_redirected (__redirect_stpcpy, __stpcpy, IFUNC_SELECTOR ());
 weak_alias (__stpcpy, stpcpy)
 # ifdef SHARED
 __hidden_ver1 (__stpcpy, __GI___stpcpy, __redirect___stpcpy)
-  __attribute__ ((visibility ("hidden")));
+  __attribute__ ((visibility ("hidden"))) __attribute_copy__ (stpcpy);
 __hidden_ver1 (stpcpy, __GI_stpcpy, __redirect_stpcpy)
-  __attribute__ ((visibility ("hidden")));
+  __attribute__ ((visibility ("hidden"))) __attribute_copy__ (stpcpy);
 # endif
 #endif
diff --git a/sysdeps/x86_64/multiarch/strcat.c b/sysdeps/x86_64/multiarch/strcat.c
index 1f7f626..1922c0a 100644
--- a/sysdeps/x86_64/multiarch/strcat.c
+++ b/sysdeps/x86_64/multiarch/strcat.c
@@ -30,6 +30,6 @@  libc_ifunc_redirected (__redirect_strcat, strcat, IFUNC_SELECTOR ());
 
 # ifdef SHARED
 __hidden_ver1 (strcat, __GI_strcat, __redirect_strcat)
-  __attribute__ ((visibility ("hidden")));
+  __attribute__ ((visibility ("hidden"))) __attribute_copy__ (strcat);
 # endif
 #endif
diff --git a/sysdeps/x86_64/multiarch/strchr.c b/sysdeps/x86_64/multiarch/strchr.c
index 76d64fb..87e99ba 100644
--- a/sysdeps/x86_64/multiarch/strchr.c
+++ b/sysdeps/x86_64/multiarch/strchr.c
@@ -50,6 +50,6 @@  libc_ifunc_redirected (__redirect_strchr, strchr, IFUNC_SELECTOR ());
 weak_alias (strchr, index)
 # ifdef SHARED
 __hidden_ver1 (strchr, __GI_strchr, __redirect_strchr)
-  __attribute__((visibility ("hidden")));
+  __attribute__((visibility ("hidden"))) __attribute_copy__ (strchr);
 # endif
 #endif
diff --git a/sysdeps/x86_64/multiarch/strcmp.c b/sysdeps/x86_64/multiarch/strcmp.c
index b903e41..e3cf39d 100644
--- a/sysdeps/x86_64/multiarch/strcmp.c
+++ b/sysdeps/x86_64/multiarch/strcmp.c
@@ -54,6 +54,6 @@  libc_ifunc_redirected (__redirect_strcmp, strcmp, IFUNC_SELECTOR ());
 
 # ifdef SHARED
 __hidden_ver1 (strcmp, __GI_strcmp, __redirect_strcmp)
-  __attribute__ ((visibility ("hidden")));
+  __attribute__ ((visibility ("hidden"))) __attribute_copy__ (strcmp);
 # endif
 #endif
diff --git a/sysdeps/x86_64/multiarch/strcpy.c b/sysdeps/x86_64/multiarch/strcpy.c
index 12e0e3f..838d916 100644
--- a/sysdeps/x86_64/multiarch/strcpy.c
+++ b/sysdeps/x86_64/multiarch/strcpy.c
@@ -30,6 +30,6 @@  libc_ifunc_redirected (__redirect_strcpy, strcpy, IFUNC_SELECTOR ());
 
 # ifdef SHARED
 __hidden_ver1 (strcpy, __GI_strcpy, __redirect_strcpy)
-  __attribute__ ((visibility ("hidden")));
+  __attribute__ ((visibility ("hidden")))  __attribute_copy__ (strcpy);
 # endif
 #endif
diff --git a/sysdeps/x86_64/multiarch/strcspn.c b/sysdeps/x86_64/multiarch/strcspn.c
index 9712e84..9d96526 100644
--- a/sysdeps/x86_64/multiarch/strcspn.c
+++ b/sysdeps/x86_64/multiarch/strcspn.c
@@ -30,6 +30,6 @@  libc_ifunc_redirected (__redirect_strcspn, strcspn, IFUNC_SELECTOR ());
 
 # ifdef SHARED
 __hidden_ver1 (strcspn, __GI_strcspn, __redirect_strcspn)
-  __attribute__ ((visibility ("hidden")));
+  __attribute__ ((visibility ("hidden"))) __attribute_copy__ (strcspn);
 # endif
 #endif
diff --git a/sysdeps/x86_64/multiarch/strlen.c b/sysdeps/x86_64/multiarch/strlen.c
index 1758d22..0860083 100644
--- a/sysdeps/x86_64/multiarch/strlen.c
+++ b/sysdeps/x86_64/multiarch/strlen.c
@@ -29,6 +29,6 @@ 
 libc_ifunc_redirected (__redirect_strlen, strlen, IFUNC_SELECTOR ());
 # ifdef SHARED
 __hidden_ver1 (strlen, __GI_strlen, __redirect_strlen)
-  __attribute__((visibility ("hidden")));
+  __attribute__((visibility ("hidden")))  __attribute_copy__ (strlen);
 # endif
 #endif
diff --git a/sysdeps/x86_64/multiarch/strncmp.c b/sysdeps/x86_64/multiarch/strncmp.c
index 02b6d0b..32f5c6c 100644
--- a/sysdeps/x86_64/multiarch/strncmp.c
+++ b/sysdeps/x86_64/multiarch/strncmp.c
@@ -55,6 +55,6 @@  libc_ifunc_redirected (__redirect_strncmp, strncmp, IFUNC_SELECTOR ());
 
 # ifdef SHARED
 __hidden_ver1 (strncmp, __GI_strncmp, __redirect_strncmp)
-  __attribute__ ((visibility ("hidden")));
+  __attribute__ ((visibility ("hidden"))) __attribute_copy__ (strncmp);
 # endif
 #endif
diff --git a/sysdeps/x86_64/multiarch/strncpy.c b/sysdeps/x86_64/multiarch/strncpy.c
index 3c3de8b..3201f0f 100644
--- a/sysdeps/x86_64/multiarch/strncpy.c
+++ b/sysdeps/x86_64/multiarch/strncpy.c
@@ -30,6 +30,6 @@  libc_ifunc_redirected (__redirect_strncpy, strncpy, IFUNC_SELECTOR ());
 
 # ifdef SHARED
 __hidden_ver1 (strncpy, __GI_strncpy, __redirect_strncpy)
-  __attribute__ ((visibility ("hidden")));
+  __attribute__ ((visibility ("hidden")))  __attribute_copy__ (strncpy);
 # endif
 #endif
diff --git a/sysdeps/x86_64/multiarch/strnlen.c b/sysdeps/x86_64/multiarch/strnlen.c
index 3ab94ce..9d64335 100644
--- a/sysdeps/x86_64/multiarch/strnlen.c
+++ b/sysdeps/x86_64/multiarch/strnlen.c
@@ -32,8 +32,8 @@  libc_ifunc_redirected (__redirect_strnlen, __strnlen, IFUNC_SELECTOR ());
 weak_alias (__strnlen, strnlen);
 # ifdef SHARED
 __hidden_ver1 (__strnlen, __GI___strnlen, __redirect___strnlen)
-  __attribute__((visibility ("hidden")));
+  __attribute__((visibility ("hidden"))) __attribute_copy__ (strnlen);
 __hidden_ver1 (strnlen, __GI_strnlen, __redirect_strnlen)
-  __attribute__((weak, visibility ("hidden")));
+  __attribute__((weak, visibility ("hidden"))) __attribute_copy__ (strnlen);
 # endif
 #endif
diff --git a/sysdeps/x86_64/multiarch/strpbrk.c b/sysdeps/x86_64/multiarch/strpbrk.c
index a0d435a..f110367 100644
--- a/sysdeps/x86_64/multiarch/strpbrk.c
+++ b/sysdeps/x86_64/multiarch/strpbrk.c
@@ -30,6 +30,6 @@  libc_ifunc_redirected (__redirect_strpbrk, strpbrk, IFUNC_SELECTOR ());
 
 # ifdef SHARED
 __hidden_ver1 (strpbrk, __GI_strpbrk, __redirect_strpbrk)
-  __attribute__ ((visibility ("hidden")));
+  __attribute__ ((visibility ("hidden"))) __attribute_copy__ (strpbrk);
 # endif
 #endif
diff --git a/sysdeps/x86_64/multiarch/strrchr.c b/sysdeps/x86_64/multiarch/strrchr.c
index a719edd..ba7458a 100644
--- a/sysdeps/x86_64/multiarch/strrchr.c
+++ b/sysdeps/x86_64/multiarch/strrchr.c
@@ -29,6 +29,6 @@  libc_ifunc_redirected (__redirect_strrchr, strrchr, IFUNC_SELECTOR ());
 weak_alias (strrchr, rindex);
 # ifdef SHARED
 __hidden_ver1 (strrchr, __GI_strrchr, __redirect_strrchr)
-  __attribute__((visibility ("hidden")));
+  __attribute__((visibility ("hidden"))) __attribute_copy__ (strrchr);
 # endif
 #endif
diff --git a/sysdeps/x86_64/multiarch/strspn.c b/sysdeps/x86_64/multiarch/strspn.c
index 56ab4d9..8b80bdc 100644
--- a/sysdeps/x86_64/multiarch/strspn.c
+++ b/sysdeps/x86_64/multiarch/strspn.c
@@ -30,6 +30,6 @@  libc_ifunc_redirected (__redirect_strspn, strspn, IFUNC_SELECTOR ());
 
 # ifdef SHARED
 __hidden_ver1 (strspn, __GI_strspn, __redirect_strspn)
-  __attribute__ ((visibility ("hidden")));
+  __attribute__ ((visibility ("hidden")))  __attribute_copy__ (strspn);
 # endif
 #endif