[libcxx-commits] [PATCH] D145183: [libc++] Implement `stop_token`

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

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.



================
Comment at: libcxx/docs/Status/Cxx20Papers.csv:107
 "`P0645R10 <https://wg21.link/P0645R10>`__","LWG","Text Formatting","Cologne","|Complete| [#note-P0645]_","14.0"
-"`P0660R10 <https://wg21.link/P0660R10>`__","LWG","Stop Token and Joining Thread, Rev 10","Cologne","",""
+"`P0660R10 <https://wg21.link/P0660R10>`__","LWG","Stop Token and Joining Thread, Rev 10","Cologne","In Progress",""
 "`P0784R7 <https://wg21.link/P0784R7>`__","CWG","More constexpr containers","Cologne","|Complete|","12.0"
----------------
Mordante wrote:
> 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.
Since you implement this feature using eelis instead of a paper you get 
LWG3254 (https://wg21.link/LWG3254 Strike stop_token's operator!=) for free, can you mark this issue as done too?


================
Comment at: libcxx/docs/Status/Cxx20Papers.csv:107
 "`P0645R10 <https://wg21.link/P0645R10>`__","LWG","Text Formatting","Cologne","|Complete| [#note-P0645]_","14.0"
-"`P0660R10 <https://wg21.link/P0660R10>`__","LWG","Stop Token and Joining Thread, Rev 10","Cologne","",""
+"`P0660R10 <https://wg21.link/P0660R10>`__","LWG","Stop Token and Joining Thread, Rev 10","Cologne","In Progress",""
 "`P0784R7 <https://wg21.link/P0784R7>`__","CWG","More constexpr containers","Cologne","|Complete|","12.0"
----------------
Mordante wrote:
> Mordante wrote:
> > 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.
> Since you implement this feature using eelis instead of a paper you get 
> LWG3254 (https://wg21.link/LWG3254 Strike stop_token's operator!=) for free, can you mark this issue as done too?
Can you use a not to mark the status. Look at the line above P0645 for an example.


================
Comment at: libcxx/include/__stop_token/stop_callback.h:87
+
+private:
+  friend __stop_callback_base;
----------------
We're already in the private section.


================
Comment at: libcxx/include/__stop_token/stop_state.h:40
+
+  static constexpr uint32_t __stop_requested_bit        = 1;
+  static constexpr uint32_t __locked_bit                = 1 << 1;
----------------
Can you add more documentation regarding what these fields mean.

Looking at the loop in 
`LIBCPP_HIDE_FROM_ABI bool __add_callback(__stop_callback_base* __cb) noexcept`
I see several flag tested and the lock is the latest to be tested.

I would expect the lock the first to be tested.



================
Comment at: libcxx/include/__stop_token/stop_state.h:85
+    _LIBCPP_HIDE_FROM_ABI void __lock() noexcept {
+      auto __current_state = __state_.load(std::memory_order_relaxed);
+      do {
----------------
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.)


================
Comment at: libcxx/include/__stop_token/stop_state.h:184
+  _LIBCPP_HIDE_FROM_ABI bool __add_callback(__stop_callback_base* __cb) noexcept {
+    auto __current_state = __state_.load(std::memory_order_relaxed);
+    do {
----------------
Can you explain why this relaxed it the right order?


================
Comment at: libcxx/include/__stop_token/stop_state.h:189
+          // already stop requested, synchronously run the callback
+          __cb->__invoke();
+          return false;
----------------
How do you prevent calling the callback multiple times.


================
Comment at: libcxx/include/__stop_token/stop_state.h:296
+
+  _LIBCPP_HIDE_FROM_ABI __stop_state* get() const noexcept { return __stop_state_; }
+
----------------



================
Comment at: libcxx/include/__stop_token/stop_state.h:311
+  _LIBCPP_HIDE_FROM_ABI void __increment_ref_count(__stop_state* __state) {
+    __state->__ref_count_.fetch_add(1, std::memory_order_relaxed);
+  }
----------------
Note my atomics knowledge is a bit rusty.
Why is this `memory_order_relaxed` instead of `memory_order_release`?

AFAIK the current increment may not be visible in a different thread doing a decrement.
Which may cause use-after-free and double-free issues of the __state.


================
Comment at: libcxx/include/stop_token:34-37
+#include <__config>
+#include <__stop_token/stop_callback.h>
+#include <__stop_token/stop_source.h>
+#include <__stop_token/stop_token.h>
----------------
I assume the feature will get a FTM once done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145183