Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sat, 6 Sep 2014 20:21:23 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH 1/9] interface additions for the C thread
 implementation

On Mon, Sep 01, 2014 at 12:45:47AM +0200, Jens Gustedt wrote:
> This adds all the constant, type and function interfaces.

Including some comments here as I work on committing it. Please don't
take them as critical; just explaining choices made during commit and
noting where things are different in the version to be committed.

> It makes pthread_mutex_t, mtx_t, pthread_cond_t and cnd_t different
> types.
> 
> This only works because
> 
>  - under hood the corresponding pairs of types use exactly the same
>    definition for the type
> 
>  - the types are a struct types without tag name
> 
>  - no comparison or assignment is allowed for any of these types. For
>    the POSIX types this interdiction is written in the standard. For the
>    C thread types, this is an extension that this implementation
>    imposes, but which might be integrated in a later version of the C
>    standard.
> 
>  - initialization is default initialization of an array of int. For the
>    POSIX types, initialization expressions are provided. For C thread
>    types the only initialization foreseen by the standard are the init
>    functions.
> 
>  - any calls to standard functions use pointers, and because pointer
>    representations for struct types are the same.
> 
> For the C++ API/ABI, these also are different types, now, with type names
> (that are used for name mangling, e.g) as listed above.
> 
> Somebody better versed in C++ could perhaps contribute code that
> overloads the comparison and assignment operators such that a compilation
> that tries to compare or copy these types fails.

I'm not sure what you meant by this last paragraph.

> ---
>  arch/arm/bits/alltypes.h.in        |   10 +++-
>  arch/i386/bits/alltypes.h.in       |   10 +++-
>  arch/microblaze/bits/alltypes.h.in |   10 +++-
>  arch/mips/bits/alltypes.h.in       |   10 +++-
>  arch/or1k/bits/alltypes.h.in       |   10 +++-
>  arch/powerpc/bits/alltypes.h.in    |   10 +++-
>  arch/sh/bits/alltypes.h.in         |   10 +++-
>  arch/x32/bits/alltypes.h.in        |   10 +++-
>  arch/x86_64/bits/alltypes.h.in     |   10 +++-
>  include/alltypes.h.in              |   10 ++++
>  include/threads.h                  |  110 ++++++++++++++++++++++++++++++++++++
>  include/time.h                     |   11 ++++
>  12 files changed, 212 insertions(+), 9 deletions(-)
>  create mode 100644 include/threads.h
> 
> diff --git a/arch/arm/bits/alltypes.h.in b/arch/arm/bits/alltypes.h.in
> index 183c4c4..1dcb920 100644
> --- a/arch/arm/bits/alltypes.h.in
> +++ b/arch/arm/bits/alltypes.h.in
> @@ -18,8 +18,16 @@ TYPEDEF struct { long long __ll; long double __ld; } max_align_t;
>  TYPEDEF long time_t;
>  TYPEDEF long suseconds_t;
>  
> -TYPEDEF struct { union { int __i[9]; unsigned __s[9]; } __u; } pthread_attr_t;
> +/* The pairs of equivalent definitions for pthread and C thread types
> + * should always be kept in sync.
> + *
> + * Also this only works because the underlying struct has no struct
> + * tag. Don't introduce one. */

No need for comments like these; introducing a tag would be an ABI
change anyway, so it can't be done.

> diff --git a/include/alltypes.h.in b/include/alltypes.h.in
> index c4ca5d5..94364e8 100644
> --- a/include/alltypes.h.in
> +++ b/include/alltypes.h.in
> @@ -43,13 +43,23 @@ TYPEDEF unsigned gid_t;
>  TYPEDEF int key_t;
>  TYPEDEF unsigned useconds_t;
>  
> +/* The pairs of equivalent definitions for pthread and C thread types
> + * should always be kept in sync. */
>  #ifdef __cplusplus
>  TYPEDEF unsigned long pthread_t;
> +TYPEDEF unsigned long thrd_t;
>  #else
>  TYPEDEF struct __pthread * pthread_t;
> +TYPEDEF struct __pthread * thrd_t;
>  #endif
>  TYPEDEF int pthread_once_t;
> +TYPEDEF int once_flag;
>  TYPEDEF unsigned pthread_key_t;
> +TYPEDEF unsigned tss_t;

The threads.h types don't need to be in alltypes.h since they're only
exposed by one file. (The pthread types are there because sys/types.h
exposes them too, in addition to pthread.h.)

> +TYPEDEF pthread_cond_t cnd_t;
> +TYPEDEF pthread_mutex_t mtx_t;

These two lines were left over and seem to have been nops due to the
order of concatenation.

> diff --git a/include/threads.h b/include/threads.h
> new file mode 100644
> index 0000000..0e99443
> --- /dev/null
> +++ b/include/threads.h
> @@ -0,0 +1,110 @@
> +#ifndef _THREADS_H
> +#define _THREADS_H
> +
> +/* This one is explicitly allowed to be included. */
> +#include <time.h>

I'm also adding features.h here because musl needs it internally for
any header that wants to use restrict (we use __restrict because gcc
disables restrict by default in its ugly gnu89 mode) and _Noreturn.

> +#ifdef __cplusplus
> +extern "C" {
> +#endif

This appeared twice with only one closing "}". I'm merging all the C++
logic into one conditional.

> +
> +/* These may or may not be implemented to be the same as some POSIX
> + * types. Don't rely on any assumption about that, in particular if
> + * you happen to use both interfaces in the same code. */
> +

This text (or similar) should eventually make it into the
documentation, but headers don't carry documentation like this in
musl.

> +int cnd_timedwait(cnd_t *restrict, mtx_t *restrict, const struct timespec *restrict);
> +int cnd_wait(cnd_t *, mtx_t *);

The lack of restrict on cnd_wait is really odd, but I checked and it's
like that in the standard too... File this as another bug for C11...
:-)

Rich

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.