[PATCH] D150184: [ORC] Fix race-condition in RTDyldObjectLinkingLayer::onObjEmit.

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: llvm/lib/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.cpp:366-391
+  // Add to MemMgrs before potentially notifying other threads, that the object
+  // was emitted. Otherwise these other threads could already execute the code
+  // and remove the object, leading to defunct resource trackers.
+  if (auto Err = R.withResourceKeyDo(
+          [&](ResourceKey K) { MemMgrs[K].push_back(std::move(MemMgr)); })) {
+    getExecutionSession().reportError(std::move(Err));
+    R.failMaterialization();
----------------
mkroll wrote:
> lhames wrote:
> > This call to `notifyObjectLoaded` will reference the moved-from `MemMgr` argument if there are any listeners attached.
> > 
> > The address of the MemMgr should be recorded in a local variable for use in the `notifyObjectLoaded` call. It would also be good if `MemMgr` were removed from the `MemMgrs` vector if the call to `notifyEmitted` fails.
> Ouch, how did I miss that `std::move()`...
> 
> But this would move the problem to the event listener. When the event listener is called, the other thread (unblocked by `R.notifyEmitted()`) could already have called `ExecutionEngine::removeJITDylib()` resulting in the memory manager to be removed in `RTDyldObjectLinkingLayer::handleRemoveResources()`.
> 
> Hm, `RtDyldObjectLinkingLayer::onObjEmit()` calls three types of notification functions:
> 
>   # `MaterializationResponsibility::notifyEmitted()`: "Notifies the target JITDylib (and any pending queries on that JITDylib) that all symbols covered by this MaterializationResponsibility instance have been emitted."
>   # `JITEventListener::notifyObjectLoaded()`: "Called after an object has had its sections allocated and addresses assigned to all symbols. Note: Section memory will not have been relocated yet. notifyFunctionLoaded will not be called for individual functions in the object." (`notifyFunctionLoaded` should probably be `notifyObjectLoaded`?)
>   # `RTDyldObjectLinkingLayer::NotifyEmitted()`: "Functor for receiving finalization notifications"
> 
> Do you think, it would be possible to change the order to
> 
>   # `JITEventListener::notifyObjectLoaded()`
>   # `RTDyldObjectLinkingLayer::NotifyEmitted()`
>   # Add `MemMgr` to `MemMgrs`
>   # `MaterializationResponsibility::notifyEmitted()`
> 
> ?
> 
> This would then require to also undo adding the memory manager and other things by calling `handleRemoveResources`, if `MaterializationResponsibility::notifyEmitted()` failed.
> 
> The OrcJITTests seem to be OK with this...
> But this would move the problem to the event listener.

Good point!

> Do you think, it would be possible to change the order to
>
> 1. `JITEventListener::notifyObjectLoaded()`
> 2. `RTDyldObjectLinkingLayer::NotifyEmitted()`
> 3. Add `MemMgr` to `MemMgrs`
> 4. `MaterializationResponsibility::notifyEmitted()`
>
> ?
>
> This would then require to also undo adding the memory manager and other things by calling `handleRemoveResources`, if `MaterializationResponsibility::notifyEmitted()` failed.
>
> The OrcJITTests seem to be OK with this...

If the unit and regression tests pass then I'd be inclined to land that. If it breaks out-of-tree clients we may need to revert and reconsider.

What's your JIT use case and architecture? Have you looked at `ObjectLinkingLayer` / `JITLink` yet? We're going to deprecate (and eventually remove) RuntimeDyld in favor of JITLink, so it's even more important that we get this right in `ObjectLinkingLayer`.


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

https://reviews.llvm.org/D150184