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

[Dan Liew via Phabricator via llvm-commits]

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


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:73
+  // process in which it is being called from.
+  bool IsLocal() const { return is_local; }
+
----------------
vitalybuka wrote:
> delcypher wrote:
> > vitalybuka wrote:
> > > If you can calculate it from target_process please remove member and simplify state
> > We cannot calculate it. It needs to be part of the sate.
> >> We cannot calculate it. It needs to be part of the sate.
> I read this as:
> ```
> bool IsLocal() const {
>   return config_ == nullptr || mach_task_self() == target_process_;
> }
> 
> ```
>   bool IsLocal() const {
>    return config_ == nullptr || mach_task_self() == target_process_;
>   }

Oh I see. I thought you where talking about `GetTargetProcess()`. However in this latest revision `IsLocal()` has actually been removed because my  internal implementation never called it.

I can probably bring it back though.


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


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


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context_mac.cc:28
+VMReadContext::VMReadContext(void* config, ProcessHandle target_process)
+    : config_(config), is_local_(false), target_process_(target_process) {
+  if (config_ == nullptr || mach_task_self() == target_process) {
----------------
vitalybuka wrote:
> can you remove "config" argument and just initialize config_ to darwin specific stuff here?
No. On Darwin `config` is a function pointer but on other platforms it can be anything (that's why it has type `void*`) that's needed to get access to the target process's memory. The `config` argument must be there to provide a consistent and flexible interface on all platforms.

There is no special function we can call from inside `VMReadContext` to get the function pointer. The client creating the `VMReadContext` on Darwin knows the value of the pointer and must pass it in.


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context_mac.cc:29
+    : config_(config), is_local_(false), target_process_(target_process) {
+  if (config_ == nullptr || mach_task_self() == target_process) {
+    is_local_ = true;
----------------
vitalybuka wrote:
> if we keep it please remove {} for consistency 
Ok. I'll fix this.


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


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D56206