[PATCH] D56206: Introduce `RemoteAddressSpaceView` and `VMReadContext`.

[Dan Liew via Phabricator via llvm-commits]

delcypher marked 2 inline comments as done.
delcypher added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_remote_address_space_view.h:56
+    // Otherwise set the owning thread.
+    if (vmrc_ == nullptr)
+      atomic_store_relaxed(&owning_tid_, 0);
----------------
vitalybuka wrote:
> So data race is possible when one thread is releasing context and another is setting.
> For simplicity I think you should never reset owning_tid_
> As soon as one thread used it, no other allowed to interact with that.
> 
> Otherwise we need to design proper synchronization which is inconvenient with all these statics. E.g. as soon someone called Load and saved returned pointer
> we will need to make sure that caller release the pointers before letting other threads to interact with context.
> 
> And maybe it's not bad idea. But you have option to go with "never reset owning_tid_" now and switch to Load/Unload with mutexes later, when needed.
>  
> So data race is possible when one thread is releasing context and another is setting.
> For simplicity I think you should never reset owning_tid_
> As soon as one thread used it, no other allowed to interact with that.

I'm fine with that. Currently `RemoteAddressSpaceView` should only be used within a single-thread.

I'll update the patch as you suggest.


================
Comment at: lib/sanitizer_common/sanitizer_remote_address_space_view.h:57
+    if (vmrc_ == nullptr)
+      atomic_store_relaxed(&owning_tid_, 0);
+    else
----------------
vitalybuka wrote:
> Nothing prevents one thread to create ScopedContext and another thread to do LoadWritable. We need to check owning_tid_ in Load and LoadWritable
> 
Did I miss something? Won't the `CHECK_EQ(atomic_load_relaxed(&owning_tid_), current_tid)` fire inside `get_context()` fire due to `LoadWritable()` or `Load()` eventually triggering a call to `get_context()`?


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D56206