[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:24
+#include "sanitizer_internal_defs.h"
+#include "sanitizer_atomic.h"
+#include "sanitizer_common.h"
----------------
vitalybuka wrote:
> includes are not ordered
Oops. Good catch. Fixed.


================
Comment at: lib/sanitizer_common/sanitizer_remote_address_space_view.h:32
+ private:
+  static atomic_uintptr_t vmrc_;
+  static atomic_uint64_t owning_tid_;
----------------
vitalybuka wrote:
> Why did you obfuscate context type?
> There is not point in atomic if you are going to use single thread
> maybe OK for owning_tid_ but we will crash immediately if this does not match
Originally I did that because I was going to make this a `DCHECK` and I didn't want it to be possible for the pointer to change non-atomically. You're right though. Now that the `CHECK` happens in all builds I should just use `VMContext*`.

I've fixed this now.


================
Comment at: lib/sanitizer_common/sanitizer_remote_address_space_view.h:50
+  static void set_context(VMReadContext* ctx) {
+    atomic_store_relaxed(&owning_tid_, GetTid());
+    atomic_store_relaxed(&vmrc_, reinterpret_cast<uptr>(ctx));
----------------
vitalybuka wrote:
> probably you should check if owning_tid_ is already set that they match
That would require that I guarantee that `owning_tid` is `0` initially (i.e. before `RemoteAddressSpaceView` is ever used). I think this might already be the case because `atomic_uint64_t RemoteAddressSpaceView::owning_tid` is a global but I wasn't 100% sure. I wasn't sure how I was supposed to write an initializer for the global in `sanitizer_remote_address_space_view.cc` so I just left it out. 

If we can confidently assume that `owning_tid_` is zero initialized then I'll add the check.

I've gone ahead and made the fix. Seems to work.


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:42
+
+  VMReadContext(const VMReadContext&) = delete;
+
----------------
vitalybuka wrote:
> delcypher wrote:
> > vitalybuka wrote:
> > > why copying is not acceptable?
> > Because a `VMReadContext` object is supposed to manage the lifetime of memory it copies from another. The implementation might hold data structures to manage this memory and so copying a `VMReadContext` object is almost certainly a mistake.
> > 
> > For the Darwin implementation  `VMReadContext` delegates this task to a Darwin specific framework that guarantees the lifetime of the copied memory will be longer than its client (`VMReadContext`). So technically nothing would go wrong right now if the `VMReadContext` copy constructor was called. However calling the `VMReadContext` is almost certainly a mistake and so it should be disallowed.
> "operator=" ?
That would also be bad. I've deleted that and also the move variants.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D56206