[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:
----------------
delcypher wrote:
> vitalybuka wrote:
> > struct -> class
> `LocalAddressSpaceView` is a `struct`. Making `RemoteAddressSpaceView` be a `class` seems inconsistent. Why do you want to make this change?
Historically sanitizers follow google c++ style and class is more appropriate here
https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes

as well for LocalAddressSpaceView  


================
Comment at: lib/sanitizer_common/sanitizer_remote_address_space_view.h:27
+ private:
+  static VMReadContext* vmrc;
+  static void* internal_read(uptr target_address, uptr size, bool writable) {
----------------
delcypher wrote:
> vitalybuka wrote:
> > delcypher wrote:
> > > vitalybuka wrote:
> > > > Why all these stuff needs to be static?
> > > @vitalybuka Because the allocators don't receive an instance of `RemoteAddressSpaceView`, they receive the type as a template parameter (e.g. SizeClassAllocator64). This design choice means that everything in an implementation of the `AddressSpaceView` interface (i.e. `LocalAddressSpaceView` and `RemoteAddressSpaceView`) needs to be static.
> > > 
> > > To implement `RemoteAddressSpaceView` we **have to store some state**. All of this state and the read operations are encapsulated in the `VMReadContext` object. We cannot store any of this state inside the allocators because that could change the data-layout.
> > > 
> > > This design is not thread-safe of course but for allocator enumeration on Darwin will only ever be doing that in a single thread anyway.
> > Can you add some CHECKs validate this single thread assumption?
> > Can you add some CHECKs validate this single thread assumption?
> @vitalybuka 
> 
> I don't see an easy way to do this because this code will be used in a context where there is no ThreadRegistry and there is no interception on `pthread_create` and other thread functions. I also couldn't see a non-asan specific (e.g. I found `AsanThread* GetCurrentThread()`) API to use because this code will be common to all sanitizers.
> 
> There are also examples of existing code that don't check the single thread assumption. E.g. from `lib/asan/asan_thread.cc`
> 
> ```
> ThreadRegistry &asanThreadRegistry() {
>   static bool initialized;
>   // Don't worry about thread_safety - this should be called when there is
>   // a single thread.
> ...
> ```
> 
> I'm not against adding some sort of check to validate the single thread assumption but I need more guidance on how to do this using the available sanitizer internal APIs.
what about saving GetTid() on the first call of set_context()
then check that saved id matches GetTid() for all subsequent calls to 
set_context()
get_context()
Load*()

I assume this is not going to be main thread?


================
Comment at: lib/sanitizer_common/sanitizer_remote_address_space_view.h:67
+  // object.
+  struct ScopedContext {
+   public:
----------------
same for class


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:17
+#define SANITIZER_VM_READ_CONTEXT_H
+#include "sanitizer_internal_defs.h"
+#include "sanitizer_platform.h"
----------------
empty live between define and include


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:30
+  bool is_local_ = false;
+  ProcessHandle target_process_;
+
----------------
please init target_process_ to something target_process_ as well


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:42
+
+  VMReadContext(const VMReadContext&) = delete;
+
----------------
why copying is not acceptable?


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:24
+ public:
+  typedef __sanitizer::process_vm_read_handle_t ProcessHandle;
+
----------------
delcypher wrote:
> vitalybuka wrote:
> > 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.
> Apart from that fact that it needs to be `unsigned` on Darwin. I'm not super comfortable doing that because the declaration of `__sanitizer::process_vm_read_handle_t` in `sanitizer_internal_defs.h` allows to detect if the system declaration of `task_t` disagrees with the Sanitizer runtimes type. This is because on Darwin we include system headers that do define the `task_t` type.
then just define process_vm_read_handle_t as task_t in sanitizer_internal_defs for Darwin?


================
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; }
+
----------------
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_;
}

```


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



================
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) {
----------------
can you remove "config" argument and just initialize config_ to darwin specific stuff here?


================
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;
----------------
if we keep it please remove {} for consistency 


================
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)));
----------------
why do we need config_?
from here looks like you expect a particular function
could you call it directly here?


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D56206