Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[libc++] implement std::`jthread`

Authored by huixie90 on May 26 2023, 6:06 AM.


Group Reviewers
Restricted Project
rG695138ca8405: [libc++] implement std::`jthread`

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ldionne added inline comments.Jun 2 2023, 9:19 AM

To increase the confidence in this test, we could instead spawn more threads and do the same check. That should get the probability that join() isn't called but the test still passes down.

Also, I'd leave a short comment behind explaining this particularity of the test (i.e. it's not going to catch issues 100%).


Same comment here about non-determinism.

ldionne published this revision for review.Jun 2 2023, 9:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2023, 9:19 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I didn't do an in depth review, I mainly looked at the code out of curiosity.

huixie90 updated this revision to Diff 532615.Jun 19 2023, 5:33 AM
huixie90 marked 7 inline comments as done.

add tests

huixie90 updated this revision to Diff 532616.Jun 19 2023, 5:38 AM

update <thread>

huixie90 updated this revision to Diff 533318.Jun 21 2023, 10:11 AM

export jthread

ldionne added inline comments.

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 ( (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 (
    #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 ( (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
    #10 __libc_start_main csu/../csu/libc-start.c:392:3 ( (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.

huixie90 added inline comments.Jun 30 2023, 12:51 PM

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);
  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?

ldionne commandeered this revision.Jul 7 2023, 6:16 AM
ldionne added a reviewer: huixie90.

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


Let's mark this as experimental.


This seems like we should file a bug report against ASAN for this one.


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.

huixie90 commandeered this revision.Wed, Sep 13, 2:20 AM
huixie90 edited reviewers, added: ldionne; removed: huixie90.
huixie90 updated this revision to Diff 556651.Wed, Sep 13, 4:35 AM

put jthread in experimental

huixie90 updated this revision to Diff 556654.Wed, Sep 13, 5:00 AM


huixie90 updated this revision to Diff 556659.Wed, Sep 13, 6:34 AM

transitive includes

huixie90 updated this revision to Diff 556691.Wed, Sep 13, 11:33 AM

transitive includes

huixie90 updated this revision to Diff 556713.Wed, Sep 13, 1:21 PM
huixie90 marked an inline comment as done.

update module map and all tests guard against experimental stop_token macro

huixie90 updated this revision to Diff 556807.Thu, Sep 14, 1:04 PM


huixie90 updated this revision to Diff 556829.Thu, Sep 14, 11:32 PM

filter platforms

ldionne accepted this revision.Fri, Sep 15, 9:33 AM

I don't think any of my comments really require me to take another look. This is really good, thank you!


Not attached: let's mention this in UsingLibcxx.rst where we have a list of experimental features.


I think you can simply replace this by __thread/id.h now that it has been split out of __thread/thread.h. This might reduce dependencies a bit.


I would remove this since there's already one in <thread>. We want to revisit how we're handling missing capabilities like this (in some cases we just omit the contents of headers, in other cases like _LIBCPP_HAS_NO_THREADS we have a hard error). Removing this doesn't change anything but it avoids giving the impression that this is the definite way we want to handle those.


I might suggest making __init_thread a static member function and passing __stop_source_ to it explicitly. Otherwise we're calling a member function at a point where the object isn't fully initialized yet, which seems questionable. I don't think there's any real issue here, it's more a matter of style / clarity.


Let's add a link to in this comment.

Also, if you don't have a concrete idea of how this might be done (portably etc..), let's not make this a TODO. If we don't realistically plan on fixing it, it shouldn't be a TODO.

We can say instead:

// Per
// Throws: system_error if unable to start the new thread.
// Error conditions:
// resource_unavailable_try_again - the system lacked the necessary resources to create another thread,
// or the system-imposed limit on the number of threads in a process would be exceeded.
// Unfortunately, this is extremely hard to test portably so we don't have a test for this error condition right now.
35 says that j1.get_id() should be id(), so we might as well assert that.


To troubleshoot whether the CI issue is that this curly brace is taking forever to complete and then the main thread finishes before the detached thread is done, you could insert a sleep at the end of the test. If that is indeed the issue, I can't think of another way to solve this problem since you don't have any handle to the thread once you've detached it. It's kind of lame but it might be acceptable to insert a short sleep with a comment at the end if it reliably solves this issue.




This seems a bit simpler. This applies in a few other tests as well.


That way if we returned a reference, the test would fail. You probably want to do this in the other tests too.


This should be a link to the github issue!


We could do UNSUPPORTED: no-exceptions instead.


I think we should check two cases here:

  1. The thread of execution has finished running before we call jt.joinable(), and
  2. the thread of execution has not finished running yet before we call jt.joinable().

You can use an atomic variable to signal that.

1 ↗(On Diff #556829)

I would suggest renaming this to types.compile.pass.cpp, I think it is a bit more consistent with the rest of the test suite.

This revision is now accepted and ready to land.Fri, Sep 15, 9:33 AM
huixie90 updated this revision to Diff 556874.Fri, Sep 15, 11:58 AM
huixie90 marked 18 inline comments as done.

addressed feedback

vitalybuka added inline comments.Fri, Sep 15, 12:39 PM

This seems like we should file a bug report against ASAN for this one.


Asan for similar case make sure result is success

INTERCEPTOR(int, pthread_join, void *thread, void **retval) {
  int result;
  asanThreadArgRetval().Join((uptr)thread, [&]() {
    result = REAL(pthread_join)(thread, retval);
    return !result;
  return result;