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

[Moritz Kroll via Phabricator via llvm-commits]

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


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

https://reviews.llvm.org/D150184