[PATCH] headers: avoid bareword attributes

[PATCH] headers: avoid bareword attributes

[Eric Blake]

Always use the __-decorated form of an attribute name in public
headers, as the bareword form is in the user's namespace, and we
don't want compilation to break just because the user defines the
bareword to mean something else.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 newlib/libc/include/sys/cdefs.h      | 28 ++++++++++++++--------------
 newlib/libc/sys/phoenix/sys/unistd.h |  2 +-
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/newlib/libc/include/sys/cdefs.h b/newlib/libc/include/sys/cdefs.h
index 2e63a07a2..9e58ee980 100644
--- a/newlib/libc/include/sys/cdefs.h
+++ b/newlib/libc/include/sys/cdefs.h
@@ -681,42 +681,42 @@
 #endif

 /* Structure implements a lock. */
-#define	__lockable		__lock_annotate(lockable)
+#define	__lockable		__lock_annotate(__lockable__)

 /* Function acquires an exclusive or shared lock. */
 #define	__locks_exclusive(...) \
-	__lock_annotate(exclusive_lock_function(__VA_ARGS__))
+	__lock_annotate(__exclusive_lock_function__(__VA_ARGS__))
 #define	__locks_shared(...) \
-	__lock_annotate(shared_lock_function(__VA_ARGS__))
+	__lock_annotate(__shared_lock_function__(__VA_ARGS__))

 /* Function attempts to acquire an exclusive or shared lock. */
 #define	__trylocks_exclusive(...) \
-	__lock_annotate(exclusive_trylock_function(__VA_ARGS__))
+	__lock_annotate(__exclusive_trylock_function__(__VA_ARGS__))
 #define	__trylocks_shared(...) \
-	__lock_annotate(shared_trylock_function(__VA_ARGS__))
+	__lock_annotate(__shared_trylock_function__(__VA_ARGS__))

 /* Function releases a lock. */
-#define	__unlocks(...)		__lock_annotate(unlock_function(__VA_ARGS__))
+#define	__unlocks(...)		__lock_annotate(__unlock_function__(__VA_ARGS__))

 /* Function asserts that an exclusive or shared lock is held. */
 #define	__asserts_exclusive(...) \
-	__lock_annotate(assert_exclusive_lock(__VA_ARGS__))
+	__lock_annotate(__assert_exclusive_lock__(__VA_ARGS__))
 #define	__asserts_shared(...) \
-	__lock_annotate(assert_shared_lock(__VA_ARGS__))
+	__lock_annotate(__assert_shared_lock__(__VA_ARGS__))

 /* Function requires that an exclusive or shared lock is or is not held. */
 #define	__requires_exclusive(...) \
-	__lock_annotate(exclusive_locks_required(__VA_ARGS__))
+	__lock_annotate(__exclusive_locks_required__(__VA_ARGS__))
 #define	__requires_shared(...) \
-	__lock_annotate(shared_locks_required(__VA_ARGS__))
+	__lock_annotate(__shared_locks_required__(__VA_ARGS__))
 #define	__requires_unlocked(...) \
-	__lock_annotate(locks_excluded(__VA_ARGS__))
+	__lock_annotate(__locks_excluded__(__VA_ARGS__))

 /* Function should not be analyzed. */
-#define	__no_lock_analysis	__lock_annotate(no_thread_safety_analysis)
+#define	__no_lock_analysis	__lock_annotate(__no_thread_safety_analysis__)

 /* Guard variables and structure members by lock. */
-#define	__guarded_by(x)		__lock_annotate(guarded_by(x))
-#define	__pt_guarded_by(x)	__lock_annotate(pt_guarded_by(x))
+#define	__guarded_by(x)		__lock_annotate(__guarded_by__(x))
+#define	__pt_guarded_by(x)	__lock_annotate(__pt_guarded_by__(x))

 #endif /* !_SYS_CDEFS_H_ */
