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

[Anubhab Ghosh via Phabricator via llvm-commits]

argentite created this revision.
Herald added subscribers: hiraditya, mgorny.
Herald added a project: All.
lhames added a comment.
argentite updated this revision to Diff 436145.
argentite edited the summary of this revision.
argentite updated this revision to Diff 436233.
argentite updated this revision to Diff 436748.
argentite marked 13 inline comments as done.
argentite published this revision for review.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

What we're looking for is a breakdown in responsibilities where:
(1) The MappingJITLinkMemoryManager is responsible for object layout (of blocks into segments), and heap management (subject to constraints that the Mapper will describe).
(2) The Mapper is responsible for address space, physical memory management (and content transfer), and recording and applying actions.


argentite added a comment.

Introduced prepare() and Allocation to contain multiple segments and actions


lhames added a comment.

Today was busier than anticipated -- I've read the new patch, but haven't had a chance to try it out yet.

I've added some notes on the `prepare` method, and will try it out tomorrow morning.


argentite added a comment.

Removed unnecessary protection argument


lhames added a comment.

Finally got to check this out -- I've added some more feedback comments.

This is looking great. I think it's ready for public review, with the aim that we land it in-tree this week.

What do you think? Are you happy with it?


argentite added a comment.

Simplified prepare return type
Used runFinalizeActions and runDeallocActions instead of reimplementing
Changed some mutex lock scopes



================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:48
+  /// Provides working memory
+  virtual Expected<char *> prepare(ExecutorAddr Addr, size_t ContentSize) = 0;
+
----------------
I think you can probably simplify this further and just return a `char *` -- I don't know of any situation where our working memory allocation could fail.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:70-96
+  Expected<ExecutorAddrRange> reserve(size_t NumBytes) {
+    std::promise<Expected<ExecutorAddrRange>> P;
+    auto F = P.get_future();
+    reserve(NumBytes, [&](auto R) { P.set_value(std::move(R)); });
+    return F.get();
+  }
+
----------------
Synchronous versions of the APIs? Very interesting. I've been assuming that `Mapper` would _only_ have `MappingJITLinkMemoryManager` as a client, and that we'd eventually end up moving the interface into that class (or making it a concept and having `MappingJITLinkMemoryManager` take a template argument), but maybe it's worth having as a utility in its own right.

Did you have a use-case in mind?


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:70-96
+  Expected<ExecutorAddrRange> reserve(size_t NumBytes) {
+    std::promise<Expected<ExecutorAddrRange>> P;
+    auto F = P.get_future();
+    reserve(NumBytes, [&](auto R) { P.set_value(std::move(R)); });
+    return F.get();
+  }
+
----------------
lhames wrote:
> Synchronous versions of the APIs? Very interesting. I've been assuming that `Mapper` would _only_ have `MappingJITLinkMemoryManager` as a client, and that we'd eventually end up moving the interface into that class (or making it a concept and having `MappingJITLinkMemoryManager` take a template argument), but maybe it's worth having as a utility in its own right.
> 
> Did you have a use-case in mind?
Not really. it just felt nicer when writing the tests. We can remove them.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:28-32
+  struct Mapping {
+    ExecutorAddr RemoteAddr;
+    void *LocalAddr;
+    size_t Size;
+  };
----------------
`Mapping` shouldn't be associated with a local address: It just represents the reserved range in the executor.

I don't think we need a separate type for this -- An `ExecutorAddrRange` should be ideal.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:35
+  /// Represents an unit of memory to finalize its contents and protections
+  /// The offest is realtive to a reservation represented by a Mapping
+  /// It ensures the offset is same on both executor and controller side for
----------------
typos: offset, relative.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:38-44
+  struct SegInfo {
+    ExecutorAddrDiff Offset;
+    size_t ContentSize;
+    size_t ZeroFillSize;
+    unsigned Prot;
+    shared::AllocActions Actions;
+  };
----------------
The current struct has actions associated with the segment. One allocation may contain multiple segments, but only one list of actions -- the actions are associated with the allocation, rather than any particular segment.

Initialize will also need the address of the working memory for each segment.

I think you probably want something more like:

```
struct AllocInfo {
  struct SegInfo {
    ExecutorAddrDiff Offset;
    const char *WorkingMem;
    size_t ContentSize;
    size_t ZeroFillSize;
    unsigned Prot;
  };
  ExecutorAddr MappingBase;
  std::vector<SegInfo> Segments;
  shared::AllocActions Actions;
};




================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:51
+  virtual void reserve(size_t NumBytes, OnReservedFunction OnReserved) = 0;
+
+  using OnInitializedFunction = unique_function<void(Error)>;
----------------
We'll want a `prepare` method in here. This is just responsible for setting up working memory, so it can be synchronous:

`prepare :: [ (executor-addr, content-size, prot) ] -> [ char * ]`


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:57-58
+  /// applies memory protections
+  virtual void initialize(Mapping &M, std::vector<SegInfo> &Segments,
+                          OnInitializedFunction OnInitialized) = 0;
+
----------------
Initialize just needs to return a unique address associated with the recorded dealloc actions in the executor, so under the changes proposed above this would become:

```
  virtual ExecutorAddr initialize(AllocInfo &AI, OnInitializedFunction OnInitialized) = 0;
```

We'll use this `ExecutorAddr` as the value for the `FinalizedAlloc` that we return from the memory manager.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h:63-64
+  /// Runs previously specified denitialization actions
+  virtual void deinitialize(Mapping &M, std::vector<ExecutorAddr> &Segments,
+                            OnDeinitializedFunction OnDeInitialized) = 0;
+
----------------
`deinitialize` should just take the `ExecutorAddr` that was returned from `initialize`, so:

```
virtual void `deinitialize(ExecutorAddr A, OnDeInitialized) = 0;
```


================
Comment at: llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp:65-73
+    Allocations[MinAddr].DeinitializationActions.reserve(AI.Actions.size());
+
+    for (auto &ActionPair : AI.Actions) {
+      if (auto Err = ActionPair.Finalize.runWithSPSRetErrorMerged())
+        return OnInitialized(std::move(Err));
+
+      Allocations[MinAddr].DeinitializationActions.push_back(
----------------
I think you can use `runFinalizeActions` from `AllocationActions.h` here.


================
Comment at: llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp:88-98
+    auto &Actions = Allocations[Base].DeinitializationActions;
+    while (!Actions.empty()) {
+      auto &Action = Actions.back();
+
+      if (Error Err = Action.runWithSPSRetErrorMerged()) {
+        AllErr = joinErrors(std::move(AllErr), std::move(Err));
+        Failed = true;
----------------
I think you can use `runDeallocActions` from `AllocationActions.h` here.


================
Comment at: llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp:132-136
+    while (!A.second.DeinitializationActions.empty()) {
+      auto &Action = A.second.DeinitializationActions.back();
+      cantFail(Action.runWithSPSRetErrorMerged());
+      A.second.DeinitializationActions.pop_back();
+    }
----------------
This looks like `runDeallocActions` again.


================
Comment at: llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp:38-41
+  if (auto EC = sys::Memory::protectMappedMemory(
+          {Addr.toPtr<void *>(), ContentSize}, Protections)) {
+    return errorCodeToError(EC);
+  }
----------------
The protection change should still be part of `initialize`, and `prepare` can drop the `Protections` argument.

The `prepare` method's job is just to associate _working memory_ with the given _executor address_ in such a way that we can follow-up with the content transfer during `initialize`. For `InProcessMapper` the executor address doubles as the address of the working memory, so `prepare` really does reduce to nothing but a `char*` cast.

For something like EPCMemoryMapper it'll be more interesting: `prepare` will malloc some memory (or more likely allocate it using the `LinkGraph`s allocator), and record the association between that allocated working memory and the target address. Then in `initialize` we'll issue an EPC call to copy the content from working memory over to the executor.


================
Comment at: llvm/unittests/ExecutionEngine/Orc/MemoryMapperTest.cpp:31
+
+TEST(ExecutorSharedMemoryManagerTest, AllocFinalizeFree) {
+  int InitializeCounter = 0;
----------------
Could you add some comments to this function describing the steps? E.g.
```
// Define counters -- these will be incremented/decremented in
// finalize/dealloc actions to verify that the actions are run.
```


================
Comment at: llvm/unittests/ExecutionEngine/Orc/MemoryMapperTest.cpp:37-38
+        std::make_unique<InProcessMemoryMapper>();
+    auto PageSize = sys::Process::getPageSize();
+    cantFail(PageSize.takeError());
+    auto AllocSize = *PageSize * 2;
----------------
You can write this as:
```
auto PageSize = cantFail(sys::Process::getPageSize());
```
That will auto-unwrap the `Expected` for you.


MemoryMapper class takes care of cross-process and in-process address space
reservation, mapping, transferring content and applying protections.

Implementations of this class can support different ways to do this such
as using shared memory, transferring memory contents over EPC or just
mapping memory in the same process (InProcessMemoryMapper).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D127491

Files:
  llvm/include/llvm/ExecutionEngine/Orc/MemoryMapper.h
  llvm/lib/ExecutionEngine/Orc/CMakeLists.txt
  llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp
  llvm/unittests/ExecutionEngine/Orc/CMakeLists.txt
  llvm/unittests/ExecutionEngine/Orc/MemoryMapperTest.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D127491.436748.patch
Type: text/x-patch
Size: 14874 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220615/9bd1a640/attachment.bin>