Message-ID: <1701081361.34159.1590503556923.JavaMail.zimbra@efficios.com>
Date:   Tue, 26 May 2020 10:32:36 -0400 (EDT)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     Florian Weimer <fweimer@...hat.com>
Cc:     libc-alpha <libc-alpha@...rceware.org>,
        Rich Felker <dalias@...c.org>,
        linux-api <linux-api@...r.kernel.org>,
        Boqun Feng <boqun.feng@...il.com>,
        Will Deacon <will.deacon@....com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ben Maurer <bmaurer@...com>, Dave Watson <davejwatson@...com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Paul <paulmck@...ux.vnet.ibm.com>, Paul Turner <pjt@...gle.com>,
        Joseph Myers <joseph@...esourcery.com>
Subject: Re: [PATCH glibc 1/3] glibc: Perform rseq registration at C startup
 and thread creation (v19)

----- On May 26, 2020, at 8:41 AM, Florian Weimer fweimer@...hat.com wrote:

> * Mathieu Desnoyers:
> 
>> Something like this ?
>>
>> #ifdef __cplusplus
>> # if  __cplusplus >= 201103L
>> #  define rseq_static_assert (expr, diagnostic)         static_assert (expr,
>> diagnostic)
>> #  define rseq_alignof                                  alignof
>> # endif
>> #elif __STDC_VERSION__ >= 201112L
>> # define rseq_static_assert (expr, diagnostic)          _Static_assert (expr,
>> diagnostic)
>> # define rseq_alignof                                   _Alignof
>> #endif
>>
>> #ifndef rseq_static_assert
>> # define rseq_static_assert (expr, diagnostic)          /* nothing */
>> #endif
> 
> You can't have a space in #defines like that, no matter what GNU style
> says. 8-)

Yes, I noticed when failing to build this ;)

> 
>> /* Ensure the compiler supports __attribute__ ((aligned)).  */
>> rseq_static_assert ((rseq_alignof (struct rseq_cs) >= 32, "alignment"));
>> rseq_static_assert ((rseq_alignof (struct rseq) >= 32, "alignment"));
> 
> You need to move the ; into rseq_static_assert.  And if you use explicit
> arguments, you can't use double parentheses.

Why move the ";" into the macro ?

AFAIU, the only gain here would be to make sure we don't emit useless
";" in the "/* nothing */" case. But does it matter ?

Examples I can find of "static_assert" explicitly have the ";" at the
end, so I find it weird to integrate it into the rseq_static_assert
macro, which makes it different from static_assert.

Agreed on the need to remove the double-parentheses.

> 
>>> And something similar for _Alignas/attribute aligned,
>>
>> I don't see where _Alignas is needed here ?
>>
>> For attribute aligned, what would be the oldest supported C and C++
>> standards ?
> 
> There are no standardized attributes for C, there is only _Alignas.
> C++11 has an alignas specifier; it's not an attribute either.  I think
> these are syntactically similar.

There appears to be an interesting difference between attribute aligned
and alignas. It seems like alignas cannot be used on a structure declaration,
only on fields, e.g.:

struct blah {
        int a;
} _Alignas (16);

o.c:3:1: warning: useless ‘_Alignas’ in empty declaration
 } _Alignas (16);

But

struct blah {
        int _Alignas (16) a;
};

is OK. So if I change e.g. struct rseq_cs to align
the first field:

struct rseq_cs
  {
    /* Version of this structure.  */
    uint32_t rseq_align (32) version;
    /* enum rseq_cs_flags.  */
    uint32_t flags;
    uint64_t start_ip;
    /* Offset from start_ip.  */
    uint64_t post_commit_offset;
    uint64_t abort_ip;
  };

It should work.

> 
>>> with an error for
>>> older standards and !__GNUC__ compilers (because neither the type nor
>>> __thread can be represented there).
>>
>> By "type" you mean "struct rseq" here ? What does it contain that requires
>> a __GNUC__ compiler ?
> 
> __attribute__ and __thread support.

OK

> 
>> About __thread, I recall other compilers have other means to declare it.
>> In liburcu, I end up with the following:
>>
>> #if defined (__cplusplus) && (__cplusplus >= 201103L)
>> # define URCU_TLS_STORAGE_CLASS thread_local
>> #elif defined (__STDC_VERSION__) && (__STDC_VERSION__ >= 201112L)
>> # define URCU_TLS_STORAGE_CLASS _Thread_local
>> #elif defined (_MSC_VER)
>> # define URCU_TLS_STORAGE_CLASS __declspec(thread)
>> #else
>> # define URCU_TLS_STORAGE_CLASS __thread
>> #endif
>>
>> Would something along those lines be OK for libc ?
> 
> Yes, it would be okay (minus the Visual C++ part).  This part does not
> have to go into UAPI headers first.  A fallback definition of __thread
> should be okay.  Outside glibc, the TLS model declaration is optional, I
> think.  The glibc *definition* ensures that the variable is
> initial-exec.

AFAIU you are technically correct when stating that the tls model
on the declaration is optional, but I think it's a good thing to have
it there rather than only at the definition. It makes it clear to all
users of this variable that its model is IE. Especially in scenarios where
early-adopter libraries and applications can define their own __rseq_abi
symbol, I think it's good to explicitly keep the IE tls model attribute in
the header.

I end up with the following:

#ifdef __cplusplus
# if  __cplusplus >= 201103L
#  define rseq_static_assert(expr, diagnostic) static_assert (expr, diagnostic)
#  define rseq_alignof(type)                   alignof (type)
#  define rseq_alignas(x)                      alignas (x)
#  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_alignas(x)                       _Alignas (x)
# 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
# ifndef rseq_alignas
#  define rseq_alignas(x) __attribute__ ((aligned (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

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

[...]

/* Ensure the compiler supports rseq_align.  */
rseq_static_assert (rseq_alignof (struct rseq_cs) >= 32, "alignment");
rseq_static_assert (rseq_alignof (struct rseq) >= 32, "alignment");

/* Allocations of struct rseq and struct rseq_cs on the heap need to
   be aligned on 32 bytes.  Therefore, use of malloc is discouraged
   because it does not guarantee alignment.  posix_memalign should be
   used instead.  */

extern rseq_tls_storage_class struct rseq __rseq_abi rseq_tls_model_ie;

Thanks,

Mathieu

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

Powered by blists - more mailing lists