diff --git a/newlib/libc/sys/phoenix/sys/unistd.h b/newlib/libc/sys/phoenix/sys/unistd.h
index bdf5bd529..77b96ddd4 100644
--- a/newlib/libc/sys/phoenix/sys/unistd.h
+++ b/newlib/libc/sys/phoenix/sys/unistd.h
@@ -35,7 +35,7 @@

 extern char **environ;

-void _exit(int status) _ATTRIBUTE ((noreturn));
+void _exit(int status) _ATTRIBUTE ((__noreturn__));

 int	access(const char *pathname, int mode);
 unsigned int alarm(unsigned seconds);
-- 
2.13.5

Re: [PATCH] headers: avoid bareword attributes

[Corinna Vinschen]

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

On Aug 16 20:50, Eric Blake wrote:
> Always use the __-decorated form of an attribute name in public
> headers, as the bareword form is in the user's namespace, and we
> don't want compilation to break just because the user defines the
> bareword to mean something else.

Looks right to me.  Please push, inclkuding the Cygwin patch you
sent to cygwin-patches.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] headers: avoid bareword attributes

[Sebastian Huber]

On 17/08/17 03:50, Eric Blake wrote:

> Always use the __-decorated form of an attribute name in public
> headers, as the bareword form is in the user's namespace, and we
> don't want compilation to break just because the user defines the
> bareword to mean something else.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

Can I try to integrate the <sys/cdefs.h> part of your patch in FreeBSD?

-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber@embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

Re: [PATCH] headers: avoid bareword attributes

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 741 bytes --]

On 08/22/2017 10:55 AM, Sebastian Huber wrote:
> On 17/08/17 03:50, Eric Blake wrote:
> 
>> Always use the __-decorated form of an attribute name in public
>> headers, as the bareword form is in the user's namespace, and we
>> don't want compilation to break just because the user defines the
>> bareword to mean something else.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Can I try to integrate the <sys/cdefs.h> part of your patch in FreeBSD?

You don't have to ask my permission (thanks to open source licensing
rules) - but if it makes you feel better, yes, I'm fine with that.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [PATCH] headers: avoid bareword attributes

[Sebastian Huber]

Hello Eric,

