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

[libc] implement pthread condition variable support (pthread_cond_*) #85282

Open
nickdesaulniers opened this issue Mar 14, 2024 · 12 comments
Open
Assignees
Labels

Comments

@nickdesaulniers
Copy link
Member

nickdesaulniers commented Mar 14, 2024

int pthread_cond_broadcast(pthread_cond_t *);
int pthread_cond_destroy(pthread_cond_t *);
int pthread_cond_init(pthread_cond_t *, const pthread_condattr_t *);
int pthread_cond_signal(pthread_cond_t *);
int pthread_cond_timedwait(pthread_cond_t *,
pthread_mutex_t *, const struct timespec *);
int pthread_cond_wait(pthread_cond_t *, pthread_mutex_t *);

Specifically, FWICT libc++ depends on:

pthread_cond_broadcast
pthread_cond_destroy
pthread_cond_signal
pthread_cond_timedwait
pthread_cond_wait

(but not pthread_cond_init?? PTHREAD_COND_INITIALIZER is used instead) I don't think we implement any of these. We'll likely need to implement pthread_cond_init first to begin testing the rest.

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 14, 2024

@llvm/issue-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

int [pthread_cond_broadcast](https://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_cond_broadcast.html)(pthread_cond_t *); int [pthread_cond_destroy](https://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_cond_destroy.html)(pthread_cond_t *); int [pthread_cond_init](https://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_cond_init.html)(pthread_cond_t *, const pthread_condattr_t *); int [pthread_cond_signal](https://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_cond_signal.html)(pthread_cond_t *); int [pthread_cond_timedwait](https://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_cond_timedwait.html)(pthread_cond_t *, pthread_mutex_t *, const struct timespec *); int [pthread_cond_wait](https://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_cond_wait.html)(pthread_cond_t *, pthread_mutex_t *);

Specifically, FWICT libc++ depends on:

pthread_cond_broadcast
pthread_cond_destroy
pthread_cond_signal
pthread_cond_timedwait
pthread_cond_wait

(but not pthread_cond_init??) I don't think we implement any of these. We'll likely need to implement pthread_cond_init first to begin testing the rest.

@elhewaty
Copy link
Member

Hi, I am new to libc, but I have experience with LLVM.
Can I work on this?
are there relevant work I can look at?

@michaelrj-google
Copy link
Contributor

You can absolutely try if you're interested, I will warn you it may be difficult. You can look at #85289 for a list of general things needed to add a new function.

For relevant work I'd recommend looking at the pthread functions that have already been implemented in LLVM-libc, they're in https://github.com/llvm/llvm-project/tree/main/libc/src/pthread

Feel free to reach out if you have more questions or want more advice on where to look.

@nickdesaulniers
Copy link
Member Author

In particular, we seem to already have support for C11 threads. We have a CndVar type in libc/src/threads/linux/CndVar.h; perhaps that can be reused between C11 threads and pthreads?

@changkhothuychung
Copy link
Contributor

I would like to take this if this is available, thanks!

@nickdesaulniers
Copy link
Member Author

@elhewaty are you working on this? Or should I assign this to someone else?

(These may be more difficult to implement than some of our other missing functions)

@elhewaty
Copy link
Member

elhewaty commented Apr 1, 2024

I am sorry I will work on it, I was busy with working on this (#85592). If you think it'll be difficult for me you can suggest something that's not very hard but not trivial too.

Thanks

@changkhothuychung
Copy link
Contributor

@nickdesaulniers just random curiosity. So currently, if a clang user tries to call these pthread_cond_* functions, it should compile, I believe. But these functions do not seem to be in libc yet, so I am curious what current implementations will be linked for those pthread_cond_* functions?

@nickdesaulniers
Copy link
Member Author

But these functions do not seem to be in libc yet

Correct, this bug is about implementing them in llvm-libc.

So currently, if a clang user tries to call these pthread_cond_* functions, it should compile, I believe.

If they're calling these and trying to use llvm libc then depending on what mode they're trying to use llvm-libc in, I'd expect 1 of 2 possible different failures:

  • in fullbuild mode I'd expect a compile time diagnostic that these functions were not declared before used (I think this is an error in new C modes).
  • in overlay mode then the code may compile+link, but I'd expect there'd probably be some kind of mysterious bugs due to possible ABI differences; we probably don't want users overlaying some of these pthread functions as opposed to all of them together.

@michaelrj-google
Copy link
Contributor

What Nick says is correct, but also if you're just using clang and not LLVM-libc, then it will just use your system's libc.

Additionally, we don't support any of the pthread functions in overlay mode due to the problems with the LLVM-libc ABI and system libc ABI possibly not matching, so in overlay mode you'll get your system libc's version.

@nickdesaulniers
Copy link
Member Author

nickdesaulniers commented Apr 11, 2024

Looking into this a bit closer, I suspect that if we can use a similar definition for our pthread_cond_t as our cnd_t, then we can probably reuse most (all?) of libc/src/threads/linux/CndVar.h. Looking at libc/test/integration/src/threads/cnd_test.cpp, it looks like to start testing any of these, we'll need pthread_cond_init, pthread_cond_signal, pthread_cond_wait, and pthread_cond_destroy all implemented together in one PR. Or perhaps just create+destroy in one PR to start.

I'm going to take this; @elhewaty I'm going to refer you to some of our perhaps easier beginner bugs: https://github.com/llvm/llvm-project/issues?q=is%3Aopen+is%3Aissue+label%3Alibc+label%3A%22good+first+issue%22

@nickdesaulniers
Copy link
Member Author

nickdesaulniers commented Apr 12, 2024

It might also be useful to add support for pthread_condattr_init first. FWICT, pthread_condattr_t have no analog in C11 threads. https://docs.oracle.com/cd/E19683-01/806-6867/sync-67790/index.html has good docs relative to man pages on what that means.

nickdesaulniers added a commit to nickdesaulniers/llvm-project that referenced this issue Apr 12, 2024
Basic support for pthread condition variables. Does not support:
- pthread_cond_{timedwait|broadcast}
- non-nullptr pthread_condattr_t values for pthread_cond_init

Relies on the existing CndVar, which should be moved from
libc/src/threads/linux/CndVar.h to libc/src/__support/threads/.

Link: llvm#85282
Link: llvm#88580
Link: llvm#88581
Link: llvm#88582
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants