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

[Dan Liew via Phabricator via llvm-commits]

delcypher added a comment.

@kcc

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

> [sorry for delay, I've been OOO]
>  Ouch. That's really a lot of code in the core parts of *san, which adds quite a bit of maintenance tax. 
>  Two questions before we start reviewing the code:
>
> - is it possible to move more of the logic into Mac-specific files?


Yes, but that is some what in conflict with the idea the goal to have this code tested on all platforms. We would like to do this ideally because
we want tests to fail when someone changes the allocators in a way that would break out-of-process enumeration.

> - can other platforms benefit from this code in some way? How?

There would be several bits to this.

- Other platforms would need to implement `ProcessVMReaderContext` so that it is able to read the memory of another ASanified process. In the code I note some ways this could be done on Linux.
- To be able to do the actually enumeration the external process would need to know where the allocator lives in the target process. We would need to add mechanism for this for other platforms. On Darwin a mechanism already exists for this.

A more important question though is, what use cases on other Platforms are there? On macOS our use case is a lot of our pre-existing memory analysis
tools assume they can pause a running application and then enumerate the paused application's allocation. I'm not sure what use cases exist on other
platforms of enumerating allocations out of process.

Something worth noting is that there are multiple steps involved in implementing out-of-process allocation enumeration and this RFC is only the first step.
It didn't make sense to me to try to upstream the whole thing because that would make the patch too large and hard to review.

On to your more general concern about this approach adding too much code to core parts of ASan leading to more maintenance.

I've been thinking about this and there is a way to unify the `ForEachChunk(...)` and `ForEachChunkOutOfProcess(...)` methods so there is only a single implementation but it would
require a complete redesign of `ProcessVMReaderContext`. A way to unify the separate implementations is to template them on a type `T`, where `T`
implements the interface of `ProcessVMReaderContext`. When doing in-process enumeration the type T would basically be doing no-ops. When doing out-of-process
enumeration `T` would actually be the  `ProcessVMReaderContext` type. There are some downsides to this approach.

- With the in-process and out-of-process variants merged into one, anyone wishing to change this methods need to be very careful. They have to exclusively use the interface of T to dereference pointers. The code is also now harder to read because its written from the perspective of doing out-of-process enumeration, with the in-process case being handled in a very non-transparent way.
- The design of `ProcessVMReaderContext` would need to be changed significantly. Currently `ProcessVMReaderContext` does not own or manage the storage allocated for data copied from a target process. Right now the storage is handled externally by having stack allocations and `internal_memcpy(...)`-ing data into those locations. This design isn't compatible if we have unified `ForEachChunk` implementations because we want the in-process enumeration to have zero added cost (we don't want additional stack allocations and `internal_memcpy(...)` calls). To do this we would need `ProcessVMReaderContext` to actually own the storage allocated for out-of-process enumeration. Arguably this a better design anyway.

A problem with the suggestion above though is I'm not 100% sure if we can unify everything. For example later patches add an out-of-process version of `AsanChunk` and
I'm not sure if we can have a single implementation of that class that supports both in-process and out-of-process enumeration.

So the question is. What is more important, keeping the in-process and out-of-process functions separate for readability or unifying for maintainability?



================
Comment at: lib/sanitizer_common/sanitizer_process_vm_reader.h:27
+struct ProcessVMReaderContext {
+#if SANITIZER_MAC
+  typedef task_t ProcessHandle;
----------------
kcc wrote:
> is it possible to avoid #ifdefs in common code? 
Sure. I guess `lib/sanitizer_common/sanitizer_internal_defs.h` would be a sensible place to put the typedef?


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D50330