Only functional for stack growsdown (same as before), but custom
stack, stacksize, guardsize, and detachstate all should be
working.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Sorry for the long delay here. Part of the reason why I have been putting this off is because you mixed up some nice cleanups with the actual feature enhancements. I have not yet gone through the tests.
libc/src/__support/threads/linux/thread.cpp | ||
---|---|---|
88 | You already check for overflow isn't it? So, do we need to keep this TODO? This pattern repeats at many places in this patch. | |
241 | Nit: Because of the naming convention we use for constants, this should be named INTTERNAL_STACK_DATA_SIZE. | |
libc/src/__support/threads/thread.h | ||
142 | Nit: Naming style for all of these constants should be fixed. | |
libc/src/pthread/pthread_attr_getstacksize.cpp | ||
23 ↗ | (On Diff #515456) | Can you point me to documentation which says the getstack* functions should fail with EINVAL? They way I read the POSIX docs, I thought the setstack* functions fail with EINVAL on improper stack sizes. |
libc/src/pthread/pthread_attr_init.cpp | ||
20 | We will do something better after this change lands. | |
21 | Can we use the value in the Thread class? | |
libc/src/pthread/pthread_create.cpp | ||
47 | Going by my comment above, do we need this? Also, considering that all pthread_attr_set* functions fail anyway on bad inputs, do we need do these checks here at all? | |
59 | You can use LIBC_UNLIKELY: https://github.com/llvm/llvm-project/blob/main/libc/src/__support/macros/optimization.h#L25 | |
libc/src/threads/thrd_create.cpp | ||
14 | Do we need this? |
- Fix error check in pthread_attr_get* changes
- Remove todos that have already been addressed
- Use defaults from thread.h
- Add back support for stacksize == 0
libc/src/__support/threads/linux/thread.cpp | ||
---|---|---|
88 | Sorry, kind of left those in as questions for you (maybe overflow check is unnecessary and such). Will drop those. | |
libc/src/pthread/pthread_attr_getstacksize.cpp | ||
23 ↗ | (On Diff #515456) | Bah, you are right. the getstack* manpage re-iterates that setstack* can fail. Misread as getstack*. Sorry. |
libc/src/pthread/pthread_attr_init.cpp | ||
21 | Done for all but PTHREAD_CREATE_JOINABLE because I believe that one we have no choice about. | |
libc/src/pthread/pthread_create.cpp | ||
47 |
My feeling is code like this which is far far far from any critical path is best future-proofed. | |
59 | Sure. Generally my feeling was its not worth the clutter in far from critical path code but annotated all the known-to-never fail cases here. |
Few more minor comments, but overall LGTM now.
libc/src/__support/threads/linux/thread.cpp | ||
---|---|---|
502 | This is a good catch! What do you think of just exiting with a return code of say -1 if the thread is detached? We can take this up separately. | |
libc/src/pthread/pthread_attr_setstack.cpp | ||
32 | We avoid calling one entrypoint from another. So, we should keep this as before. | |
libc/src/threads/thrd_create.cpp | ||
14 | This is there for IWYU which we try to enforce in general. But, few parts of the libc have likely regressed. | |
libc/test/integration/src/pthread/pthread_create_test.cpp | ||
40 | General style fixes for this file:
| |
62 | I am a bit surprised this is not leading to a build failure. We should do an equality test here: ASSERT_EQ(__llvm_libc::pthread_attr_getstack(...), 0); Same with other similar checks below. | |
219 | Change the "person" of your comments, here and elsewhere are necessary, by using "we" instead of "I". | |
286 | You can add such tests which are reasonable separately. |
libc/src/__support/threads/linux/thread.cpp | ||
---|---|---|
502 | Won't that be a massive memory leak? Although its not super clear how to handle this w.o some degree of inline-assembly. |
libc/src/__support/threads/linux/thread.cpp | ||
---|---|---|
502 | What I mean is, just add __llvm_libc::syscall_impl(SYS_exit, -1); after line 489 but before the end of the scope. |
libc/src/__support/threads/linux/thread.cpp | ||
---|---|---|
502 | Ahh, makes sense. I'll create follow up for that. |
Fix style/comments in tests.
Add tests for invalidly set parameters (and support in pthread_create).
Use STACK_ALIGNMENT variable instead of magic number 16 throughout.
Don't use pthread_set_stacksize in pthread_set_stack, just inline impl
This seems to be causing build-bot failures:
https://lab.llvm.org/buildbot/#/builders/223/builds/20528/steps/4/logs/stdio
FAILED: projects/libc/test/integration/src/pthread/libc.test.integration.src.pthread.pthread_create_test.__build__ : && /usr/bin/clang++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -g -nostdlib -static projects/libc/startup/linux/aarch64/CMakeFiles/libc.startup.linux.aarch64.crt1.dir/start.cpp.o projects/libc/test/IntegrationTest/CMakeFiles/libc.test.IntegrationTest.test.dir/test.cpp.o projects/libc/test/integration/src/pthread/CMakeFiles/libc.test.integration.src.pthread.pthread_create_test.__build__.dir/pthread_create_test.cpp.o -o projects/libc/test/integration/src/pthread/libc.test.integration.src.pthread.pthread_create_test.__build__ projects/libc/test/integration/src/pthread/liblibc.test.integration.src.pthread.pthread_create_test.libc.a && : /usr/bin/ld: projects/libc/test/integration/src/pthread/CMakeFiles/libc.test.integration.src.pthread.pthread_create_test.__build__.dir/pthread_create_test.cpp.o: in function `__clang_call_terminate': pthread_create_test.cpp:(.text.__clang_call_terminate[__clang_call_terminate]+0x4): undefined reference to `__cxa_begin_catch' /usr/bin/ld: pthread_create_test.cpp:(.text.__clang_call_terminate[__clang_call_terminate]+0x8): undefined reference to `std::terminate()' /usr/bin/ld: projects/libc/test/integration/src/pthread/CMakeFiles/libc.test.integration.src.pthread.pthread_create_test.__build__.dir/pthread_create_test.cpp.o:(.data.DW.ref.__gxx_personality_v0[DW.ref.__gxx_personality_v0]+0x0): undefined reference to `__gxx_personality_v0'
@sivachandra do you know whats missing? I'm unable to repro this locally.
Will revert if don't hear back in a few hours.
I have landed a fix: https://reviews.llvm.org/D151158
There were two problems - 1. The integration tests were being built without -fno-exceptions, 2. On the aarch64 builder, the page size is 64KB which was way bigger than the 16KB stack min.
You already check for overflow isn't it? So, do we need to keep this TODO? This pattern repeats at many places in this patch.