Include pthread only in the implementation

[edubart]

Would be nicer if pthread.h was only included when MINIAUDIO_IMPLEMENTATION is defined. Thus removing this line:

#include <pthread.h>    /* Unfortunate #include, but needed for pthread_t, pthread_mutex_t and pthread_cond_t types. */

Motivation

Currently when binding miniaudio in another language I also need to bind pthread structures on POSIX systems due to this, which is boilerplate and make miniaudio bindings platform dependent, thus I have to branch the bindings between different platforms.

These are the structures I need to bind with miniaudio on Linux:

typedef unsigned long int pthread_t;
typedef struct __pthread_internal_list
{
  struct __pthread_internal_list *__prev;
  struct __pthread_internal_list *__next;
} __pthread_list_t;
struct __pthread_mutex_s
{
  int __lock;
  unsigned int __count;
  int __owner;
  unsigned int __nusers;
  int __kind;
  short __spins;
  short __elision;
  __pthread_list_t __list;
};
struct __pthread_cond_s
{
  __extension__ union
  {
    __extension__ unsigned long long int __wseq;
    struct
    {
      unsigned int __low;
      unsigned int __high;
    } __wseq32;
  };
  __extension__ union
  {
    __extension__ unsigned long long int __g1_start;
    struct
    {
      unsigned int __low;
      unsigned int __high;
    } __g1_start32;
  };
  unsigned int __g_refs[2] ;
  unsigned int __g_size[2];
  unsigned int __g1_orig_size;
  unsigned int __wrefs;
  unsigned int __g_signals[2];
};
typedef union
{
  struct __pthread_mutex_s __data;
  char __size[40];
  long int __align;
} pthread_mutex_t;
typedef union
{
  struct __pthread_cond_s __data;
  char __size[48];
  __extension__ long long int __align;
} pthread_cond_t;

Also I am not sure if the structures would be the same on all POSIX platforms. I would have to try each of them and branching differences by platform, to make a platform agnostic bindings for miniaudio, or reuse a pthread binding in the language.

Possible change

The pthread_t, pthread_mutex_t and pthread_cond_t could be handles (pointers) on POSIX systems, just like they are on WIN32, then the init and deinit functions would allocate and free them. This would make bindings of miniaudio agnostic of the threading library.

[mackron]

I need to think about this one. This is a downgrade for C and C++ because it means a memory allocation for every pthread object, and minimizing memory allocations is one of miniaudio's big features. It'll also make it more complicated to initialize because I'll need to pass in a pointer to a ma_allocation_callbacks object to the init/uninit functions (which is also an API change). I understand your dilemma, though.

[frink]

While it's probably wise to conditionally include pthread.h we should expect that the audio is running in its own thread.

Otherwise, everything gets a little wonky.

[mackron]

He doesn't mean disabling threading, but rather moving the pthread.h dependency out of the header section and into the implementation section - exactly like I do with platform-specific headers like windows.h.

[f1nalspace]

There is a super simple solution that I use successfully in my own library.

When the handle is simply an int or pointer, you can use int or **void*, but for more complex types you can use a uint8_t** typedef as an array of N-bytes.

For example, a win32 CRITICAL_SECTION (mutex) requires at least 80-bytes, so we define a typedef with 96 bytes and we are fine ;) I round it up to a boundary of 32-bytes, so I have some extra padding when there may be changes in the future.

Of course, you have to cast back your opaque handles to the correct one in the implementation code, but almost all APIs there require a pointer to the handle anyway... so there is no real change there.

Types i use (copied for my library):

//
// Platform handles
//
#if !defined(FPL__HAS_PLATFORM_INCLUDES) || defined(FPL_OPAQUE_HANDLES)

// @NOTE(final): Semi opaque handles for win32, posix, linux

#if defined(FPL_PLATFORM_WINDOWS)

