Re: Usage of __attribute__used__ in (system) headers?

Re: Usage of __attribute__used__ in (system) headers?

[Carlos O'Donell]

On Fri, Feb 15, 2013 at 4:54 AM, Thomas Schwinge
<thomas@codesourcery.com> wrote:
> Hi!
>
> I found that sysdeps/ieee754/bits/nan.h is different from
> ports/sysdeps/mips/bits/nan.h in that the former has seen the following
> change, whereas the latter has not been updated accordingly:

Good catch!

Adding libc-ports since my suggestion is the opposite of yours.

> | commit b575c52b86fe0c00adec925e356eb72cf95b23a7
> | Author: Ulrich Drepper <drepper@redhat.com>
> | Date:   Fri Apr 16 22:04:55 2004 +0000
> |
> |     Update.
> |
> |     2004-04-16  Ulrich Drepper  <drepper@redhat.com>
> |
> |       * sysdeps/ieee754/bits/nan.h (__nan_union): Add __attribute_used__
> |       attribute to keep gcc quiet.
> |
> | [...]
> | --- sysdeps/ieee754/bits/nan.h
> | +++ sysdeps/ieee754/bits/nan.h
> | @@ -46,7 +46,8 @@
> |  #  define __nan_bytes                { 0, 0, 0xc0, 0x7f }
> |  # endif
> |
> | -static union { unsigned char __c[4]; float __d; } __nan_union = { __nan_bytes };
> | +static union { unsigned char __c[4]; float __d; } __nan_union
> | +    __attribute_used__ = { __nan_bytes };
> |  # define NAN (__nan_union.__d)
> |
> |  #endif       /* GCC.  */
>
> This code in question (cited/changed above) is only ever used for
> non-__GNUC__-defining compilers.  If my reading of misc/sys/cdefs.h is
> correct, for these compilers, the attribute declaration is not applied
> anyway.  So, instead of adding it to the MIPS copy of that file, I
> suggest we remove it from the generic one.  OK?
>
> diff --git sysdeps/ieee754/bits/nan.h sysdeps/ieee754/bits/nan.h
> index d3ab38b..00ad0b5 100644
> --- sysdeps/ieee754/bits/nan.h
> +++ sysdeps/ieee754/bits/nan.h
> @@ -46,7 +46,7 @@
>  # endif
>
>  static union { unsigned char __c[4]; float __d; } __nan_union
> -    __attribute_used__ = { __nan_bytes };
> +  = { __nan_bytes };
>  # define NAN   (__nan_union.__d)
>
>  #endif /* GCC.  */

I disagree, it's useful to mark the non-GCC version with
an __attribute_used__ such that in the future we might
use another compiler without problems.

I think you should update the MIPS nan.h version to
match the generic version, and on top of that you should
also apply the following untested patch to minimize the
diff between the versions.

diff --git a/ports/sysdeps/mips/bits/nan.h b/ports/sysdeps/mips/bits/nan.h
index ffbb3b5..a768dd7 100644
--- a/ports/sysdeps/mips/bits/nan.h
+++ b/ports/sysdeps/mips/bits/nan.h
@@ -13,7 +13,7 @@
    Lesser General Public License for more details.

    You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library.  If not, see
+   License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */

 #ifndef _MATH_H
@@ -21,20 +21,21 @@
 #endif


-/* IEEE Not A Number (QNaN). Note that MIPS has the QNaN and SNaN patterns
+/* IEEE Not A Number.  */
+/* By default a NaN is a QNaN. Note that MIPS has the QNaN and SNaN patterns
    reversed compared to most other architectures. The IEEE spec left
    the definition of this open to implementations, and for MIPS the top
    bit of the mantissa must be SET to indicate a SNaN.  */

 #if __GNUC_PREREQ(3,3)

-# define NAN	(__builtin_nanf(""))
+# define NAN	(__builtin_nanf (""))

 #elif defined __GNUC__

 # define NAN \
-  (__extension__                                                            \
-   ((union { unsigned __l __attribute__((__mode__(__SI__))); float __d; })  \
+  (__extension__							      \
+   ((union { unsigned __l __attribute__ ((__mode__ (__SI__))); float __d; })  \
     { __l: 0x7fbfffffUL }).__d)

 #else
---

Which then reduces the diff to this:
--- sysdeps/ieee754/bits/nan.h	2013-02-18 13:51:00.618157335 -0500
+++ ports/sysdeps/mips/bits/nan.h	2013-02-18 13:51:49.836908372 -0500
@@ -22,6 +22,10 @@


 /* IEEE Not A Number.  */
+/* A normal NaN is a QNaN. Note that MIPS has the QNaN and SNaN patterns
+   reversed compared to most other architectures. The IEEE spec left
+   the definition of this open to implementations, and for MIPS the top
+   bit of the mantissa must be SET to indicate a SNaN.  */

 #if __GNUC_PREREQ(3,3)

@@ -32,21 +36,20 @@
 # define NAN \
   (__extension__							      \
    ((union { unsigned __l __attribute__ ((__mode__ (__SI__))); float __d; })  \
-    { __l: 0x7fc00000UL }).__d)
+    { __l: 0x7fbfffffUL }).__d)

 #else

 # include <endian.h>

 # if __BYTE_ORDER == __BIG_ENDIAN
-#  define __nan_bytes		{ 0x7f, 0xc0, 0, 0 }
+#  define __nan_bytes		{ 0x7f, 0xbf, 0xff, 0xff }
 # endif
 # if __BYTE_ORDER == __LITTLE_ENDIAN
-#  define __nan_bytes		{ 0, 0, 0xc0, 0x7f }
+#  define __nan_bytes		{ 0xff, 0xff, 0xbf, 0x7f }
 # endif

-static union { unsigned char __c[4]; float __d; } __nan_union
-    __attribute_used__ = { __nan_bytes };
+static union { unsigned char __c[4]; float __d; } __nan_union = {
__nan_bytes };
 # define NAN	(__nan_union.__d)

 #endif	/* GCC.  */
---

Which shows clearly where the difference lies, and with your change
the diff would be even smaller.

Cheers,
Carlos.

Re: Usage of __attribute__used__ in (system) headers?

[Richard Henderson]

On 02/18/2013 11:00 AM, Carlos O'Donell wrote:
>> >  static union { unsigned char __c[4]; float __d; } __nan_union
>> > -    __attribute_used__ = { __nan_bytes };
>> > +  = { __nan_bytes };
>> >  # define NAN   (__nan_union.__d)
>> >
>> >  #endif /* GCC.  */
> I disagree, it's useful to mark the non-GCC version with
> an __attribute_used__ such that in the future we might
> use another compiler without problems.

The annotation should not be attribute used at all, but rather unused.

Used says that the object is referenced in some non-visible way by
assembly, and thus cannot be elided.  Unused says that we understand the
variable may not be referenced, and don't warn; but do, in particular,
remove it if it is unused.

As for whether or not to annotate for non-GCC... I'm ok with either.


r~

Re: Usage of __attribute__used__ in (system) headers?

[Carlos O'Donell]

On Mon, Feb 18, 2013 at 2:21 PM, Richard Henderson <rth@twiddle.net> wrote:
> On 02/18/2013 11:00 AM, Carlos O'Donell wrote:
>>> >  static union { unsigned char __c[4]; float __d; } __nan_union
>>> > -    __attribute_used__ = { __nan_bytes };
>>> > +  = { __nan_bytes };
>>> >  # define NAN   (__nan_union.__d)
>>> >
>>> >  #endif /* GCC.  */
>> I disagree, it's useful to mark the non-GCC version with
>> an __attribute_used__ such that in the future we might
>> use another compiler without problems.
>
> The annotation should not be attribute used at all, but rather unused.
>
> Used says that the object is referenced in some non-visible way by
> assembly, and thus cannot be elided.  Unused says that we understand the
> variable may not be referenced, and don't warn; but do, in particular,
> remove it if it is unused.

Yes, thanks for catching that, honestly I forget that it's a "possibly
unused" in that case.

glibc doesn't have an __attribute_unused__.

I'd be happy to review a patch that adds it and fixes up the 8
references to the bare attribute.

Cheers,
Carlos.

Re: Usage of __attribute__used__ in (system) headers?

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 5230 bytes --]

Hi!

On Mon, 18 Feb 2013 14:00:03 -0500, Carlos O'Donell <carlos@systemhalted.org> wrote:
> I think you should update the MIPS nan.h version to
> match the generic version [...]

Yes, I already had prepared such a patch and planned to commit it "as
obvious", without discussion.  Thusly pushed in commit
2636ffe65438af689e12b7977fe8609a6ca07c90:

ports/
	* sysdeps/mips/bits/nan.h: Align to generic IEEE 754 file.

diff --git ports/sysdeps/mips/bits/nan.h ports/sysdeps/mips/bits/nan.h
index ffbb3b5..af168ce 100644
--- ports/sysdeps/mips/bits/nan.h
+++ ports/sysdeps/mips/bits/nan.h
@@ -1,4 +1,4 @@
-/* `NAN' constant for IEEE 754 machines.
+/* `NAN' constant for IEEE 754 machines.  MIPS version.
    Copyright (C) 1992-2013 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
@@ -13,7 +13,7 @@
    Lesser General Public License for more details.
 
    You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library.  If not, see
+   License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
 #ifndef _MATH_H
@@ -21,20 +21,21 @@
 #endif
 
 
-/* IEEE Not A Number (QNaN). Note that MIPS has the QNaN and SNaN patterns
-   reversed compared to most other architectures. The IEEE spec left
-   the definition of this open to implementations, and for MIPS the top
-   bit of the mantissa must be SET to indicate a SNaN.  */
+/* IEEE Not A Number.  */
+/* Note that MIPS has the QNaN and SNaN patterns reversed compared to most
+   other architectures.  The IEEE spec left the definition of this open to
+   implementations, and for MIPS the top bit of the mantissa must be SET to
+   indicate a SNaN.  */
 
 #if __GNUC_PREREQ(3,3)
 
-# define NAN	(__builtin_nanf(""))
+# define NAN	(__builtin_nanf (""))
 
 #elif defined __GNUC__
 
 # define NAN \
-  (__extension__                                                            \
-   ((union { unsigned __l __attribute__((__mode__(__SI__))); float __d; })  \
+  (__extension__							      \
+   ((union { unsigned __l __attribute__ ((__mode__ (__SI__))); float __d; })  \
     { __l: 0x7fbfffffUL }).__d)
 
 #else

I also added
<http://sourceware.org/glibc/wiki/Development_Todo/Master?action=diff&rev2=49&rev1=48>.

On Mon, 18 Feb 2013 14:41:02 -0500, Carlos O'Donell <carlos@systemhalted.org> wrote:
> On Mon, Feb 18, 2013 at 2:21 PM, Richard Henderson <rth@twiddle.net> wrote:
> > On 02/18/2013 11:00 AM, Carlos O'Donell wrote:
> >>> >  static union { unsigned char __c[4]; float __d; } __nan_union
> >>> > -    __attribute_used__ = { __nan_bytes };
> >>> > +  = { __nan_bytes };
> >>> >  # define NAN   (__nan_union.__d)
> >>> >
> >>> >  #endif /* GCC.  */

In commit 72f0ffdcbeb8135d04cf2dc73f8a5f5c7783a283, I first add the
missing __attribute_used__:

ports/
	* sysdeps/mips/bits/nan.h [!__GNUC__] (__nan_union): Add
	__attribute_used__.

diff --git ports/sysdeps/mips/bits/nan.h ports/sysdeps/mips/bits/nan.h
index af168ce..71f372d 100644
--- ports/sysdeps/mips/bits/nan.h
+++ ports/sysdeps/mips/bits/nan.h
@@ -49,7 +49,8 @@
 #  define __nan_bytes		{ 0xff, 0xff, 0xbf, 0x7f }
 # endif
 
-static union { unsigned char __c[4]; float __d; } __nan_union = { __nan_bytes };
+static union { unsigned char __c[4]; float __d; } __nan_union
+    __attribute_used__ = { __nan_bytes };
 # define NAN	(__nan_union.__d)
 
 #endif	/* GCC.  */

> >> I disagree, it's useful to mark the non-GCC version with
> >> an __attribute_used__ such that in the future we might
> >> use another compiler without problems.
> >
> > The annotation should not be attribute used at all, but rather unused.

Good catch, thanks!

> glibc doesn't have an __attribute_unused__.

It doesn't need one; per <sys/cdefs.h>'s __attribute_used__ definition,
we assume that any compiler that supports __attribute__ at all  also
supports __attribute__ ((unused)).  So, I see no reason to introduce
__attribute_unused__ bit instead directly use __attribute__ ((unused));
commit c7b275d6b3bceb6b400fa3044d13d1001bc605ca:

	* sysdeps/ieee754/bits/nan.h [!__GNUC__] (__nan_union): Change
	__attribute_used__ to __attribute__ ((unused)).

ports/
	* sysdeps/mips/bits/nan.h [!__GNUC__] (__nan_union): Change
	__attribute_used__ to __attribute__ ((unused)).

diff --git ports/sysdeps/mips/bits/nan.h ports/sysdeps/mips/bits/nan.h
index 71f372d..8f4666d 100644
--- ports/sysdeps/mips/bits/nan.h
+++ ports/sysdeps/mips/bits/nan.h
@@ -50,7 +50,7 @@
 # endif
 
 static union { unsigned char __c[4]; float __d; } __nan_union
-    __attribute_used__ = { __nan_bytes };
+  __attribute__ ((unused)) = { __nan_bytes };
 # define NAN	(__nan_union.__d)
 
 #endif	/* GCC.  */
diff --git sysdeps/ieee754/bits/nan.h sysdeps/ieee754/bits/nan.h
index d3ab38b..a1e6a51 100644
--- sysdeps/ieee754/bits/nan.h
+++ sysdeps/ieee754/bits/nan.h
@@ -46,7 +46,7 @@
 # endif
 
 static union { unsigned char __c[4]; float __d; } __nan_union
-    __attribute_used__ = { __nan_bytes };
+  __attribute__ ((unused)) = { __nan_bytes };
 # define NAN	(__nan_union.__d)
 
 #endif	/* GCC.  */


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

Re: Usage of __attribute__used__ in (system) headers?

[Carlos O'Donell]

On 02/20/2013 11:06 AM, Thomas Schwinge wrote:
> ports/
> 	* sysdeps/mips/bits/nan.h [!__GNUC__] (__nan_union): Change
> 	__attribute_used__ to __attribute__ ((unused)).
> 
> diff --git ports/sysdeps/mips/bits/nan.h ports/sysdeps/mips/bits/nan.h
> index 71f372d..8f4666d 100644
> --- ports/sysdeps/mips/bits/nan.h
> +++ ports/sysdeps/mips/bits/nan.h
> @@ -50,7 +50,7 @@
>  # endif
>  
>  static union { unsigned char __c[4]; float __d; } __nan_union
> -    __attribute_used__ = { __nan_bytes };
> +  __attribute__ ((unused)) = { __nan_bytes };
>  # define NAN	(__nan_union.__d)
>  
>  #endif	/* GCC.  */
> diff --git sysdeps/ieee754/bits/nan.h sysdeps/ieee754/bits/nan.h
> index d3ab38b..a1e6a51 100644
> --- sysdeps/ieee754/bits/nan.h
> +++ sysdeps/ieee754/bits/nan.h
> @@ -46,7 +46,7 @@
>  # endif
>  
>  static union { unsigned char __c[4]; float __d; } __nan_union
> -    __attribute_used__ = { __nan_bytes };
> +  __attribute__ ((unused)) = { __nan_bytes };
>  # define NAN	(__nan_union.__d)
>  
>  #endif	/* GCC.  */

Please fix this to use __unused__.

We should not use attributes that are in the user's namespace.

Cheers,
Carlos.

Re: Usage of __attribute__used__ in (system) headers?

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 1551 bytes --]

Hi!

On Wed, 20 Feb 2013 11:25:22 -0500, Carlos O'Donell <carlos@redhat.com> wrote:
> >  static union { unsigned char __c[4]; float __d; } __nan_union
> > -    __attribute_used__ = { __nan_bytes };
> > +  __attribute__ ((unused)) = { __nan_bytes };

> Please fix this to use __unused__.
> 
> We should not use attributes that are in the user's namespace.

Pushed as commit 50022a93fcc36a0130b9f882709ff69f4b91c0cc:

	* sysdeps/ieee754/bits/nan.h [!__GNUC__] (__nan_union): Change
	__attribute__ ((unused)) to __attribute__ ((__unused__)).

ports/
	* sysdeps/mips/bits/nan.h [!__GNUC__] (__nan_union): Change
	__attribute__ ((unused)) to __attribute__ ((__unused__)).

diff --git ports/sysdeps/mips/bits/nan.h ports/sysdeps/mips/bits/nan.h
index 8f4666d..7aa157b 100644
--- ports/sysdeps/mips/bits/nan.h
+++ ports/sysdeps/mips/bits/nan.h
@@ -50,7 +50,7 @@
 # endif
 
 static union { unsigned char __c[4]; float __d; } __nan_union
-  __attribute__ ((unused)) = { __nan_bytes };
+  __attribute__ ((__unused__)) = { __nan_bytes };
 # define NAN	(__nan_union.__d)
 
 #endif	/* GCC.  */
diff --git sysdeps/ieee754/bits/nan.h sysdeps/ieee754/bits/nan.h
index a1e6a51..935271a 100644
--- sysdeps/ieee754/bits/nan.h
+++ sysdeps/ieee754/bits/nan.h
@@ -46,7 +46,7 @@
 # endif
 
 static union { unsigned char __c[4]; float __d; } __nan_union
-  __attribute__ ((unused)) = { __nan_bytes };
+  __attribute__ ((__unused__)) = { __nan_bytes };
 # define NAN	(__nan_union.__d)
 
 #endif	/* GCC.  */


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

Re: Usage of __attribute__used__ in (system) headers?

[Carlos O'Donell]

On 02/20/2013 12:56 PM, Thomas Schwinge wrote:
> Hi!
> 
> On Wed, 20 Feb 2013 11:25:22 -0500, Carlos O'Donell <carlos@redhat.com> wrote:
>>>  static union { unsigned char __c[4]; float __d; } __nan_union
>>> -    __attribute_used__ = { __nan_bytes };
>>> +  __attribute__ ((unused)) = { __nan_bytes };
> 
>> Please fix this to use __unused__.
>>
>> We should not use attributes that are in the user's namespace.
> 
> Pushed as commit 50022a93fcc36a0130b9f882709ff69f4b91c0cc:
> 
> 	* sysdeps/ieee754/bits/nan.h [!__GNUC__] (__nan_union): Change
> 	__attribute__ ((unused)) to __attribute__ ((__unused__)).
> 
> ports/
> 	* sysdeps/mips/bits/nan.h [!__GNUC__] (__nan_union): Change
> 	__attribute__ ((unused)) to __attribute__ ((__unused__)).

Thanks!

Cheers,
Carlos.