On 17/08/17 03:50, Eric Blake wrote:
> Always use the __-decorated form of an attribute name in public
> headers, as the bareword form is in the user's namespace, and we
> don't want compilation to break just because the user defines the
> bareword to mean something else.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   newlib/libc/include/sys/cdefs.h      | 28 ++++++++++++++--------------
>   newlib/libc/sys/phoenix/sys/unistd.h |  2 +-
>   2 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/newlib/libc/include/sys/cdefs.h b/newlib/libc/include/sys/cdefs.h
> index 2e63a07a2..9e58ee980 100644
> --- a/newlib/libc/include/sys/cdefs.h
> +++ b/newlib/libc/include/sys/cdefs.h
> @@ -681,42 +681,42 @@
>   #endif
>
>   /* Structure implements a lock. */
> -#define	__lockable		__lock_annotate(lockable)
> +#define	__lockable		__lock_annotate(__lockable__)
>
>   /* Function acquires an exclusive or shared lock. */
>   #define	__locks_exclusive(...) \
> -	__lock_annotate(exclusive_lock_function(__VA_ARGS__))
> +	__lock_annotate(__exclusive_lock_function__(__VA_ARGS__))
>   #define	__locks_shared(...) \
> -	__lock_annotate(shared_lock_function(__VA_ARGS__))
> +	__lock_annotate(__shared_lock_function__(__VA_ARGS__))
>
>   /* Function attempts to acquire an exclusive or shared lock. */
>   #define	__trylocks_exclusive(...) \
> -	__lock_annotate(exclusive_trylock_function(__VA_ARGS__))
> +	__lock_annotate(__exclusive_trylock_function__(__VA_ARGS__))
>   #define	__trylocks_shared(...) \
> -	__lock_annotate(shared_trylock_function(__VA_ARGS__))
> +	__lock_annotate(__shared_trylock_function__(__VA_ARGS__))
>
>   /* Function releases a lock. */
> -#define	__unlocks(...)		__lock_annotate(unlock_function(__VA_ARGS__))
> +#define	__unlocks(...)		__lock_annotate(__unlock_function__(__VA_ARGS__))
>
>   /* Function asserts that an exclusive or shared lock is held. */
>   #define	__asserts_exclusive(...) \
> -	__lock_annotate(assert_exclusive_lock(__VA_ARGS__))
> +	__lock_annotate(__assert_exclusive_lock__(__VA_ARGS__))
>   #define	__asserts_shared(...) \
> -	__lock_annotate(assert_shared_lock(__VA_ARGS__))
> +	__lock_annotate(__assert_shared_lock__(__VA_ARGS__))
>
>   /* Function requires that an exclusive or shared lock is or is not held. */
>   #define	__requires_exclusive(...) \
> -	__lock_annotate(exclusive_locks_required(__VA_ARGS__))
> +	__lock_annotate(__exclusive_locks_required__(__VA_ARGS__))
>   #define	__requires_shared(...) \
> -	__lock_annotate(shared_locks_required(__VA_ARGS__))
> +	__lock_annotate(__shared_locks_required__(__VA_ARGS__))
>   #define	__requires_unlocked(...) \
> -	__lock_annotate(locks_excluded(__VA_ARGS__))
> +	__lock_annotate(__locks_excluded__(__VA_ARGS__))
>
>   /* Function should not be analyzed. */
> -#define	__no_lock_analysis	__lock_annotate(no_thread_safety_analysis)
> +#define	__no_lock_analysis	__lock_annotate(__no_thread_safety_analysis__)
>
>   /* Guard variables and structure members by lock. */
> -#define	__guarded_by(x)		__lock_annotate(guarded_by(x))
> -#define	__pt_guarded_by(x)	__lock_annotate(pt_guarded_by(x))
> +#define	__guarded_by(x)		__lock_annotate(__guarded_by__(x))
> +#define	__pt_guarded_by(x)	__lock_annotate(__pt_guarded_by__(x))
>
>   #endif /* !_SYS_CDEFS_H_ */

did you test the clang thread safety analysis after this patch? It not 
longer works using clang 3.8. I get now errors like this:

error: use of undeclared identifier '__mutex'
                     __locks_exclusive(*__mutex);

If I revert this change it works.

-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber@embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

Re: [PATCH] headers: avoid bareword attributes

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 3375 bytes --]

On 08/24/2017 12:44 AM, Sebastian Huber wrote:> Hello Eric,
>
> On 17/08/17 03:50, Eric Blake wrote:
>> Always use the __-decorated form of an attribute name in public
>> headers, as the bareword form is in the user's namespace, and we
>> don't want compilation to break just because the user defines the
>> bareword to mean something else.
>>
>
> did you test the clang thread safety analysis after this patch? It not
> longer works using clang 3.8. I get now errors like this:
>

No, I usually compile with gcc, not clang.

> error: use of undeclared identifier '__mutex'
>                     __locks_exclusive(*__mutex);

Can you figure out what the preprocessor is producing at the point where
clang is complaining?

  __locks_exclusive(*__mutex)
should expand to
  __lock_annotate(__exclusive_lock_function__(*__mutex))
where that expansion then depends on:

#if __has_extension(c_thread_safety_attributes)
#define __lock_annotate(x)      __attribute__((x))
#else
#define __lock_annotate(x)
#endif

Hmm, it looks like gcc lacks c_thread_safety_attributes, which means
compile-testing under gcc didn't really test anything, since under that
compiler, the overall expansion is empty.  But if I'm right, then under
clang, the expansion would be:

__attribute__((__exclusive_lock_function__(*__mutex)))

