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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: lib/sanitizer_common/CMakeLists.txt:45
+  sanitizer_vm_read_context_stubs.cc
+  sanitizer_win.cc)
 
----------------
Please return )


================
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);
----------------
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.
 


================
Comment at: lib/sanitizer_common/sanitizer_remote_address_space_view.h:57
+    if (vmrc_ == nullptr)
+      atomic_store_relaxed(&owning_tid_, 0);
+    else
----------------
Nothing prevents one thread to create ScopedContext and another thread to do LoadWritable. We need to check owning_tid_ in Load and LoadWritable



================
Comment at: lib/sanitizer_common/sanitizer_remote_address_space_view.h:102
+    }
+    ScopedContext() : vmrc() {
+      RemoteAddressSpaceView::set_context(&vmrc);
----------------
: vmrc() is not needed


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:30
+  bool is_local_ = false;
+  ProcessHandle target_process_;
+
----------------
vitalybuka wrote:
> delcypher wrote:
> > vitalybuka wrote:
> > > please init target_process_ to something target_process_ as well
> > Not doing inline initialisation here is deliberate. The type is platform specific so we might not be able to initialise it using an int. This is why the platform specific constructors do the initialisation.
> > 
> > Technically we could initialise to 0 now because `task_t` happens to be a typedef for `unsigned` on Darwin but this might not work for other platforms in the future.
> Maybe platform specific invalid_vm_read_handle near process_vm_read_handle_t?
> also process_vm_read_handle_t is too specific, why not just some task_id?
> Maybe platform specific invalid_vm_read_handle near process_vm_read_handle_t?
> also process_vm_read_handle_t is too specific, why not just some task_id?

I still think you should remove process_vm_read_handle_t from sanitizer_internal_defs.h and just define it in the Context. u64 everywhere, like for tid_t, is good enough for me.




================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context_mac.cc:56
+  if (config_ == nullptr || mach_task_self() == target_process_)
+    return true;
+  return false;
----------------
```
bool VMReadContext::IsLocal() const {
  return config_ == nullptr || mach_task_self() == target_process_;
}
```


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D56206