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

[Dan Liew via Phabricator via llvm-commits]

delcypher marked an inline comment as done.
delcypher added inline comments.


================
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.
----------------
vitalybuka wrote:
> delcypher wrote:
> > vitalybuka wrote:
> > > Just remove it. You'll add it when needed
> > By "it" I presume you mean that `writable` argument?
> > 
> > But if I do that, how far up the stack should I go? Taken to the extreme that means reverting what was landed in https://reviews.llvm.org/D54879 . I really don't want to do that because the distinction between writable and read-only access is likely important.
> It still looks like better to split Read and ReadWriteble when we have different implementation for them.
> So If you have/expect such patch quite soon, then fine, let's keep it. If this is just expectation that some-when in undefined future we will likely need to spit them, then I'd prefer we split when it's actually necessary.
> 
Using the read-only/writable information will land much later so I'm going to remove passing this information to `VMReadContext`.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D56206