[Orc] Fix race condition in DebugObjectManagerPlugin

[sgraenitz]

Summary

During finalization the debug object is registered with the target. Materialization must wait for this process to finish. Otherwise we might start running code before the debugger finished processing the corresponding debug info.

[sgraenitz]

This means that materialization is blocked until finalization of the debug object allocation is done, i.e.:

• memory was copied over to the target
• an entry was added to the JIT descriptor (locks a global mutex)
• the executing process ran into the rendezvous breakpoint (1st process switch)
• the debugger processed the debug object, read the symbol table and returned control back to the executor (2nd process switch)

This might introduce a considerable delay in materialization. I wonder if this points out an interesting design question for the plugin API: Debug registration could run safely in parallel up to the point that execution reaches the emitted code. The only way I see to achieve it with the current API is blocking the notifyEmitted() callback. Could we instead return the promise and wait for it at a later point in time?

[lhames]

LGTM.

This might introduce a considerable delay in materialization. I wonder if this points out an interesting design question for the plugin API...

I think you're right, but rather than approach this through the plugin API I think the solution will come from rethinking "finalization" on the memory manager API: If we represented all the various pieces of metadata (eh-frames, objc registration, debug registration, etc.) as sections in the allocation, and allowed additional operations to be attached to the finalization call, then we could do everything in one RPC round trip.

[sgraenitz]

I think you're right, but rather than approach this through the plugin API I think the solution will come from rethinking "finalization" on the memory manager API

Alright, that sounds reasonable. Having this delay for debugging use cases should be acceptable for the moment. It can be avoided by feeding in modules without debug info. The plugin detects that and stops any further processing early on.

llvm/lib/ExecutionEngine/Orc/DebugObjectManagerPlugin.cpp

 #include "llvm/ADT/StringRef.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/ExecutionEngine/JITLink/JITLinkDylib.h"
 #include "llvm/ExecutionEngine/JITLink/JITLinkMemoryManager.h"
 #include "llvm/ExecutionEngine/JITSymbol.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Errc.h"
+#include "llvm/Support/MSVCErrorWorkarounds.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include <set>
 
 #define DEBUG_TYPE "orc"
 
Error DebugObjectManagerPlugin::notifyEmitted(
   std::lock_guard<std::mutex> Lock(PendingObjsLock);
   auto It = PendingObjs.find(Key);
   if (It == PendingObjs.end())
     return Error::success();
 
   DebugObject *UnownedDebugObj = It->second.release();
   PendingObjs.erase(It);
 
+  // During finalization the debug object is registered with the target.
+  // Materialization must wait for this process to finish. Otherwise we might
+  // start running code before the debugger processed the corresponding debug
+  // info.
+  std::promise<MSVCPError> FinalizePromise;
+  std::future<MSVCPError> FinalizeErr = FinalizePromise.get_future();
+
   // FIXME: We released ownership of the DebugObject, so we can easily capture
   // the raw pointer in the continuation function, which re-owns it immediately.
   if (UnownedDebugObj)
     UnownedDebugObj->finalizeAsync(
-        [this, Key, UnownedDebugObj](Expected<sys::MemoryBlock> TargetMem) {
+        [this, Key, UnownedDebugObj,
+         &FinalizePromise](Expected<sys::MemoryBlock> TargetMem) {
           std::unique_ptr<DebugObject> ReownedDebugObj(UnownedDebugObj);
           if (!TargetMem) {
-            ES.reportError(TargetMem.takeError());
+            FinalizePromise.set_value(TargetMem.takeError());
             return;
           }
           if (Error Err = Target->registerDebugObject(*TargetMem)) {
-            ES.reportError(std::move(Err));
+            FinalizePromise.set_value(std::move(Err));
             return;
           }
 
+          // Registration successful, notifyEmitted() can return now and
+          // materialization can finish.
+          FinalizePromise.set_value(Error::success());
+
           std::lock_guard<std::mutex> Lock(RegisteredObjsLock);
           RegisteredObjs[Key].push_back(std::move(ReownedDebugObj));
         });
 
-  return Error::success();
+  return FinalizeErr.get();
 }
 
 Error DebugObjectManagerPlugin::notifyFailed(
     MaterializationResponsibility &MR) {
   std::lock_guard<std::mutex> Lock(PendingObjsLock);
   PendingObjs.erase(getResourceKey(MR));
   return Error::success();
 }