[libcxx-commits] [PATCH] D151559: [libc++] implement std::`jthread`

[Louis Dionne via Phabricator via libcxx-commits]

ldionne commandeered this revision.
ldionne added a reviewer: huixie90.
ldionne added a comment.

I'll pick up that patch as well per discussion with Hui.



================
Comment at: libcxx/include/__thread/jthread.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
Let's mark this as experimental.


================
Comment at: libcxx/test/std/thread/thread.jthread/join.pass.cpp:102
+      } catch (const std::system_error& err) {
+        assert(err.code() == std::errc::resource_deadlock_would_occur);
+      }
----------------
Let's move this to a different test like `join.deadlock.pass.cpp` and mark it as UNSUPPORTED on Windows with a comment. We can also mark it as UNSUPPORTED on ASAN with a link to the bug report.


================
Comment at: libcxx/test/std/thread/thread.jthread/join.pass.cpp:84
+
+  // resource_deadlock_would_occur - if deadlock is detected or get_id() == this_thread::get_id().
+  {
----------------
huixie90 wrote:
> ldionne wrote:
> > This test is failing with:
> > 
> > ```
> > ThreadSanitizer: CHECK failed: sanitizer_thread_registry.cpp:348 "((t)) != (0)" (0x0, 0x0) (tid=3411214)
> >     #0 __tsan::CheckUnwind() <null> (t.tmp.exe+0xcbd1b) (BuildId: d7fd9285e8961410872ba6334aecc0115d0ff192)
> >     #1 __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) <null> (t.tmp.exe+0x45042) (BuildId: d7fd9285e8961410872ba6334aecc0115d0ff192)
> >     #2 __sanitizer::ThreadRegistry::ConsumeThreadUserId(unsigned long) <null> (t.tmp.exe+0x43e94) (BuildId: d7fd9285e8961410872ba6334aecc0115d0ff192)
> >     #3 pthread_join <null> (t.tmp.exe+0x604bb) (BuildId: d7fd9285e8961410872ba6334aecc0115d0ff192)
> >     #4 std::__1::__libcpp_thread_join[abi:v170000](unsigned long*) /home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-69f521df8409-1/llvm-project/libcxx-ci/build/generic-tsan/include/c++/v1/__threading_support:398:10 (libc++.so.1+0x73358) (BuildId: 71b8f06279b5e8117756a82c1e642e312c2a0e30)
> >     #5 std::__1::thread::join() /home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-69f521df8409-1/llvm-project/libcxx-ci/libcxx/src/thread.cpp:51:14 (libc++.so.1+0x73358)
> >     #6 std::__1::jthread::join[abi:v170000]() /home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-69f521df8409-1/llvm-project/libcxx-ci/build/generic-tsan/include/c++/v1/__thread/jthread.h:91:49 (t.tmp.exe+0xed5f9) (BuildId: d7fd9285e8961410872ba6334aecc0115d0ff192)
> >     #7 std::__1::jthread::~jthread[abi:v170000]() /home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-69f521df8409-1/llvm-project/libcxx-ci/build/generic-tsan/include/c++/v1/__thread/jthread.h:60:7 (t.tmp.exe+0xed708) (BuildId: d7fd9285e8961410872ba6334aecc0115d0ff192)
> >     #8 main /home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-69f521df8409-1/llvm-project/libcxx-ci/libcxx/test/std/thread/thread.jthread/join.pass.cpp:108:3 (t.tmp.exe+0xe836d) (BuildId: d7fd9285e8961410872ba6334aecc0115d0ff192)
> >     #9 __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16 (libc.so.6+0x29d8f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
> >     #10 __libc_start_main csu/../csu/libc-start.c:392:3 (libc.so.6+0x29e3f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
> >     #11 _start <null> (t.tmp.exe+0x30484) (BuildId: d7fd9285e8961410872ba6334aecc0115d0ff192)
> > ```
> > 
> > What might be happening is that TSAN instruments `pthread_join` and it thinks that the thread has already been joined when you call it the second time in the destructor. But in reality, the thread wasn't joined the first time around because of the system error. You might be able to figure out what's going on by looking at how these functions are interposed, I think it's in compiler-rt. @vitalybuka Might be able to provide more information as well.
> > 
> > If that's the case, then the issue would be that TSAN's internal state is inconsistent with reality and the fix would be in TSAN. I'm not sure, I'm just guessing.
> had a looked at TSAN's code
> 
> ``` 
> TSAN_INTERCEPTOR(int, pthread_join, void *th, void **ret) {
>   SCOPED_INTERCEPTOR_RAW(pthread_join, th, ret);
>   Tid tid = ThreadConsumeTid(thr, pc, (uptr)th);
>   ThreadIgnoreBegin(thr, pc);
>   int res = BLOCK_REAL(pthread_join)(th, ret);
>   ThreadIgnoreEnd(thr);
>   if (res == 0) {
>     ThreadJoin(thr, pc, tid);
>   }
>   return res;
> }
> ```
> 
> `ThreadConsumeTid` calls `ConsumeThreadUserId`, which
> 1. try to find by id, assert id is found
> 2. remove what is found
> 
> From what I can see, the first time join is called, the user id is removed. then join throws.
> The second time join is called, the user id is no longer there and assertion failure. 
> 
> so I guess this is a TSAN problem?
This seems like we should file a bug report against ASAN for this one.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151559/new/

https://reviews.llvm.org/D151559