This is an archive of the discontinued LLVM Phabricator instance.

[LIBC] Support custom attributes in pthread_create
ClosedPublic

Authored by goldstein.w.n on Apr 13 2023, 8:03 PM.

Details

Summary

Only functional for stack growsdown (same as before), but custom
stack, stacksize, guardsize, and detachstate all should be
working.

Diff Detail

Event Timeline

goldstein.w.n created this revision.Apr 13 2023, 8:03 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 13 2023, 8:03 PM
goldstein.w.n requested review of this revision.Apr 13 2023, 8:03 PM

Use LLVM style

Fix some nits

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.

230

Nit: Because of the naming convention we use for constants, this should be named INTTERNAL_STACK_DATA_SIZE.

libc/src/__support/threads/thread.h
140

Nit: Naming style for all of these constants should be fixed.

libc/src/pthread/pthread_attr_getstacksize.cpp
23

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
46

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?

58
libc/src/threads/thrd_create.cpp
14–15

Do we need this?

goldstein.w.n marked 7 inline comments as done.
  1. Fix error check in pthread_attr_get* changes
  2. Remove todos that have already been addressed
  3. Use defaults from thread.h
  4. 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

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
46

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?

My feeling is code like this which is far far far from any critical path is best future-proofed.
But if you think its not worth it LMK and I'll drop.

58

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
492

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
29

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:

  • Member names should be in snake_case style.
  • Function and method names should be in snake_case style.
  • Variable names are in lower_case style.
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.

goldstein.w.n added inline comments.May 22 2023, 9:23 AM
libc/src/__support/threads/linux/thread.cpp
492

Won't that be a massive memory leak?

Although its not super clear how to handle this w.o some degree of inline-assembly.

sivachandra added inline comments.May 22 2023, 10:24 AM
libc/src/__support/threads/linux/thread.cpp
492

What I mean is, just add

__llvm_libc::syscall_impl(SYS_exit, -1);

after line 489 but before the end of the scope.

goldstein.w.n added inline comments.May 22 2023, 10:54 AM
libc/src/__support/threads/linux/thread.cpp
492

Ahh, makes sense. I'll create follow up for that.

goldstein.w.n retitled this revision from Support custom attributes in pthread_create to [LIBC] Support custom attributes in pthread_create.May 22 2023, 12:31 PM
goldstein.w.n marked 6 inline comments as done.May 22 2023, 12:40 PM

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

goldstein.w.n added inline comments.May 22 2023, 12:44 PM
libc/src/__support/threads/linux/thread.cpp
492

Made 0, is I think its all the same and we have some tests that pthread_exit returns success. But see: D151142

sivachandra accepted this revision.May 22 2023, 1:26 PM
This revision is now accepted and ready to land.May 22 2023, 1:26 PM
This revision was automatically updated to reflect the committed changes.

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.

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.

Ah, thank you :)