[PATCH] D128544: [Orc][JITLink] Add a shared memory based implementation of MemoryMapper

[Lang Hames via Phabricator via llvm-commits]

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

This looks great. Thanks @argentite!



================
Comment at: llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp:362
+  release(ReservationAddrs, [&](Error Err) { P.set_value(std::move(Err)); });
+  cantFail(F.get());
+}
----------------
The call to `release` could fail -- we'll want to figure out the right error plumbing for this in the long run. Could you add a `// FIXME` for now?


================
Comment at: llvm/lib/ExecutionEngine/Orc/TargetProcess/ExecutorSharedMemoryMapperService.cpp:156
+#else
+    llvm_unreachable("SharedMemoryMapper is not supported on this platform");
+#endif
----------------
Are there any other guards to prevent `ExecutorSharedMemoryMapperService` from being instantiated / used on unsupported platforms? If not, I think this should return an `llvm::Error` for now.


================
Comment at: llvm/unittests/ExecutionEngine/Orc/SharedMemoryMapperTest.cpp:58-63
+  // barrier
+  std::promise<void> P;
+  auto F = P.get_future();
+
+  auto PageSize = cantFail(sys::Process::getPageSize());
+  size_t ReqSize = PageSize;
----------------
Looks like these could be moved into the scope below.


================
Comment at: llvm/unittests/ExecutionEngine/Orc/SharedMemoryMapperTest.cpp:59-60
+  // barrier
+  std::promise<void> P;
+  auto F = P.get_future();
+
----------------
There's no explicit test that the lambdas are executed, but I think the use of this promise/future pair in the innermost lambda is sufficient -- the test will block unless all lambdas are run. That's probably worth a comment to point out though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128544