Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a stub pthreads library for THREAD_MODEL=single #518

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

ArcaneNibble
Copy link
Contributor

@ArcaneNibble ArcaneNibble commented Jul 18, 2024

This patch series first starts with a number of commits stubbing out functions in the existing THREAD_model=posix code. According to "The Open Group Base Specifications Issue 7, 2018 edition", there are a number of mandatory functions which have not been provided. There are also some optional functions that have been partially provided in a not-useful way (e.g. get but no set function). For these, I have chosen to clean them up and remove the get functions for consistency. EDIT: These have been split off into separate PRs and merged.

The remainder of the patches then build up a stub implementation of pthreads for THREAD_MODEL=single. I have done my best to try to make sure that all functions are as conforming as possible (under the assumption that another thread cannot ever be launched). This means that objects such as mutexes and rwlocks actually do update their state and will correctly fail when locks cannot be acquired.

When an inevitable deadlock occurs, I have chosen to return EDEADLK when it has been explicitly listed as a permissible return value, and to invoke __builtin_trap otherwise.

I have tested this by rebuilding libc++ with threads enabled and then smoke-testing Clang/LLVM-on-WASI to make sure that it can compile a simple program. I have not run any more-extensive conformance testing.

Fixes #501

@yamt
Copy link
Contributor

yamt commented Jul 18, 2024

does this break pthread detection mechanisms like cmake find_package(Threads)?

@ArcaneNibble
Copy link
Contributor Author

does this break pthread detection mechanisms like cmake find_package(Threads)?

I don't know. I don't use cmake beyond building other software that uses it.

Several of the comments in the linked issue are implying that, even if it might, not having these stubs is causing even more difficulty with porting software.

libc-top-half/musl/arch/wasm32/pthread_arch.h Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
libc-top-half/musl/src/thread/pthread_atfork.c Outdated Show resolved Hide resolved
libc-top-half/musl/src/thread/pthread_attr_setguardsize.c Outdated Show resolved Hide resolved
@yamt
Copy link
Contributor

yamt commented Aug 2, 2024

does this break pthread detection mechanisms like cmake find_package(Threads)?

I don't know. I don't use cmake beyond building other software that uses it.

Several of the comments in the linked issue are implying that, even if it might, not having these stubs is causing even more difficulty with porting software.

having a stub for mutex etc is probably fine.

otoh, i feel it isn't a great idea to provide pthread_create.

sbc100 pushed a commit that referenced this pull request Aug 2, 2024
This is one part of breaking up #518 into smaller PRs. This should be
(IMO) the least-controversial batch of commits
abrown pushed a commit that referenced this pull request Aug 6, 2024
This is the next part of breaking up #518 into smaller PRs.

This is the rest of the commits which change the existing
`THREAD_MODEL=posix` functionality. It:

* _removes_ some functions which are optional and which were already
nonfunctional.
* (Continues to) establish a precedent of trying to remain as compatible
with Open Group specifications as possible, even when there are major
differences in WASI capabilities (i.e. thread cancellation)

Compared to the RFC PR, the `pthread_atfork` stub has been dropped as it
is now officially obsolete as of the very recent Issue 8 of the
specifications.
@ArcaneNibble ArcaneNibble changed the title [RFC] Implement a stub pthreads library for THREAD_MODEL=single Implement a stub pthreads library for THREAD_MODEL=single Aug 6, 2024
@ArcaneNibble
Copy link
Contributor Author

does this break pthread detection mechanisms like cmake find_package(Threads)?

I don't know. I don't use cmake beyond building other software that uses it.
Several of the comments in the linked issue are implying that, even if it might, not having these stubs is causing even more difficulty with porting software.

having a stub for mutex etc is probably fine.

otoh, i feel it isn't a great idea to provide pthread_create.