//! A win32 GUID (opaque, min 16 bytes)
typedef uint8_t fpl__Win32Guid[16];
//! A win32 handle (opaque, min 4/8 bytes)
typedef void *fpl__Win32Handle;
//! A win32 library handle (opaque, min 4/8 bytes)
typedef fpl__Win32Handle fpl__Win32LibraryHandle;
//! A win32 file handle (opaque, min 4/8 bytes)
typedef fpl__Win32Handle fpl__Win32FileHandle;
//! A win32 thread handle (opaque, min 4/8 bytes)
typedef fpl__Win32Handle fpl__Win32ThreadHandle;
//! A win32 mutex handle (opaque, min 80 bytes)
typedef uint8_t fpl__Win32MutexHandle[96];
//! A win32 signal handle (opaque, min 4/8 bytes)
typedef fpl__Win32Handle fpl__Win32SignalHandle;
//! A win32 condition variable (opaque, min 4/8 bytes)
typedef void *fpl__Win32ConditionVariable;
//! A win32 semaphore handle (opaque, min 4/8 bytes)
typedef fpl__Win32Handle fpl__Win32SemaphoreHandle;

#endif // FPL_PLATFORM_WINDOWS

#if defined(FPL_SUBPLATFORM_POSIX)

//! A POSIX library handle (opaque, min 4/8 bytes)
typedef void *fpl__POSIXLibraryHandle;
//! A POSIX file handle (opaque, min 4 bytes)
typedef int fpl__POSIXFileHandle;
//! A POSIX directory handle (opaque, min 4/8 bytes)
typedef void *fpl__POSIXDirHandle;
//! A POSIX thread handle (opaque, min 8 bytes)
typedef uint64_t fpl__POSIXThreadHandle;
//! A POSIX mutex handle (opaque, min 40 bytes)
typedef uint8_t fpl__POSIXMutexHandle[64];
//! A POSIX semaphore handle (opaque, min 32 bytes)
typedef uint8_t fpl__POSIXSemaphoreHandle[32];
//! A POSIX condition variable (opaque, min: 48 bytes)
typedef uint8_t fpl__POSIXConditionVariable[64];

#endif // FPL_SUBPLATFORM_POSIX

#if defined(FPL_PLATFORM_LINUX)

//! A linux signal handle (opaque, min 4 bytes)
typedef int fpl__LinuxSignalHandle;

#endif // FPL_PLATFORM_LINUX

#else

// @NOTE(final): Real handles for win32, posix, linux

#if defined(FPL_PLATFORM_WINDOWS)

//! A win32 GUID
typedef GUID fpl__Win32Guid;
//! A win32 handle
typedef HANDLE fpl__Win32Handle;
//! A win32 library handle
typedef HMODULE fpl__Win32LibraryHandle;
//! A win32 thread handle
typedef HANDLE fpl__Win32ThreadHandle;
//! A win32 file handle
typedef HANDLE fpl__Win32FileHandle;
//! A win32 mutex handle
typedef CRITICAL_SECTION fpl__Win32MutexHandle;
//! A win32 signal handle
typedef HANDLE fpl__Win32SignalHandle;
//! A win32 condition variable
typedef CONDITION_VARIABLE fpl__Win32ConditionVariable;
//! A win32 semaphore handle
typedef HANDLE fpl__Win32SemaphoreHandle;

#endif // FPL_PLATFORM_WINDOWS

#if defined(FPL_SUBPLATFORM_POSIX)

//! A POSIX library handle
typedef void *fpl__POSIXLibraryHandle;
//! A POSIX file handle
typedef int fpl__POSIXFileHandle;
//! A POSIX directory handle
typedef DIR *fpl__POSIXDirHandle;
//! A POSIX thread handle
typedef pthread_t fpl__POSIXThreadHandle;
//! A POSIX mutex handle
typedef pthread_mutex_t fpl__POSIXMutexHandle;
//! A POSIX semaphore handle
typedef sem_t fpl__POSIXSemaphoreHandle;
//! A POSIX condition variable
typedef pthread_cond_t fpl__POSIXConditionVariable;

#endif // FPL_SUBPLATFORM_POSIX

#if defined(FPL_PLATFORM_LINUX)

//! A linux signal handle
typedef int fpl__LinuxSignalHandle;

#endif // FPL_PLATFORM_LINUX

#endif

[edubart]

There is a super simple solution that I use successfully in my own library.

When the handle is simply an int or pointer, you can use int or **void*, but for more complex types you can use a uint8_t** typedef as an array of N-bytes.

For example, a win32 CRITICAL_SECTION (mutex) requires at least 80-bytes, so we define a typedef with 96 bytes and we are fine ;) I round it up to a boundary of 32-bytes, so I have some extra padding when there may be changes in the future.