If that is not valid syntax under clang, then I would argue that the bug
is in clang for providing a bareword attribute with no __-decorated
counterpart (particularly true since clang borrowed the __attribute__
syntax from gcc, and gcc long ago made it a point that bareword forms
always have a decorated counterpart, precisely for the use case of
public headers that must be immune to macros in the user's namespace).

A google search found
https://clang.llvm.org/docs/ThreadSafetyAnalysis.html as documentation
of some of the attributes supported; which recommends:

// Enable thread safety attributes only with clang.
// The attributes can be safely erased when compiling with other compilers.
#if defined(__clang__) && (!defined(SWIG))
#define THREAD_ANNOTATION_ATTRIBUTE__(x)   __attribute__((x))
#else
#define THREAD_ANNOTATION_ATTRIBUTE__(x)   // no-op
#endif

#define CAPABILITY(x) \
  THREAD_ANNOTATION_ATTRIBUTE__(capability(x))
...

#ifdef USE_LOCK_STYLE_THREAD_SAFETY_ATTRIBUTES
// The original version of thread safety analysis the following attribute
// definitions.  These use a lock-based terminology.  They are still in use
// by existing thread safety code, and will continue to be supported.

...
// Replaced by ACQUIRE
#define EXCLUSIVE_LOCK_FUNCTION(...) \
  THREAD_ANNOTATION_ATTRIBUTE__(exclusive_lock_function(__VA_ARGS__))

Uggh - their example uses the bareword form, which does NOT make it
obvious whether the decorated form should work.  It also states that we
are using the older spelling form, and should consider migrating the
newlib code base to the newer ACQUIRE macro form.

>
> If I revert this change it works.

I still think this is more likely to be a bug in clang, but I'm also
okay if we need to revert this part of the patch since it only affects
attributes that only clang uses, and since I don't test on clang.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [PATCH] headers: avoid bareword attributes

[Sebastian Huber]

On 24/08/17 21:02, Eric Blake wrote:

> On 08/24/2017 12:44 AM, Sebastian Huber wrote:> Hello Eric,
>> On 17/08/17 03:50, Eric Blake wrote:
>>> Always use the __-decorated form of an attribute name in public
>>> headers, as the bareword form is in the user's namespace, and we
>>> don't want compilation to break just because the user defines the
>>> bareword to mean something else.
>>>
>> did you test the clang thread safety analysis after this patch? It not
>> longer works using clang 3.8. I get now errors like this:
>>
> No, I usually compile with gcc, not clang.

The thread safety analysis development for GCC has been stopped in 2012:

https://gcc.gnu.org/ml/gcc/2012-04/msg00714.html

[...]
>> If I revert this change it works.
> I still think this is more likely to be a bug in clang, but I'm also
> okay if we need to revert this part of the patch since it only affects
> attributes that only clang uses, and since I don't test on clang.
>

I also think that this is a clang bug and I will file a bug report, 
however, it will take time to fix this and we should not break existing 
clang support for now.

With this test program

void f0(void) __attribute__((noreturn));
void f1(void) __attribute__((__noreturn__));
void f2(int *l) __attribute__((exclusive_lock_function(*l)));
void f3(int *l) __attribute__((__exclusive_lock_function__(*l)));

I get

clang -c test.c
test.c:4:61: error: use of undeclared identifier 'l'
void f3(int *l) __attribute__((__exclusive_lock_function__(*l)));
                                                             ^
1 error generated.

-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber@embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

Re: [PATCH] headers: avoid bareword attributes

[Sebastian Huber]

On 25/08/17 09:53, Sebastian Huber wrote:
[...]
> I also think that this is a clang bug and I will file a bug report, 
> however, it will take time to fix this and we should not break 
> existing clang support for now.
>
> With this test program
>
> void f0(void) __attribute__((noreturn));
> void f1(void) __attribute__((__noreturn__));
> void f2(int *l) __attribute__((exclusive_lock_function(*l)));
> void f3(int *l) __attribute__((__exclusive_lock_function__(*l)));
>
> I get
>
> clang -c test.c
> test.c:4:61: error: use of undeclared identifier 'l'
> void f3(int *l) __attribute__((__exclusive_lock_function__(*l)));
>                                                             ^
> 1 error generated.
>

https://bugs.llvm.org/show_bug.cgi?id=34319

-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber@embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.