[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:25
+
+struct RemoteAddressSpaceView {
+ private:
----------------
struct -> class


================
Comment at: lib/sanitizer_common/sanitizer_remote_address_space_view.h:68
+// object.
+struct ScopedRemoteAddressSpaceViewContext {
+ public:
----------------
maybe nested: ScopedRemoteAddressSpaceViewContext -> RemoteAddressSpaceView::ScopedContest
and make RemoteAddressSpaceView::set_context private


================
Comment at: lib/sanitizer_common/sanitizer_remote_address_space_view.h:68
+// object.
+struct ScopedRemoteAddressSpaceViewContext {
+ public:
----------------
vitalybuka wrote:
> maybe nested: ScopedRemoteAddressSpaceViewContext -> RemoteAddressSpaceView::ScopedContest
> and make RemoteAddressSpaceView::set_context private
class and 
private:
  VMReadContext vmrc_;


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:24
+ public:
+  typedef __sanitizer::process_vm_read_handle_t ProcessHandle;
+
----------------
Just define "typedef int ProcessHandle;" and remove process_vm_read_handle_t
On other platforms it does not matter, and we will add more specific type if needed later.


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:31
+  void* config;
+  ProcessHandle target_process;
+  bool is_local;
----------------
historically sanitizer common uses Google C++ naming convention so members need to have _ suffux.
 


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:50
+  // Returns `nullptr` if no successful read has been performed.
+  void* GetLocalAddress() const { return (void*)local_address; }
+
----------------
Please remove all stuff you are not using in upcoming patches.
When can extend API when needed



================
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; }
+
----------------
If you can calculate it from target_process please remove member and simplify state


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:93
+  // Returns `true` on a sucessful read and `false` otherwise.
+  bool Read(uptr target_address, u64 size, bool writable = true);
+};
----------------
As I understand we do not expect a lot of context callers
so let remove default "writable = true" and make calls more verbose


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context_common.cc:20
+
+void VMReadContext::Reset() {
+  local_address = 0;
----------------
Remove it if you don't use it.
I'd expected ScopedContext is the why to reset it.


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context_mac.cc:42
+bool VMReadContext::Read(uptr target_address, u64 size, bool writable) {
+  // TODO(dliew): Investigate if `writable` is useful for optimization.  For
+  // now we just ignore this argument and assume that we are compliant.
----------------
Just remove it. You'll add it when needed


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D56206