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

Improve some pthreads stub functions, batch 1 #525

Merged
merged 4 commits into from
Aug 6, 2024

Conversation

ArcaneNibble
Copy link
Contributor

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.

@sbc100 sbc100 requested a review from abrown August 6, 2024 15:00
libc-top-half/musl/include/sched.h Outdated Show resolved Hide resolved
libc-top-half/musl/src/thread/pthread_attr_setschedparam.c Outdated Show resolved Hide resolved
The Open Group Base Specifications state that:
* The `sched_param` _shall_ be defined, it isn't optional
* Likewise, `pthread_attr_{get|set}schedparam` are not optional,
  but they can be implemented as stubs that only allow priority 0
* `pthread_attr_getinheritsched` *is* optional.
  It is part of the "TPS" Thread Execution Scheduling functionality
  and the Realtime Threads option group. The current stubs do not
  provide set, so, for consistency, get shouldn't be provided.
This is part of the "TPS" Thread Execution Scheduling functionality,
and the corresponding set function is already not provided
This function isn't optional, and in any case setclock *is* provided
Although it is not supported in WASI, thread cancellation is an important
feature of pthreads with wide-ranging implications.

Provide the best stubs we can within the limitations by allowing
cancellation type and state to be set, even though they will have no
effect as pthread_cancel is stubbed to always return ENOTSUP

Remove the THREAD_MODEL=single guard as it's currently not being
compiled in that case, and because we intend to provide
single-threaded stub implementations in the future.
Incidentally replace the existing pthread_setcancelstate stub
with one which actually correctly writes a value to the old parameter.
Copy link
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.

lgtm, but lets wait for @abrown to chip in too

Copy link
Collaborator

@abrown abrown left a comment

Choose a reason for hiding this comment

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

@ArcaneNibble, thanks for working on this. One thing I didn't understand: you mention that

the pthread_atfork stub has been dropped as it is now officially obsolete as of the very recent Issue 8 of the specifications.

Which specification? Which issue? Maybe a link would help me understand what you mean here.

Otherwise I think this makes sense. We've talked for some time about adding failing stubs to get more programs to compile and this goes down that path.

@ArcaneNibble
Copy link
Contributor Author

The specification I'm referring to is "The Open Group Base Specifications Issue 8, IEEE Std 1003.1-2024"

The relevant page is at https://pubs.opengroup.org/onlinepubs/9799919799/functions/pthread_atfork.html

As stated in the "Change History" section, pthread_atfork is officially marked as obsolete even though it hasn't been completely removed yet. This is because the function is basically impossible to use correctly (as explained in the "Application Usage" section). Because this is a "new" implementation of pthreads, I don't see a strong need for a failing stub for this function.

@abrown
Copy link
Collaborator

abrown commented Aug 6, 2024

Ok, thanks for that link! And I agree with the rationale for leaving it out.

@abrown abrown merged commit 230d4be into WebAssembly:main Aug 6, 2024
8 checks passed
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