[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_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:
> 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_;
> 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.

I could but I'm strongly against that because.

* A function pointer isn't great. It can't hold any state. Even in Darwin's case I'd have to write a wrapper function that implements the callback interface. That function needs to know the address of the actual function to call. So the callback would need to be able to receive arbitrary data (i.e. passing a void*) so I could pass that address.
* Having a function pointer encourages implementations to actually live outside of `VMReadContext`. This is the opposite of what `VMReadContext()` tries to do -- Hide as much platform specific behaviour as possible inside `VMReadContext`. It is a Darwin specific implementation detail that we have to pass a function pointer to `VMReadContext`, other platforms should **not be forced** to do that in-order to write their implementations. They should be able to pass whatever makes sense for their platform.
* On other platforms the interface may need to work differently. More parameters might need to be passed to the reader. In this case the `void*` would be a pointer to a struct containing the extra data needed by the platform.
* It ties the design of `VMReadContext` very closely to Darwin's `memory_reader_t`.

Besides, I could probably achieve the non-platform specific error handling simplify by refactoring `Read(...)` into the platform agnostic part `Read(...)` and the platform specific part `ReadImpl(...)`.

I think we could clean up the interface in a better way. Instead of a `void*` argument to the constructor it could be

VMReadContextConfig* which would be

```
class VMReadContextConfig {
   public:
    __sanitizer::process_vm_read_handle_t target_process;
};
```

Then other platforms could sub-class this and add their own platform specific fields. For Darwin we'd add a function pointer field so that the Darwin specific function pointer can be passed.

That way

* At most one argument needs to be passed `VMReadContext`
* It allows for platform specific implementations in a way that's slightly more type safe than `void*`.
* We can pass arbitrary data to be passed to `VMReadContext`.

  We don't have RTTI or LLVM's dyn_cast so  we would have to statically cast inside each implementation to the expected `VMReadContextConfig` sub-class.



Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D56206