This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add linux implementations of thrd_create and thrd_join functions.
ClosedPublic

Authored by sivachandra on Feb 28 2020, 1:00 PM.

Diff Detail

Event Timeline

sivachandra created this revision.Feb 28 2020, 1:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2020, 1:00 PM
abrachet added inline comments.Feb 28 2020, 6:56 PM
libc/src/threads/linux/thrd_create.cpp
28

Make this [[noreturn]]

66

Maybe (u)intptr_t is a more descriptive type to use than long?

69

unnecessary brackets

libc/src/threads/linux/thrd_join.cpp
22

I don't think this is used anywhere.

28

Is it safe to do while (atomic_load(clear_tid_address))?

libc/src/threads/linux/thread_utils.h
17–19

These can be constexpr

libc/test/src/threads/linux/thrd_test.cpp
17 ↗(On Diff #247352)

You can remove arg and just have thread_func(void *)to avoid any unused variable warnings.

23 ↗(On Diff #247352)

for (counter = 0; counter <= 1000; ) {
Even if you would rather keep the current loop, I think it is important to set counter to 0 in case we ever change the tester to run tests in a different order or run them multiple times whatever the case may be.

46 ↗(On Diff #247352)

Make this thread_list + i or make args + i &args[i].

51 ↗(On Diff #247352)

Might as well choose a value greater than thread_count

I am still of the feeling that we should implement pthread first, then the C11 thread support library is just a wrapper over pthread. Doing C thread first, then we will hit inevitable limitation when implementing pthread on top of C11 thread.

sivachandra marked 10 inline comments as done.

Address comments.

sivachandra added inline comments.Mar 2 2020, 11:58 AM
libc/src/threads/linux/thrd_create.cpp
28

Sanitizers blow up at this point. I do not understand sanitizers enough yet to explain why. I will be sharing a more holistic treatment of sanitizers in LLVM-libc soon so I will try to include talking about this there.

FWIW, I have shared this problem with the sanitizer experts I have access to and they are investigating.

libc/src/threads/linux/thrd_join.cpp
28

Should be fine so changed it to that.

libc/src/threads/linux/thread_utils.h
17–19

I created a struct with static members for this.

abrachet added inline comments.Mar 2 2020, 7:02 PM
libc/src/threads/linux/thrd_create.cpp
28

I have tried this and it is failing for me too, on a clang and compiler-rt built a few days ago. If I had to take a guess, and it really is just that, I've gone through it with lldb a couple of times and played around a bit, is that we aren't setting up tls right now which is messing with asan when it calls __asan_handle_no_return. If you try this with pthread_create or std::thread it works fine. If you make start_thread and thrd_create __attribute__((no_sanitize("undefined"), no_sanitize("address")) it also obviously stops the crashes.

I tried making the function we pass to thrd_create make calls to asan by making memory reads (__asan_report_store) etc, it doesn't crash but perhaps future tests might.

libc/test/src/threads/linux/thrd_test.cpp
1 ↗(On Diff #247352)

Should this be in a linux specific directory for tests?

  • Move thrd_test to a platform independent area.
sivachandra marked 2 inline comments as done.Mar 2 2020, 11:46 PM
sivachandra added inline comments.
libc/src/threads/linux/thrd_create.cpp
28

Yes, TLS was the first suspect. However, if I add a dummy TLS, I still see the problem. More strangely though, on a sanitizer team member's machine, the test just passes as is. This makes me feel the problem is something which I do not understand fully yet. I do not think it should block us though.

libc/test/src/threads/linux/thrd_test.cpp
1 ↗(On Diff #247352)

Indeed! Moved to the higher level directory now.

abrachet accepted this revision.Mar 3 2020, 12:30 AM

LGTM

libc/src/threads/linux/thrd_create.cpp
28

I do not think it should block us though.

Agreed. If there is a problem it will present itself later and right now it isn't clear that there is a problem at all.

This revision is now accepted and ready to land.Mar 3 2020, 12:30 AM
phosek accepted this revision.Mar 5 2020, 11:45 AM

LGTM

libc/src/threads/linux/thrd_create.cpp
51

Nit: this might be more readable as a ternary expression.

66

reinterpret_cast<uintptr_t>(stack) instead of C-style cast.

72

Could this be else if (clone_result < 0)? clone_result cannot be == 0 and < 0 at the same time so those cases are disjoint.

74

Nit: the same here, ternary expression would be more readable.

sivachandra marked 4 inline comments as done.Mar 5 2020, 2:00 PM
This revision was automatically updated to reflect the committed changes.