This is an archive of the discontinued LLVM Phabricator instance.

[libc] Take 2: Add linux implementations of thrd_create and thrd_join functions.
ClosedPublic

Authored by sivachandra on Mar 7 2020, 3:37 PM.

Details

Summary

The following are the differences from the first version:

  1. The kernel does not copy the stack for the new thread (it cannot).

The previous version missed this fact. In this new version, the new
thread's start args are copied on to the new stack in a known location
so that the new thread can sniff them out.

  1. A start args sniffer for x86_64 has been added.
  2. Default stack size has been increased to 64KB.

Diff Detail

Event Timeline

sivachandra created this revision.Mar 7 2020, 3:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2020, 3:37 PM

Use atomic_uint_least32_t type for FutexData.

Actually use atomic_uchar for the FutexData type.

abrachet accepted this revision.Mar 7 2020, 7:24 PM

This looks good to me. Maybe worth waiting for @phosek.

FWIW the tests works fine on my machine but the last patch did as well. Also, exceptions can properly unwind the threads stack which I thought might have been an issue with calling clone in C and not being able to emit the correct cfi directives. Which is to say I'm fairly confident everything is set up properly now :)

libc/src/threads/linux/thrd_create.cpp
82–83

Maybe put a TODO here because clone's arguments are different on other architectures. Perhaps we should just use the libc wrapper for clone here later?

libc/src/threads/linux/thrd_join.cpp
32–34

Maybe this comment should be moved up to the while loops condition

libc/src/threads/linux/x86_64/thread_start_args.h.in
13

workd -> work

This revision is now accepted and ready to land.Mar 7 2020, 7:24 PM

I think implementing pthread on top of C11 threads is plausible on Fuchsia because it just provides POSIX lite. However, the same idea copying over to Linux may not be practical. For one thing, the performance of pthread_mutex_lock matters. Implementing it on top of mtx_lock costs an extra function call, not to say that some features are not available in C11 mutex. mtx_* and thrd_* are usely so rarely in practice. Implementing them on top of pthread will have an infinitesimal cost.

sivachandra marked 3 inline comments as done.

Address comments.

FWIW the tests works fine on my machine but the last patch did as well. Also, exceptions can properly unwind the threads stack which I thought might have been an issue with calling clone in C and not being able to emit the correct cfi directives. Which is to say I'm fairly confident everything is set up properly now :)

I have access to a machine on which it was failing previously but passing now :)

phosek accepted this revision.Mar 9 2020, 6:07 PM

I think implementing pthread on top of C11 threads is plausible on Fuchsia because it just provides POSIX lite. However, the same idea copying over to Linux may not be practical. For one thing, the performance of pthread_mutex_lock matters. Implementing it on top of mtx_lock costs an extra function call, not to say that some features are not available in C11 mutex. mtx_* and thrd_* are usely so rarely in practice. Implementing them on top of pthread will have an infinitesimal cost.

I don't think that either of these is practical, that is neither implementing pthreads on top C11 threads nor C11 threads on top of pthreads. In the former case, pthreads provide richer more complex API that's difficult to implement solely on top of the C11 threads one. In the latter case, you're forcing all platforms, even the non-POSIX ones, to implement enough of POSIX to allow the implementation of pthreads which is unnecessarily restrictive.

The solution we've adopted in Fuchsia is to implement a set of lower-level libraries, e.g. libsync which provide basic primitives, and then implement mtx_t and pthread_mutex_t separately on top of these rather than layering one on top of the other or vice versa.

This revision was automatically updated to reflect the committed changes.