bulbazord added inline comments.

================
Comment at: lldb/source/Host/posix/PipePosix.cpp:69
   PipeBase::operator=(std::move(pipe_posix));
-  m_fds[READ] = pipe_posix.ReleaseReadFileDescriptor();
-  m_fds[WRITE] = pipe_posix.ReleaseWriteFileDescriptor();
+  std::lock_guard<std::mutex> guard(m_lock);
+  m_fds[READ] = pipe_posix.ReleaseReadFileDescriptorUnlocked();
----------------
I think this lock needs to be at the beginning. Scenario:

Thread 1 calls the move assign operator and performs the base move assign. Gets 
unscheduled before acquiring lock.
Thread 2 calls the move assign operator and performs the base move assign. 
Acquires the lock, fills in `m_fds`, and returns.
Thread 1 is scheduled again, grabs the lock, and fills in `m_fds`.
`this` will have its `PipeBase` members filled in by the Thread 2 call while 
`m_fds` will be filled in by the Thread 1 call. Granted, one thread may be 
surprised that suddenly the thing didn't get moved in as expected, but at least 
`this` will be in a consistent state.

I think you also need to lock the mutex of `pipe_posix` at the same time. 
Imagine Thread 1 is trying to access something from `pipe_posix` while Thread 2 
is trying to move it into another `PipePosix` object. (Not sure how likely this 
scenario is but I'd rather be safe than sorry).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157654

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to