Re: [PATCH] headers: avoid bareword attributes

[Eric Blake <eblake at redhat dot com>]

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: signature.asc
Description: OpenPGP digital signature