[Orc] Memory Mapper fixes

[argentite]

Summary

[Orc] Actually save the callback

The callback function was captured by reference but it lived on the
stack and was in danger of being overwritten and could cause a crash.

[Orc] Only unmap shared memory in controller process destructor

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.

Deinitializers are called and mapped memory is released at the
executor side by ExecutorSharedMemoryMapperService::shutdown()
instead. Memory can also be released earlier by calling release()
earlier before RPC connection is closed.

[Orc] Provide correct Reservation address for slab allocations

When slab allocator is used, the MappingBase is not necessarily
the same as the original reservation base as the allocation could
be a part of the whole reservation.

In this case the original reservation address needs to be passed to
ExecutorSharedMemoryMapperService to associate the new allocation
with the original reservation.

[Orc] Improve deintialize and shutdown logic

When deinitializing, the allocation needs to be removed from the
allocation list of its associated reservation so that remaining
allocations can be deinitialized when releasing the reservation.

[lhames]

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!

llvm/lib/ExecutionEngine/Orc/MapperJITLinkMemoryManager.cpp

35–37 ↗

(On Diff #454253)

Nice catch!

llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp

305–308 ↗

(On Diff #454253)

So MappingBase is the lowest allocated address for this allocation?

llvm/lib/ExecutionEngine/Orc/TargetProcess/ExecutorSharedMemoryMapperService.cpp

281–283

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()));

286

Missing std::move.

[argentite]

Addressed the above issues

[argentite]

In D132313#3737642, @lhames wrote:

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.

We don't keep track of allocations in the controller process other than through the FinalizedAlloc class. That needs to be release()'d which is done only after deinitializing at MapperJITLinkMemoryManager::deallocate(). So I guess we are safe there.

llvm/lib/ExecutionEngine/Orc/MemoryMapper.cpp

305–308 ↗

(On Diff #454253)

Yes it is actually the address that the slab allocator returns which may start at the middle of a reservation. We use this address as the identifier for this allocation.

[argentite]

Properly rebase everything

[argentite]

Properly base everything hopefully

llvm/lib/ExecutionEngine/Orc/TargetProcess/ExecutorSharedMemoryMapperService.cpp

Error AllErr = Error::success();
     std::lock_guard<std::mutex> Lock(Mutex);
 
     for (auto Base : Bases) {
       if (Error Err = shared::runDeallocActions(
               Allocations[Base].DeinitializationActions)) {
         AllErr = joinErrors(std::move(AllErr), std::move(Err));
       }
 
+      // Remove the allocation from the allocation list of its reservation
+      for (auto &Reservation : Reservations) {
+        auto AllocationIt =
+            std::find(Reservation.second.Allocations.begin(),
+                      Reservation.second.Allocations.end(), Base);
+        if (AllocationIt != Reservation.second.Allocations.end()) {
+          Reservation.second.Allocations.erase(AllocationIt);
+          break;
+        }
+      }
+
       Allocations.erase(Base);
     }
   }
 
   return AllErr;
 }
 
 Error ExecutorSharedMemoryMapperService::release(

 #else
   return make_error<StringError>(
       "SharedMemoryMapper is not supported on this platform yet",
       inconvertibleErrorCode());
 #endif
 }
 
 Error ExecutorSharedMemoryMapperService::shutdown() {
-  std::vector<ExecutorAddr> ReservationAddrs;
-  {
-    std::lock_guard<std::mutex> Lock(Mutex);
-
-    if (Reservations.empty())
-      return Error::success();
 
-    ReservationAddrs.reserve(Reservations.size());
-    for (const auto &R : Reservations) {
-      ReservationAddrs.push_back(ExecutorAddr::fromPtr(R.getFirst()));
-    }
-
-    Reservations.clear();
-  }
+  std::vector<ExecutorAddr> ReservationAddrs;
+  for (const auto &R : Reservations)
 
-  return release(ReservationAddrs);
+  return release(std::move(ReservationAddrs));
 }
 
 void ExecutorSharedMemoryMapperService::addBootstrapSymbols(
     StringMap<ExecutorAddr> &M) {
   M[rt::ExecutorSharedMemoryMapperServiceInstanceName] =
       ExecutorAddr::fromPtr(this);
   M[rt::ExecutorSharedMemoryMapperServiceReserveWrapperName] =
       ExecutorAddr::fromPtr(&reserveWrapper);