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

[Dan Liew via Phabricator via llvm-commits]

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


================
Comment at: lib/sanitizer_common/sanitizer_remote_address_space_view.h:25
+
+struct RemoteAddressSpaceView {
+ private:
----------------
vitalybuka wrote:
> struct -> class
`LocalAddressSpaceView` is a `struct`. Making `RemoteAddressSpaceView` be a `class` seems inconsistent. Why do you want to make this change?


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


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

That's a great idea. I'll do that.


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:24
+ public:
+  typedef __sanitizer::process_vm_read_handle_t ProcessHandle;
+
----------------
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.


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context.h:27
+ private:
+  uptr local_address;
+  uptr target_address;
----------------
vitalybuka wrote:
> could you please init these inline?
Yes. I'll do that.


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


================
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; }
+
----------------
vitalybuka wrote:
> Please remove all stuff you are not using in upcoming patches.
> When can extend API when needed
> 
I'll have a look through my out-of-tree implementation and see what I can remove.


================
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);
+};
----------------
vitalybuka wrote:
> As I understand we do not expect a lot of context callers
> so let remove default "writable = true" and make calls more verbose
Sure.


================
Comment at: lib/sanitizer_common/sanitizer_vm_read_context_common.cc:20
+
+void VMReadContext::Reset() {
+  local_address = 0;
----------------
vitalybuka wrote:
> Remove it if you don't use it.
> I'd expected ScopedContext is the why to reset it.
`Reset()` gets called if the read call fails. Although I've just realised that's actually inconsistent with the API comments. I'll fix that.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D56206