Fix pthread undefined behaviour in thread_create

[tobil4sk]

Closes #232.

As described in the bug report, it is undefined behaviour for a mutex to be destroyed while it is still locked, and also for a locked mutex to be unlocked by a different thread (regardless of PTHREAD_PROCESS_SHARED, which was suggested in the bug report but does not help here).

These issues can be avoided using a condition variable instead of relying on undefined behaviour of mutexes.

The relevant docs are provided below:

Attempting to destroy a locked mutex results in undefined behavior.

https://pubs.opengroup.org/onlinepubs/007904875/functions/pthread_mutex_destroy.html

If a thread attempts to unlock a mutex that it has not locked or a mutex which is unlocked, undefined behaviour results.

https://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_mutex_lock.html

Here is another example of using condition variables like this: https://gist.github.com/rtv/4989304

[tobil4sk]

Recently, I've occasionally been getting pthread errors when running haxe tools that use neko, so I think this is something that is worth fixing not just for the theoretical value of avoiding undefined behaviour.

If you add the error checking flag to the mutex, it is possible to verify that the current behaviour is incorrect:

diff --git a/vm/threads.c b/vm/threads.c
index 524d9ac8..62d427c2 100644
--- a/vm/threads.c
+++ b/vm/threads.c
@@ -150,7 +150,12 @@ EXTERN int neko_thread_create( thread_main_func init, thread_main_func main, voi
        pthread_attr_t attr;
        pthread_attr_init(&attr);
        pthread_attr_setdetachstate(&attr,PTHREAD_CREATE_DETACHED);
-       pthread_mutex_init(&p.lock,NULL);
+
+       pthread_mutexattr_t mutex_attr;
+       pthread_mutexattr_init(&mutex_attr);
+       pthread_mutexattr_settype(&mutex_attr, PTHREAD_MUTEX_ERRORCHECK_NP);
+
+       pthread_mutex_init(&p.lock,&mutex_attr);
        pthread_mutex_lock(&p.lock);
        // force the use of a the GC method to capture created threads
        // this function should be defined in gc/gc.h

This way you get a segmentation fault with a simple haxe program:

function main() {
	sys.thread.Thread.create(function() {
		trace("hello world");
	});
}

[Simn]

This stuff is always confusing to review, but what you're saying makes sense to me.

Intuitively, I'd expect this order to be reversed:

	pthread_cond_signal(&lp->cond);
	pthread_mutex_unlock(&lp->lock);

I can't really find any decisive information on this though, so I guess it's alright. Feel free to merge and if everything breaks we know where to find you!

[tobil4sk]

Good point. It is the way I have seen it in other examples, including the example in my man page for pthread_cond_signal.

I did some research and found a few sources that say it doesn't matter in terms of logic, but may result in more predictable scheduling.

The signal and broadcast functions can be called by a thread whether or not it currently owns the mutex associated with the condition variable. If predictable scheduling behavior is required from the applications viewpoint, however, the mutex should be locked by the thread that calls pthread_cond_signal() or pthread_cond_broadcast().

https://www.ibm.com/docs/en/i/7.4.0?topic=ssw_ibm_i_74/apis/users_76.html

https://danluu.com/threads-faq/#Q233

https://web.archive.org/web/20150419055326/http://www.domaigne.com/blog/computing/condvars-signal-with-mutex-locked-or-not/

https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_cond_signal.html

We probably don't care much about predictability but I think it's good to go with the standard order that most examples use.