[PATCH] D50330: [RFC] Implement out-of-process allocator enumeration for macOS

[Dan Liew via Phabricator via llvm-commits]

delcypher added a comment.

In https://reviews.llvm.org/D50330#1281420, @kcc wrote:

> Sorry, this isn't any better than the previous version.


I don't agree with that but perhaps I misunderstood precisely what you disliked about the previous version of the patch. My understanding was that you didn't like the fact there were duplicate implementation for enumerating allocations, one for in-process (what exists today), and one for out-of-process (what I'm trying to add).
This patch removes the duplicate implementations by unifying the implementations and templating them on a `ObjectView` type which abstracts the out-of-process operations (allocating memory, copying memory from another process into that allocated memory, deallocating the memory).

> Please understand my position: this is the core code for all the existing sanitizers, scudo, and the new hwasan, 
>  we are planing to have this code in production (in fact, we already have it in production in many places). 
>  There is no way we can maintain code with so much extra stuff in it. The change has to be less intrusive.

Okay, I understand your position but I can't easily fix this patch unless you give me more explicit guidance on **which parts you do not like**. If you point to a particular thing (e.g. The design pattern that is being consistently followed) you don't like I can explain the reasoning behind it and also discuss alternatives.
Enumerating the allocators in an out-of-process mode is complicated and I think it is inevitable that the change will be intrusive.

There are certain places where I have made changes for safety reasons, but they could be dropped. For example the `SpaceBeg()` instance method has been modified because it is in the call graph of instance methods that are called from `ForEachChunk(...)`. The rule I have been following is that instance methods should **not** be called directly
because doing so may be unsafe because they might deference memory that does not exist in the out-of-process scenario. A method like `SpaceBeg()` is actually safe to call directly on an allocator copied for another process but there is nothing enforcing that. Changing `SpaceBeg()` (in this case it's actually renamed to `SpaceBegOOP()` and a new method named `SpaceBeg()` just calls it with the appropriate arguments) to be templated on `ObjectView` and be a static method means that:

- There is no `this` pointer with prevents anyone calling instance methods directly.
- The fact that the method is templated on `ObjectView` documents the fact that this method is in the call graph for out-of-process enumeration. This forces the programmer to actually think about what they are doing because the method is used in the in-process and out-of-process code paths.

But as I mentioned earlier `SpaceBeg()` is currently safe to call directly on an Allocator so I could drop the changes (and to similar methods that are safe to call directly) if you don't like them. I'm not a fan of this because I prefer safety over simplicity.

>>> Just to note at our discussions at LLVMDev you wanted the template type (in this case what I call ObjectView) to an allocator parameter. I have not done this because I discovered this is impossible (when ObjectView is templated on the type of the object it is representing) because you end up with mutually recursive types. I.e.
>>>  ObjectView<Allocator<Params< ObjectView<Allocator<Params< ... > > > > > >
> 
> I don't understand this. Let's take some specific class, e.g. SizeClassAllocator64, 
>  and see why ObjectView can't be made part of "Params".

Sorry I was incorrect when I said this was "impossible". When I looked this initially I couldn't see a way to do because `ObjectView` is templated on the type it represents. If you have `Params` contain a fully instantiated typedef (e.g. `ObjectView<SizeClassAllocator<...>>`) you end up with
recursive types. To illustrate this consider this

  #include <stdlib.h>
  
  template <typename ObjectTy> class LocalObjectView {
  private:
    uintptr_t local_address;
  
  public:
    LocalObjectView(ObjectTy *addr)
        : local_address(reinterpret_cast<uintptr_t>(addr)) {}
    ObjectTy *GetLocalAddress() const {
      return reinterpret_cast<ObjectTy *>(local_address);
    }
    size_t GetSize() const { return sizeof(ObjectTy); }
  };
  
  template <template ObjectViewT>
  class AP64 {
  public:
    static const int kSomeConstant = 0;
    typedef ObjectViewT ObjectView;
  };
  
  template <typename Params> class SizeClassAllocator {
  public:
    typedef SizeClassAllocator<Params> ThisT;
    static const auto kValue = Params::kSomeConstant;
    int foo = 0;
  
    static void DoSomething(Params::ObjectView* view) {
      view->GetLocalAddress()->foo = 1;
    }
  };
  
  int main() {
    // Can't declare the type because of recursive type
    // ??? = SizeClassAllocator<AP64<LocalObjectView<???>>>
    SizeClassAllocator<AP64<LocalObjectView<???>>> allocator;
  }

Notice how it's impossible to actually instantiate the `SizeClassAllocator` in this example. Because of this I went with the approach of templating the methods I needed to change.

I've given this some thought and I've realised that there is a way to make this work by making `Params::ObjectView` a dependent type as shown below.

  #include <stdlib.h>
  
  template <typename ObjectTy> class LocalObjectView {
  private:
    uintptr_t local_address;
  
  public:
    LocalObjectView(ObjectTy *addr)
        : local_address(reinterpret_cast<uintptr_t>(addr)) {}
    ObjectTy *GetLocalAddress() const {
      return reinterpret_cast<ObjectTy *>(local_address);
    }
    size_t GetSize() const { return sizeof(ObjectTy); }
  };
  
  template <template <typename> class ObjectViewT> class AP64 {
  public:
    static const int kSomeConstant = 0;
    template <typename ObjectTy> using ObjectView = ObjectViewT<ObjectTy>;
  };
  
  template <typename Params> class SizeClassAllocator {
  public:
    typedef SizeClassAllocator<Params> ThisT;
    static const auto kValue = Params::kSomeConstant;
    int foo = 0;
  
    static void DoSomething(typename Params::template ObjectView<ThisT> *view) {
      view->GetLocalAddress()->foo = 1;
    }
  };
  
  int main() {
    SizeClassAllocator<AP64<LocalObjectView>> allocator;
    LocalObjectView<decltype(allocator)> view(&allocator);
    decltype(allocator)::DoSomething(&view);
  }

So actually I could make `ObjectView` be part of the allocator parameters instead of what I've done in this patch. 
A nice aspect of this is it means all the static_asserts can be removed (I think). A downside however is that it makes declaring allocator types a bit more cumbersome because
this type will have to appear in three places in the allocator (in the combined, primary, and secondary). Does this seem like the right trade-off?

> Is it possible to make this change in several iterations where you start from introducing one more template parameter (ObjectView?) that does very little, 
>  then we can iterate extending it.

I can certainly break this patch up into smaller pieces but I haven't done so because some parts don't make any sense in isolation. If you'd like me to submit in smaller pieces I can, but basically it would be this patch but with `ObjectView` as an allocator parameter. Is that something you'd accepted or are there more fundamental changes that you want to make?
I'm very wary of putting in the work to breaking up this patch if it's definitely going to be rejected again.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D50330