Skip to content

threads: implement support for pthread mutexes#315

Merged
abrown merged 5 commits intoWebAssembly:mainfrom
abrown:pthreads-mutex
Aug 22, 2022
Merged

threads: implement support for pthread mutexes#315
abrown merged 5 commits intoWebAssembly:mainfrom
abrown:pthreads-mutex

Conversation

@abrown
Copy link
Copy Markdown
Collaborator

@abrown abrown commented Aug 17, 2022

This change adds pthread's mutex support to the THREAD_MODEL=posix
build of wasi-libc. Some less-common features are unsupported and
documented here:

  • mutex robust lists are disabled and their use will return a runtime
    error; currently WASI does not support the concept of multiple
    processes so maintaining robust mutexes across processes does not yet
    make sense in wasi-libc
  • timed locks with priority inheritance (PI) are disabled and will act
    as any other mutex; this feature is related to task priorities which
    is not yet relevant in the WASI ecosystem (see priority-inheritance
    futexes
    )
  • thread cancellation is ignored; this feature is difficult to support
    and @sunfishcode would likely want to discuss this before adding it at
    some later time.

This change adds pthread's mutex support to the `THREAD_MODEL=posix`
build of wasi-libc. Some less-common features are unsupported and
documented here:
- mutex robust lists are disabled and their use will return a runtime
  error; currently WASI does not support the concept of multiple
  processes so maintaining robust mutexes across processes does not yet
  make sense in wasi-libc
- timed locks with priority inheritance (PI) are disabled and will act
  as any other mutex; this feature is related to task priorities which
  is not yet relevant in the WASI ecosystem (see [priority-inheritance
  futexes](https://man7.org/linux/man-pages/man2/futex.2.html))
- thread cancellation is ignored; this feature is difficult to support
  and @sunfishcode would likely want to discuss this before adding it at
  some later time.
Comment thread libc-top-half/musl/src/internal/pthread_impl.h Outdated
@abrown abrown requested review from sbc100 and sunfishcode August 17, 2022 17:32
Comment thread libc-top-half/musl/src/thread/pthread_mutexattr_setprotocol.c Outdated
Copy link
Copy Markdown
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

I think it would be good to be able to do some kind of verification that these changes work... can you build a version of wasi-sdk that includes this multi-threaded build of libc and test some basic programs? Even just a program that doesn't create any thread and locks and unlock a mutex would be better than nothing... otherwise its hard to show that these changes are correct.

Comment thread libc-top-half/musl/src/thread/pthread_mutexattr_setrobust.c
((char *)next - sizeof(void *)) = prev;
}
#ifdef __wasilibc_unmodified_upstream
if (type&8) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you have a map of what bits are what in the mutex type? What do bit 8 and bit 128 mean?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not sure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the long run I think it would be good for us to have a clear understand on what those bits mean and what we are expluding from our build. But its very early days and I'm OK with landing this as is if you like.

Comment thread libc-top-half/musl/src/thread/__timedwait.c Outdated
Comment thread libc-top-half/musl/src/internal/pthread_impl.h Outdated
@abrown
Copy link
Copy Markdown
Collaborator Author

abrown commented Aug 17, 2022

I think it would be good to be able to do some kind of verification that these changes work... can you build a version of wasi-sdk that includes this multi-threaded build of libc and test some basic programs? Even just a program that doesn't create any thread and locks and unlock a mutex would be better than nothing... otherwise its hard to show that these changes are correct.

My plan, which I had discussed with @sunfishcode, was to use libc-test as much as possible. It has tests like src/function/pthread_mutex.c that are almost simple enough to run today, but still need pthread_create, pthread_join, and some sem_* definitions to compile. Once we have those functions defined, @sunfishcode, what do you think about incorporating libc-test into wasi-sdk as a Git submodule as an alternate way to run some functionality tests? Or alternately we could do the same here in wasi-libc?

@sbc100
Copy link
Copy Markdown
Member

sbc100 commented Aug 17, 2022

libc-test sounds great in the medium/long term. That work could start right away and we could use it to test the non-threaded version of wask-sdk.

In the short term I think just starting with something much smaller would be fine. Even its just something you build and run locally for now. Just so we can have some kind of guarantee that basic programs can be linked and run against this WIP libc.

@sunfishcode
Copy link
Copy Markdown
Member

Something like libc-test would indeed be great to have integrated in some form. Note that it contains GPLv2-licensed code, so if we integrate it into the repo we should clearly document the licenses.

I guess to me, it doesn't feel urgent to add a test that we can build and run locally, when we don't have pthread_create yet.

@abrown
Copy link
Copy Markdown
Collaborator Author

abrown commented Aug 19, 2022

@sbc100, I don't have a strong desire or the time to build a libc test suite that we would later replace by something like libc-test. It seems reasonable to me to wait for more of the pthread implementation to be exposed before integrating some form of testing (perhaps in a separate repository to deal with the licensing issues @sunfishcode points out). That being said, if you had a C file you created to test this functionality, I wouldn't mind compiling it with wasi-sdk and running it in a Wasm engine to see what happens.

@sbc100
Copy link
Copy Markdown
Member

sbc100 commented Aug 19, 2022

@sbc100, I don't have a strong desire or the time to build a libc test suite that we would later replace by something like libc-test. It seems reasonable to me to wait for more of the pthread implementation to be exposed before integrating some form of testing (perhaps in a separate repository to deal with the licensing issues @sunfishcode points out). That being said, if you had a C file you created to test this functionality, I wouldn't mind compiling it with wasi-sdk and running it in a Wasm engine to see what happens.

I guess I'll leave it up you.. since you are doing all the work here. As long as its clear to everyone that there zero expectation that any of this works.. until we have some testing.

@abrown abrown merged commit 33c3753 into WebAssembly:main Aug 22, 2022
@abrown abrown deleted the pthreads-mutex branch August 22, 2022 15:39
john-sharratt pushed a commit to john-sharratt/wasix-libc that referenced this pull request Mar 6, 2023
This change adds pthread's mutex support to the `THREAD_MODEL=posix`
build of wasi-libc. Some less-common features are unsupported and
documented here:
- mutex robust lists are disabled and their use will return a runtime
  error; currently WASI does not support the concept of multiple
  processes so maintaining robust mutexes across processes does not yet
  make sense in wasi-libc
- timed locks with priority inheritance (PI) are disabled and will act
  as any other mutex; this feature is related to task priorities which
  is not yet relevant in the WASI ecosystem (see [priority-inheritance
  futexes](https://man7.org/linux/man-pages/man2/futex.2.html))
- thread cancellation is ignored; this feature is difficult to support
  and @sunfishcode would likely want to discuss this before adding it at
  some later time.
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.

3 participants