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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka 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"
----------------
includes are not ordered


================
Comment at: lib/sanitizer_common/sanitizer_remote_address_space_view.h:32
+ private:
+  static atomic_uintptr_t vmrc_;
+  static atomic_uint64_t owning_tid_;
----------------
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


================
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));
----------------
probably you should check if owning_tid_ is already set that they match


================
Comment at: lib/sanitizer_common/sanitizer_remote_address_space_view.h:57
+    CHECK_EQ(atomic_load_relaxed(&owning_tid_), GetTid());
+    return reinterpret_cast<VMReadContext*>(atomic_load_relaxed(&vmrc_));
+  }
----------------
Please avoid reinterpret_cast, just return VMReadContext* as member


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:47
+  // Returns `nullptr` if no successful read has been performed.
+  void* GetLocalAddress() const { return (void*)local_address_; }
+
----------------
(void*) -> reinterpret_cast<>


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:30
+  bool is_local_ = false;
+  ProcessHandle target_process_;
+
----------------
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?


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:42
+
+  VMReadContext(const VMReadContext&) = delete;
+
----------------
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=" ?


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context_mac.cc:50
+  memory_reader_t* native_reader = (memory_reader_t*)config_;
+  kern_return_t result = native_reader(target_process_, target_address, size,
+                                       (void**)(&(local_address)));
----------------
delcypher wrote:
> vitalybuka wrote:
> > why do we need config_?
> > from here looks like you expect a particular function
> > could you call it directly here?
> No we cannot. It must be passed in via the `VMReadContext` constructor.
Instead of void* can you make it function pointer?
With type similar to memory_reader_t
If you make memory_reader_t return bool and handle error there, the only platform specific part of VMReadContext will be the callback.

also config_ -> native_reader_;


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D56206