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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

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.



================
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);
+  }
----------------
huixie90 wrote:
> huixie90 wrote:
> > Mordante wrote:
> > > 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.
> > Note that this is the same case for `shared_ptr` where increment is `relaxed` and decrement is `acq_rel`
> > https://github.com/llvm/llvm-project/blob/main/libcxx/include/__memory/shared_ptr.h#L105
> > 
> > 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.
> > For decrement, we are potentially evaluating the destructor of underlying type `T`. We need to make sure that there are no other threads that are still poking around the underlying object through a different `shared_ptr` instance. i.e. any operations on the underlying object on any thread should be "finished" before evaluating the destructor.
> > 
> > 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)
> > In case we are calling the destructor, we need `acquire` to see other threads write (see above).
> > So decrement needs both.
> > 
> > The way I understand is that:
> > `acquire` is `git pull`. All the read operations after it would see the latest.
> > `release` is `git push`. All the write operations before it would be visible by other threads which pulls.
> > 
> > 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) 
> 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.
> Whether or not thread B sees b is 3 is not memory order problem. It is just a timing issue, 
Thanks for the additional information. I also reread some parts of Anthony Williams book. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145183