Although there are also thoughts of potentially changing it, libcxx currently does require/expect pthread_create to exist. Incidentally, it performs detection whether or not pthreads is supported by looking for pthread_once and not pthread_create (although said detection doesn't actually seem to work properly for some reason, possibly because wasm handles undefined symbols by importing them from env rather than failing?)

@ArcaneNibble
Copy link
Contributor Author

I've rebased this PR and fixed the issues that CI uncovered. I do wish to merge this, but a decision needs to be made about how to handle the "pthreads detection in existing code" issue.

@@ -0,0 +1,21 @@

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call this directory pthread-stubs?

And maybe it should live at the top level alongside dlmalloc and emmalloc (which override/replace libc functions too). @sunfishcode any preference?

@ArcaneNibble
Copy link
Contributor Author

ping?

@yamt
Copy link
Contributor

yamt commented Aug 19, 2024

does this break pthread detection mechanisms like cmake find_package(Threads)?

I don't know. I don't use cmake beyond building other software that uses it.
Several of the comments in the linked issue are implying that, even if it might, not having these stubs is causing even more difficulty with porting software.

having a stub for mutex etc is probably fine.
otoh, i feel it isn't a great idea to provide pthread_create.

Although there are also thoughts of potentially changing it, libcxx currently does require/expect pthread_create to exist. Incidentally, it performs detection whether or not pthreads is supported by looking for pthread_once and not pthread_create (although said detection doesn't actually seem to work properly for some reason, possibly because wasm handles undefined symbols by importing them from env rather than failing?)

anyway, libcxx is not a representative application, i suspect.

can you explain why having a stub pthread_create is important for you?
otherwise, can you consider to drop it?

@ArcaneNibble
Copy link
Contributor Author

The immediate reason is: I want to be able to build clang+LLVM for WASI (using @whitequark's patchset), which requires threading functions inside libcxx. The usage of types such as std::mutex is so pervasive throughout the LLVM codebase that it will probably never be acceptable to put compile-time conditionals around all of them. This is what started the desire for some kind of dummy threading stubs.

As far as I can tell, the only possible choices are:

  1. do what I've done here, where all of the stubs (i.e. enough to build libcxx with threads enabled) exist inside wasi-libc. Subsequently change wasi-sdk such that all builds have threads enabled inside libcxx. Use the resulting wasi-sdk to build clang+LLVM
  2. do choice 1 except introduce three threading models (i.e. "single", "stub", "posix"). This was discussed in a review comment with @sbc100, who didn't seem to see it necessary to do that.
  3. make it entirely libcxx's problem
    1. by making libcxx carry stubs for everything (synchronization and nonfunctional thread creation). I predict that this would result in simply having this exact same discussion in a brand-new GitHub thread.
    2. by making libcxx carry stubs for synchronization primitives only, still removing e.g. std::thread. This would subsequently require hunting down all uses of these types inside the clang+LLVM codebase in order to build the final intended application, but this looks much more doable compared to trying to remove e.g. std::mutex. There does exist some desire to do this. However, this doesn't handle any C code that would need pthreads stubs, only C++ code. I am currently trying to avoid having to spend the effort required to make this option work.
  4. reject upstreaming a stub for pthread_create here (keeping only synchronization primitive stubs). This would mean that libcxx still wouldn't build.
    1. Fix libcxx to support "synchronization primitives exist but pthread_create doesn't" as a platform. I don't know about the libcxx maintainers, but I personally would consider this very inelegant and clearly a hack, as such a "platform" would certainly not be POSIX-conformant. This can technically be done by simply conditionally guarding one line of code inside libcxx in order to end up with a libcxx where std::thread exists but always fails, but once again "I predict that this would result in simply having this exact same discussion in a brand-new GitHub thread." Alternatively, this can be done by introducing the ability for libcxx to have std::thread etc. deleted but std::mutex etc. present. This would be just as difficult as doing option 3.ii.
    2. Do option 3.i or 3.ii anyways. This works but results in twice the amount of stubs existing.
    3. Some downstream user creates a stub for pthread_create anyways and subsequently rebuilds libcxx against it. This is strictly worse as you're now doing the exact same thing except for each downstream user.

The tl;dr is "I need libcxx. I am trying to avoid having to put in all of the effort needed to enable a new 'libcxx has synchronization primitives but no thread creation' build mode for libcxx. I also generally don't think omitting pthread_create is a particularly good idea."

In general, the code I want to get ported to WASI would look something like

// in do_work_{foo|bar|baz|qux}.c
if (use_multiple_threads) {
  pthread_create(...);
} else {
  do_work_synchronously();
}
// in main.c
if (check_command_line_flags("-threads"))
  use_multiple_threads = 1;

With full pthreads stubs in wasi-libc, this code can either be ported by doing nothing ("just" don't pass the -threads command line argument) or by having a single #ifdef __WASI__ in main.c, instead of multiple #ifdefs sprinkled throughout all of the do_work_*.c parts of the program.

Can you perhaps elaborate further as to why you oppose having pthread_create?

@whitequark
Copy link
Contributor

(Thanks for doing this work, @ArcaneNibble)

@yamt
Copy link
Contributor

yamt commented Aug 20, 2024

The immediate reason is: I want to be able to build clang+LLVM for WASI (using @whitequark's patchset), which requires threading functions inside libcxx. The usage of types such as std::mutex is so pervasive throughout the LLVM codebase that it will probably never be acceptable to put compile-time conditionals around all of them. This is what started the desire for some kind of dummy threading stubs.

how about having libwasi-emulated-pthread.a to follow the precedents?

Can you perhaps elaborate further as to why you oppose having pthread_create?

i don't want to break pthread-detection mechanisms around there, including cmake FindThreads.

@ArcaneNibble
Copy link
Contributor Author

how about having libwasi-emulated-pthread.a to follow the precedents?

Oh! I quite like this idea! I would still want libcxx to be built with threading support enabled for both single and posix though, and I hope to avoid having to hack up its build system to do something special.

i don't want to break pthread-detection mechanisms around there, including cmake FindThreads.

The deliberate intention was to cause detection mechanisms to now detect "this platform has pthreads." This is why I implemented all of the stub functions s.t. they comply with the POSIX specifications (rather than just returning 0 like some other examples that have been posted in #501 ). EAGAIN is explicitly listed as a permissible way for pthread_create to fail, so software should be expected to do something in response.

Can we get community consensus on the approach of "put single-threaded pthreads stubs into libwasi-emulated-pthread.a but always build libcxx with threading enabled (thus causing a program to have undefined symbols if it uses libcxx threading but doesn't link the .a file)" before I go ahead and implement it?

@yamt
Copy link
Contributor

yamt commented Aug 21, 2024

I would still want libcxx to be built with threading support enabled for both single and posix though

do you mean to make wasi-sdk ship libcxx built with LIBCXX_ENABLE_THREADS=ON?

why?

@ArcaneNibble
Copy link
Contributor Author

do you mean to make wasi-sdk ship libcxx built with LIBCXX_ENABLE_THREADS=ON?

why?

Yes, because I specifically want to easily port software which uses C++11 threading. i.e. the exact same reason why I am trying to enable pthreads stubs

@ArcaneNibble
Copy link
Contributor Author

@yamt I feel like we are repeatedly talking past each other. Is there any way we can jump into a real-time discussion (e.g. IRC)?

@ArcaneNibble
Copy link
Contributor Author

ping, is there some way through which we can arrive at community consensus about how this pthreads stub functionality should be named and exposed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider including a pthread.h for all targets
4 participants