Of course, you have to cast back your opaque handles to the correct one in the implementation code, but almost all APIs there require a pointer to the handle anyway... so there is no real change there.

This idea can work, but there is a danger there, typedefs to uint8_t will not match the same alignment of the original struct. And accessing a pointer for a misaligned address is undefined behavior. So you should probably use typedef to a type with at least the same struct alignment to be safe, like using a typedef to uint64_t should be safer, so it will have alignment of at least 8 bytes, and this would work for most structures (except the ones using long double).

[f1nalspace]

Oh yes you are right. Totally forget about that.

[mackron]

I'm not sure how to handle this one. If it was specific to one platform, like Win32, I'd be open to changing this, but considering each platform can have different sizes for the pthread_t, pthread_mutex_t and pthread_cond_t types I'm not quite sure what I should do here.

For example, on my Linux machine sizeof(pthread_mutex_t) = 40, whereas on FreeBSD sizeof(pthread_mutex_t) = 8. I'm just thinking about if some other platform comes along and causes problems due to it's size not being compatible.

The safest way to do this is to allocate the structures on the heap, but that introduces a memory allocation and also adds the requirement for a ma_allocation_callbacks pointer to be passed into ma_mutex_init(), etc. I'm not sure this feature request is worth it to be honest. It's not like pthread.h is as intrusive as something like windows.h.

[mackron]

In my c89thread project I did an experiment with this. I added a compile time option called NO_PTHREAD_IN_HEADER which if set, will manually defined some structs. Commit if you're curious: https://github.com/mackron/c89thread/commit/fa4fc5eed14acb9c4972c43398f290d7e138d941

This seems to work on Linux and BSD, but I'm still unsure if it's the right approach. To use it you would need to put that option on the command line or before including the header.

[edubart]

In my c89thread project I did an experiment with this. I added a compile time option called NO_PTHREAD_IN_HEADER which if set, will manually defined some structs. Commit if you're curious: mackron/c89thread@fa4fc5e

I think that is fair solution for the issue, and we can manually enable it so won't affect everyone, and does not involve allocations, we just need to make sure of two things:

1. The size of the struct is big enough for all platforms.
2. The struct contains at least a field type with the biggest alignment type of the original struct.

Looks like some pthread fields uses long long int, and that will have alignment of 8 bytes, while in your patch you have just used char, this will make the structure align to be 1 in most C compilers, while the original struct would have align 8. So there is a misalign bug there, that may cause undefined behavior in some architectures (not all, some does support unaligned reads). To fix add a dummy field with long long int type, you could do like pthread originally does that:

typedef union
{
  struct __pthread_cond_s __data;
  char __size[48];
  __extension__ long long int __align;
} pthread_cond_t;

Notice the __align field inside a union, that is there just to force structure alignment.

[mackron]

1. The size of the struct is big enough for all platforms.

Yeah that's the part I'm concerned about. I've set it to 40 bytes and 48 bytes, but it's not 100% reliable because some other unknown platform might be bigger (unlikely, but possible). For example, Linux seems to use transparent structs, whereas the BSD variants seem to use opaque structs so their struct size is totally different. There's also memory wastage due to the custom structs being more than needed on some platforms.

2. The struct contains at least a field type with the biggest alignment type of the original struct.

You're absolutely right and I'm an idiot for not considering that, even when you mentioned it in an earlier comment which should have reminded me! I fixed that.

I guess by having a compile time option to exclude pthread.h from the header, and disabled by default, would be safe. If someone wants to enable it, they need to do so explicitly and the risk is on them. I'll sit on this and have a bit of a think about this one.

[edubart]

Will miniaudio start using c89thread? So we will be able to include it without pthread.h?

[mackron]

Maybe at some point, but I'm not sure. I just put that experimental stuff into c89thread because it's not in as widespread use as miniaudio and also because it's just a convenient place to experiment. Most likely I'll be doing what I did in c89thread and intentionally leave the compile time option undocumented and put a comment in there that it's use at your own risk.

[mackron]

I've gone ahead and done that: 6501a6fdb00b523eb82bed31bc80a56fa8c26d2c. This is in the dev branch.

[mackron]

This has been released. Closing.