Re: [PATCH 1/3] glibc: Perform rseq registration at C startup and thread creation (v21)

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- On Jun 24, 2020, at 10:20 AM, Florian Weimer fweimer@redhat.com wrote:

> * Mathieu Desnoyers:
> 
>> diff --git a/manual/threads.texi b/manual/threads.texi
>> index bb7a42c655..d5069d5581 100644
>> --- a/manual/threads.texi
>> +++ b/manual/threads.texi
> 
>> +@deftypevar {struct rseq} __rseq_abi
>> +@standards{Linux, sys/rseq.h}
>> +@Theglibc{} implements a @code{__rseq_abi} TLS symbol to interact with
>> +the Restartable Sequences system call.  The layout of this structure is
>> +defined by the @file{sys/rseq.h} header.  Registration of each thread's
>> +@code{__rseq_abi} is performed by @theglibc{} at library initialization
>> +and thread creation. The manual for the rseq system call can be found
>> +at
>> @uref{https://git.kernel.org/pub/scm/libs/librseq/librseq.git/tree/doc/man/rseq.2}.
> 
> Should be “creation.  The” (two spaces after a sentence-ending period).

OK

> 
>> diff --git a/sysdeps/unix/sysv/linux/sys/rseq.h
>> b/sysdeps/unix/sysv/linux/sys/rseq.h
>> new file mode 100644
>> index 0000000000..5e118c1781
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/sys/rseq.h
> 
>> +#ifdef __cplusplus
>> +# if  __cplusplus >= 201103L
>> +#  define __rseq_static_assert(expr, diagnostic) static_assert (expr,
>> diagnostic)
>> +#  define __rseq_alignof(type)                   alignof (type)
>> +#  define __rseq_tls_storage_class               thread_local
>> +# endif
>> +#elif (defined __STDC_VERSION__ ? __STDC_VERSION__ : 0) >= 201112L
>> +# define __rseq_static_assert(expr, diagnostic)  _Static_assert (expr,
>> diagnostic)
>> +# define __rseq_alignof(type)                    _Alignof (type)
>> +# define __rseq_tls_storage_class                _Thread_local
>> +#endif
>> +
>> +#ifndef __rseq_static_assert
>> +/* Try to use _Static_assert macro from sys/cdefs.h.  */
>> +# ifdef _Static_assert
>> +#  define __rseq_static_assert(expr, diagnostic) _Static_assert (expr,
>> diagnostic)
>> +# else
>> +#  define __rseq_static_assert(expr, diagnostic) /* Nothing.  */
>> +# endif
>> +#endif
>> +
>> +/* Rely on GNU extensions for older standards and tls model.  */
>> +#ifdef __GNUC__
>> +# ifndef __rseq_alignof
>> +#  define __rseq_alignof(x) __alignof__ (x)
>> +# endif
>> +# define __rseq_tls_model_ie __attribute__ ((__tls_model__ ("initial-exec")))
>> +#else
>> +/* Specifying the TLS model on the declaration is optional.  */
>> +# define __rseq_tls_model_ie /* Nothing.  */
>> +#endif
> 
> I'm still worried that __rseq_static_assert and __rseq_alignof will show
> up in the UAPI with textually different definitions.  (This does not
> apply to __rseq_tls_model_ie.)

What makes this worry not apply to __rseq_tls_model_ie ?

> 
> Is my worry unfounded?

So AFAIU you worry that eventually sys/rseq.h and linux/rseq.h carry different
definitions of __rseq_static_assert and __rseq_alignof.

Indeed, I did not surround those #define with #ifndef/#endif. Maybe we should ?

Just in case the definitions end up being different (worse case scenario), we
should expect their behavior to be pretty much equivalent. So going for the
following should address your concern I think:

#ifdef __cplusplus
# if  __cplusplus >= 201103L
#  ifndef __rseq_static_assert
#   define __rseq_static_assert(expr, diagnostic) static_assert (expr, diagnostic)
#  endif
#  ifndef __rseq_alignof
#   define __rseq_alignof(type)                   alignof (type)
#  endif
#  ifndef __rseq_tls_storage_class
#   define __rseq_tls_storage_class               thread_local
#  endif
# endif
#elif (defined __STDC_VERSION__ ? __STDC_VERSION__ : 0) >= 201112L
# ifndef __rseq_static_assert
#  define __rseq_static_assert(expr, diagnostic)  _Static_assert (expr, diagnostic)
# endif
# ifndef __rseq_alignof
#  define __rseq_alignof(type)                    _Alignof (type)
# endif
# ifndef __rseq_tls_storage_class
#  define __rseq_tls_storage_class                _Thread_local
# endif
#endif

#ifndef __rseq_static_assert
/* Try to use _Static_assert macro from sys/cdefs.h.  */
# ifdef _Static_assert
#  define __rseq_static_assert(expr, diagnostic) _Static_assert (expr, diagnostic)
# else
#  define __rseq_static_assert(expr, diagnostic) /* Nothing.  */
# endif
#endif

/* Rely on GNU extensions for older standards and tls model.  */
#ifdef __GNUC__
# ifndef __rseq_alignof
#  define __rseq_alignof(x) __alignof__ (x)
# endif
# ifndef __rseq_tls_model_ie
#  define __rseq_tls_model_ie __attribute__ ((__tls_model__ ("initial-exec")))
# endif
#else
/* Specifying the TLS model on the declaration is optional.  */
# ifndef __rseq_tls_model_ie
#  define __rseq_tls_model_ie /* Nothing.  */
# endif
#endif

/* Fall back to __thread for TLS storage class.  */
#ifndef __rseq_tls_storage_class
# define __rseq_tls_storage_class __thread
#endif

Thoughts ?

Thanks,

Mathieu



> 
> Thanks,
> Florian

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com