This is an archive of the discontinued LLVM Phabricator instance.

[Orc] Memory Mapper fixes
ClosedPublic

Authored by argentite on Aug 20 2022, 2:41 PM.

Details

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.

Diff Detail

Event Timeline

argentite created this revision.Aug 20 2022, 2:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2022, 2:41 PM
argentite retitled this revision from [Orc] Actually save the callback to [Orc] Memory Mapper fixes.Aug 20 2022, 2:42 PM
argentite edited the summary of this revision. (Show Details)
argentite published this revision for review.Aug 20 2022, 3:17 PM
argentite added reviewers: lhames, sgraenitz.
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2022, 3:17 PM
lhames accepted this revision.Aug 20 2022, 3:58 PM

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–310

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.

This revision is now accepted and ready to land.Aug 20 2022, 3:58 PM
argentite updated this revision to Diff 454273.Aug 20 2022, 8:27 PM
argentite marked 4 inline comments as done.

Addressed the above issues

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–310

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 updated this revision to Diff 454286.Aug 21 2022, 2:29 AM

Properly rebase everything

argentite updated this revision to Diff 454288.Aug 21 2022, 2:41 AM

Properly base everything hopefully