This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

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

Diff Detail

Event Timeline

huixie90 created this revision.May 26 2023, 6:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2023, 6:06 AM
ldionne added inline comments.
libcxx/include/__thread/formatter.h
1 ↗(On Diff #526039)

Would it be possible to granularize these headers in a separate patch? That should be a quick NFC to review.

libcxx/include/__thread/jthread.h
11–12
42–43

Note to self: this is tested.

49

Let's make sure we have a test for this note:

[Note 1: This implies that any exceptions not thrown from the invocation of the copy of f will be thrown in the constructing thread, not the new thread. — end note]

(near https://eel.is/c++draft/thread.jthread.class#thread.jthread.cons-5)

Edit: We do already have a test!

72

Do you have a test for self-assignment?

Edit: Of course, you do.

133
libcxx/include/__thread/this_thread.h
10 ↗(On Diff #526039)

Same here and elsewhere when you do it in another patch.

libcxx/test/std/thread/thread.jthread/assign.move.pass.cpp
38

I think we don't have a test for the case where the source thread isn't joinable.

libcxx/test/std/thread/thread.jthread/cons.func.token.pass.cpp
112

TEST_HAS_NO_EXCEPTIONS

148

Per your suggestion, we might be able to test https://eel.is/c++draft/thread.jthread.class#thread.jthread.cons-8 by setting multiple threads' affinity to the same core. It might not be possible to test this in a robust way, and I think it would be reasonable not to.

huixie90 updated this revision to Diff 527830.Jun 2 2023, 6:06 AM
huixie90 marked 4 inline comments as done.

rebase on splitting <thread>

huixie90 updated this revision to Diff 527869.Jun 2 2023, 8:16 AM

more tests

huixie90 updated this revision to Diff 527871.Jun 2 2023, 8:18 AM

more tests

ldionne added inline comments.Jun 2 2023, 9:19 AM
libcxx/include/__stop_token/stop_state.h
18

#include <__thread/thread.h>? I think this might require exporting * or std.__threading_support.__thread_id from the modulemap.

libcxx/test/std/thread/thread.jthread/dtor.pass.cpp
24
30

assert(jt.joinable()); // test the test

Those are just suggestions, I'm actually not sure how much I like this.

ldionne added inline comments.Jun 2 2023, 9:19 AM
libcxx/test/std/thread/thread.jthread/dtor.pass.cpp
39–45

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%).

libcxx/test/std/thread/thread.jthread/join.pass.cpp
28

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.

libcxx/include/__thread/jthread.h
66
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.
libcxx/test/std/thread/thread.jthread/join.pass.cpp
85

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.

huixie90 added inline comments.Jun 30 2023, 12:51 PM
libcxx/test/std/thread/thread.jthread/join.pass.cpp
85

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?

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.

libcxx/include/__thread/jthread.h
2

Let's mark this as experimental.

libcxx/test/std/thread/thread.jthread/join.pass.cpp
85

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

103

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.Sep 13 2023, 2:20 AM
huixie90 edited reviewers, added: ldionne; removed: huixie90.
huixie90 updated this revision to Diff 556651.Sep 13 2023, 4:35 AM

put jthread in experimental

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

formatting

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

transitive includes

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

transitive includes

huixie90 updated this revision to Diff 556713.Sep 13 2023, 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.Sep 14 2023, 1:04 PM

formatting

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

filter platforms

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

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

libcxx/include/CMakeLists.txt
663

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

libcxx/include/__stop_token/stop_state.h
18

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.

libcxx/include/__thread/jthread.h
32–34

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.

52

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.

libcxx/test/std/thread/thread.jthread/cons.func.token.pass.cpp
151

Let's add a link to https://eel.is/c++draft/thread.jthread.class#thread.jthread.cons-8 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 https://eel.is/c++draft/thread.jthread.class#thread.jthread.cons-8:
// 
// 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.
libcxx/test/std/thread/thread.jthread/cons.move.pass.cpp
35

https://eel.is/c++draft/thread.jthread.class#thread.jthread.cons-10 says that j1.get_id() should be id(), so we might as well assert that.

libcxx/test/std/thread/thread.jthread/detach.pass.cpp
36 ↗(On Diff #556829)

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.

69–71 ↗(On Diff #556829)
libcxx/test/std/thread/thread.jthread/dtor.pass.cpp
63–64
libcxx/test/std/thread/thread.jthread/get_id.pass.cpp
23–28 ↗(On Diff #556829)

Suggestion:

static_assert(noexcept(std::declval<std::jthread>().get_id()));

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

libcxx/test/std/thread/thread.jthread/get_stop_token.pass.cpp
37 ↗(On Diff #556829)

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

libcxx/test/std/thread/thread.jthread/join.deadlock.pass.cpp
13 ↗(On Diff #556829)

This should be a link to the github issue!

36 ↗(On Diff #556829)

We could do UNSUPPORTED: no-exceptions instead.

libcxx/test/std/thread/thread.jthread/joinable.pass.cpp
40–45

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.

libcxx/test/std/thread/thread.jthread/typedef.compile.pass.cpp
2

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.Sep 15 2023, 9:33 AM
huixie90 updated this revision to Diff 556874.Sep 15 2023, 11:58 AM
huixie90 marked 18 inline comments as done.

addressed feedback

vitalybuka added inline comments.Sep 15 2023, 12:39 PM
libcxx/test/std/thread/thread.jthread/join.pass.cpp
85

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

TSAN

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;
}