[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#1206984, @kcc wrote:

> I've looked at the code once more and I am really afraid of this extra complexity in the core of asan that is never needed outside of OSX/iOS. 
>  It would be much better to hide this into Mac-specific code that would use allocator's public APIs.
>  It's fine to add some public APIs to the allocator for that purpose, as long as they are testable. 
>  Is that possible?


Could you elaborate on what you are envisioning? The only methods I've added to the allocators in this patch are the `ForEachChunkOutOfProcess(...)`
methods (later patches do add a few out-of-process variants of existing methods). At present the `ForEachChunkOutOfProcess(...)` methods **have to
be in the allocator classes** because they access internal implementation details of the allocators which are `private`. Adding public methods to let external code (that arguably belongs in the allocator) get at these internals
seems like a horrible layering violation. I'm not convinced doing that would have any positive impact because now:

- There will be a public API that has to be maintained for just a single client in addition to having to maintain the allocator internals. Previously we only needed to worry about maintaining the allocator internals. Creating more maintenance burden than what is in this patch seems like the wrong way to go.
- It's even easier to break out-of-process enumeration because the code for it would live far away the relevant allocators.

What about the other idea I suggested where the `ForEachChunk` methods are unified so there is only one implementation for both in-process and out-of-process?
For in-process we'd use a zero-cost abstraction so that for in-process enumeration we don't do any of the expensive operations required for out-of-process enumeration.
That would seem to me to fix your valid concerns over having to maintain two different ForEachChunk implementations. I've not tried implementing this yet because I don't
want to spend time implementing something that won't be accepted upstream. However if this an approach you might consider acceptable then I can try to implement it.

> Also, just for my education, why would someone need to use `heap`, `vmmap`, or `leaks` on an asan-ified process? 
>  For leaks, we have lsan (already works on Mac IIRC). 
>  For `heap`, asan introduces a significant distortion so that the heap profiles won't be similar to the ones in production builds. 
>  For `vmmap`, part of that can be achieved by asan's builtin heap profiler (linux-only today).
> 
> You certainly know more about Mac than I do, I just want to make sure you are solving a problem that needs to be solved.

@kubamracek Has already listed the main reasons why we want to do this. I'd like to add this will also help LSan support on macOS. Right now LSan "kind of" works on macOS. It has false positives due it not understanding various internal implementation details of  various macOS specific libraries like GCD and the Swift runtime. The `leaks` tool on the other hand does not have these false positives because it is aware of these internal implementation details. Improvements to LSan on macOS will be greatly helped by comparing LSan reports against reports produced by the `leaks` tool. Being able to run the `leaks` tool on ASanified binary would make this process easier because programs don't need to be recompiled with ASan disabled to do leak checking with the `leaks` tool.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D50330