[PATCH] D132313: [Orc] Memory Mapper fixes

[Lang Hames via Phabricator via llvm-commits]

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

> By the time SharedMemoryMapper destructor is called, the RPC
> connection is no longer available causing the release() call to
> always fail. Instead at this point only shared memory regions
> can be unmapped safely.

I think that you can assert that there are no remaining allocations in the destructor -- these should all have been destroyed by a call to `endSession()`, which should happen before the allocator is destroyed.

Otherwise LGTM. Thanks Argentite!



================
Comment at: llvm/lib/ExecutionEngine/Orc/MapperJITLinkMemoryManager.cpp:35-37
+    Parent.Mapper->initialize(AI, [OnFinalize = std::move(OnFinalize)](
+                                      Expected<ExecutorAddr> Result) mutable {
       if (!Result) {
----------------
Nice catch!


================
Comment at: llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp:305-308
+  auto Reservation = std::lower_bound(
+      Reservations.rbegin(), Reservations.rend(), AI.MappingBase,
+      [](const auto &A, const auto &B) { return A.first > B; });
+  assert(Reservation != Reservations.rend() && "Attempt to initialize unreserved range");
----------------
So `MappingBase` is the lowest allocated address for this allocation?


================
Comment at: llvm/lib/ExecutionEngine/Orc/TargetProcess/ExecutorSharedMemoryMapperService.cpp:284-286
+  for (const auto &R : Reservations) {
+    ReservationAddrs.push_back(ExecutorAddr::fromPtr(R.getFirst()));
   }
----------------
No braces needed for single lines. You could sink the definition of `ReservationAddrs` down below the if test too:

```
  if (Reservations.empty())
    return Error::success();

  std::vector<ExecutorAddr> ReservationAddrs;
  ReservationAddrs.reserve(Reservations.size());
  for (const auto &R : Reservations)
    ReservationAddrs.push_back(ExecutorAddr::fromPtr(R.getFirst()));
```



================
Comment at: llvm/lib/ExecutionEngine/Orc/TargetProcess/ExecutorSharedMemoryMapperService.cpp:288
 
   return release(ReservationAddrs);
 }
----------------
Missing `std::move`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132313