[PATCH] D127491: [JITLink][Orc] Add MemoryMapper interface with InProcess implementation

[Lang Hames via Phabricator via llvm-commits]

lhames accepted this revision.
lhames added a comment.
This revision is now accepted and ready to land.

A couple of small comments, but otherwise LGTM. I think this is ready to land in tree.

Stefan — did you have any other feedback?



================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:55
+  /// applies memory protections
+  /// Returns minimum address of all the segments as an unique identifier for
+  /// the allocation
----------------
I think we want to keep the contract on the address returned by `initialize` loose:
```
/// Returns a unique address identifying the allocation. This address should
/// be passed to deinitialize to run deallocation actions (and reset permissions
/// where possible).
```
The `InProcessAllocator` uses the lowest allocated address as a key into a DenseMap, but you could imagine schemes that return a pointer to a bookkeeping struct instead.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:62
+  virtual void deinitialize(std::vector<ExecutorAddr> &Allocations,
+                            OnDeinitializedFunction OnDeInitialized) = 0;
+
----------------
argentite wrote:
> sgraenitz wrote:
> > In which case do we have multiple `ExecutorAddr` here? `initialize()` only returns a single one -- can it run multiple times for a single MemoryMapper? If this is the case, it may deserve a test case and/or a comment.
> > 
> > If we keep the `std::vector` I guess it should be passed as `const std::vector<ExecutorAddr> &` -- or is there a design reason for mutability here?
> As I understand the a single MemoryMapper should live for the whole duration of JITLink use. When terminating, we can pass the results of multiple initialize to a single deinitialize to save RPC overhead. I have made the vectors const for both deinitialize and release.
I think that an ArrayRef would do — that way any array-like list of addresses would work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127491