Depends on D75379
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
2,190 ms | Clang.Driver::Unknown Unit Message ("") |
Event Timeline
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 | ||
18 | You can remove arg and just have thread_func(void *)to avoid any unused variable warnings. | |
24 | for (counter = 0; counter <= 1000; ) { | |
47 | Make this thread_list + i or make args + i &args[i]. | |
52 | 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.
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. |
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 | ||
2 | Should this be in a linux specific directory for tests? |
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 | ||
2 | Indeed! Moved to the higher level directory now. |
LGTM
libc/src/threads/linux/thrd_create.cpp | ||
---|---|---|
28 |
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. |
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. |
Make this [[noreturn]]