[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_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:
> > > 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?
Okay I've added the thread check. I don't know if this code will be called on the main thread or not. Presumably that's irrelevant here?


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D56206