Implement stop_token
http://eel.is/c++draft/thread.stoptoken
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Time | Test | |
---|---|---|
4,610 ms | libcxx CI ASAN > llvm-libc++-shared-cfg-in.std/thread/thread_stoptoken/stopcallback::dtor.pass.cpp Script:
--
: 'COMPILED WITH'; /usr/bin/clang++-17 /home/libcxx-builder/.buildkite-agent/builds/a8cf9f586a97-1/llvm-project/libcxx-ci/libcxx/test/std/thread/thread.stoptoken/stopcallback/dtor.pass.cpp --target=x86_64-unknown-linux-gnu -g -fno-omit-frame-pointer -fsanitize=address -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/a8cf9f586a97-1/llvm-project/libcxx-ci/build/generic-asan/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/a8cf9f586a97-1/llvm-project/libcxx-ci/build/generic-asan/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/a8cf9f586a97-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -lc++experimental -nostdlib++ -L /home/libcxx-builder/.buildkite-agent/builds/a8cf9f586a97-1/llvm-project/libcxx-ci/build/generic-asan/lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/a8cf9f586a97-1/llvm-project/libcxx-ci/build/generic-asan/lib -lc++ -pthread -o /home/libcxx-builder/.buildkite-agent/builds/a8cf9f586a97-1/llvm-project/libcxx-ci/build/generic-asan/test/std/thread/thread.stoptoken/stopcallback/Output/dtor.pass.cpp.dir/t.tmp.exe
| |
3,450 ms | libcxx CI Apple back-deployment macosx10.15 > apple-libc++-backdeployment-cfg-in.libcxx::double_include.sh.cpp Script:
--
: 'RUN: at line 11'; /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -c /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/libcxx/test/libcxx/double_include.sh.cpp -o /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.15/test/libcxx/Output/double_include.sh.cpp.dir/t.tmp.first.o -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.3.sdk --target=x86_64-apple-macosx10.15 -nostdinc++ -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.15/include/c++/v1 -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Werror=thread-safety -Wuser-defined-warnings
| |
6,150 ms | libcxx CI Apple back-deployment macosx10.15 > apple-libc++-backdeployment-cfg-in.libcxx::min_max_macros.compile.pass.cpp Script:
--
: 'COMPILED WITH'; /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/libcxx/test/libcxx/min_max_macros.compile.pass.cpp -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.3.sdk --target=x86_64-apple-macosx10.15 -nostdinc++ -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.15/include/c++/v1 -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Werror=thread-safety -Wuser-defined-warnings -fsyntax-only
| |
124,620 ms | libcxx CI Apple back-deployment macosx10.15 > apple-libc++-backdeployment-cfg-in.libcxx::modules_include.sh.cpp Script:
--
: 'RUN: at line 58'; echo "" > /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.15/test/libcxx/Output/modules_include.sh.cpp.dir/t.tmp.sh
| |
6,800 ms | libcxx CI Apple back-deployment macosx10.15 > apple-libc++-backdeployment-cfg-in.libcxx::nasty_macros.compile.pass.cpp Script:
--
: 'COMPILED WITH'; /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/libcxx/test/libcxx/nasty_macros.compile.pass.cpp -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.3.sdk --target=x86_64-apple-macosx10.15 -nostdinc++ -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/build/apple-system-backdeployment-10.15/include/c++/v1 -I /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/y10-8-macminivault-com/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Werror=thread-safety -Wuser-defined-warnings -fsyntax-only
| |
View Full Test Results (25 Failed) |
Event Timeline
I mainly glanced over the patch and didn't look at the wording.
libcxx/docs/Status/Cxx20Papers.csv | ||
---|---|---|
107 | After this patch it's partial :-) Can you document which parts are still missing? We have multiple partial done papers, without any information what is and what is not done. That makes it hard to finish these papers. | |
libcxx/include/__stop_token/stop_source.h | ||
34 | I miss _LIBCPP_HIDE_FROM_ABI on most (all?) functions. | |
libcxx/include/__stop_token/stop_state.h | ||
48 | maybe move constants __stop_requested_bit here too, that makes it clear they belong together. | |
191 | Did you consider using RAII based locking? |
Geez, this is more complicated than I expected. Thanks a lot for working on this, we'll get through the whole thing.
libcxx/include/__stop_token/stop_state.h | ||
---|---|---|
54 | Would it make sense to try to extract this into a more general utility? It's really tricky to review, as we both just discussed it for a while, and I think trying to extract it into its own utility would help us make gradual progress on the review. | |
65–66 | Are we ever calling this with __is_locked == false? If so, it seems like this class is kind of like an optional<__lock_guard>. I would have expected that this constructor instead takes something like a __already_locked_t tag to express that the lock has already been taken. | |
131 | Maybe we should look into extracting the intrusive linked-list related functionality into something reusable? Would that be doable, and if so would it make the code simpler? |
Thanks a lot for working on this!
I looked at the header code and did some reviewing, but not completely.
This code is quite hard to understand and the Standard gives implementers a lot of freedom how to implement this. So I really would like more documentation regarding the design and some of the libc++ specific pre and post conditions. Without this information it will very hard to review and maintain this code.
libcxx/docs/Status/Cxx20Papers.csv | ||
---|---|---|
107 | Since you implement this feature using eelis instead of a paper you get | |
107 | Can you use a not to mark the status. Look at the line above P0645 for an example. | |
libcxx/include/__stop_token/stop_callback.h | ||
88 | We're already in the private section. | |
libcxx/include/__stop_token/stop_state.h | ||
41 | Can you add more documentation regarding what these fields mean. Looking at the loop in I would expect the lock the first to be tested. | |
86 | Please don't use auto. This makes it hard to review the code or use it without and IDE. I need to look at load to see the type. (Note that the LLVM coding style also discourage auto.) | |
185 | Can you explain why this relaxed it the right order? | |
190 | How do you prevent calling the callback multiple times. | |
297 | ||
312 | Note my atomics knowledge is a bit rusty. AFAIK the current increment may not be visible in a different thread doing a decrement. | |
libcxx/include/stop_token | ||
35–38 | I assume the feature will get a FTM once done. |
libcxx/include/__stop_token/stop_state.h | ||
---|---|---|
41 | yeah I will refactor/rename the code and add more comments. the lock bit is for locking the linked-list. The reason why it was not locked first is that the spec specified that if the requested bit was set, we won't go through the linked-list and instead directly invoke the callback on the current thread. | |
185 | will add comments. all the atomic operations here won't need any memory ordering except the one actually does the lock. | |
190 | once it is called, the next line would return. | |
312 | Note that this is the same case for shared_ptr where increment is relaxed and decrement is acq_rel The memory ordering controls the visibility of the write operations (including non-atomic operations) prio to current atomic operation, and read operations after the current atomic operation. For increment, we don't need to do anything before and afterwards, so memory_order_relaxed is enough. We need release to tell other threads that we are done with the underlying object and all the writes to the underlying object should be visible now. (suppose you are writing to the underlying object before the shared_ptr goes out of scope, so that in case a different thread which has the last shared_ptr calls the destructor, it needs to see this write) The way I understand is that: please also note that shared_ptr (similarly here) does not protect you from incorrect use. When used properly, the copy operations would either copy from a nullptr or a shared_ptr which ref count is at least 1.. if you invoke a copy constructor and spawn a thread to assign the copied-from instance to nullptr. it is plain UB because you are writing and reading to the same shared_ptr object (data race) | |
libcxx/include/stop_token | ||
35–38 | yes there is only one FTM for stop_token and jthread. will get the FTM once jthread is done |
libcxx/include/__stop_token/stop_state.h | ||
---|---|---|
312 | Adding to my previous explanation. I think you might mix the visibility of other operations prior/after the atomic operations, with the visibility of the atomic operation itself. I think we never talk about the visibility of atomic operation itself when we talk about memory_order. The question of memory_order is, when atomic load sees another atomic store, what other operations are guaranteed to be seen. for example int a = 0; atomic<int> b = 0; // thread A a = 5; b.store(3, std::memory_order_release); // thread B auto tmp = b.load(std::memory_order_acquire); assert(tmp!=3 || a==5); what the memory ordering concerns is that when thread B loads b and the value is 3, the release/acquire memory order ensures that thread B can also see that a has value 5. |
Thanks for the additional information and the additional documentation. I see the CI is still red, so I prefer to review it with a green CI. (Especially the thread sanitizer.) Feel free to ping me when the CI is green.
libcxx/include/__stop_token/stop_state.h | ||
---|---|---|
312 | Thanks for the additional information. I also reread some parts of Anthony Williams book. |