[PATCH] D98785: [Orc] Fix pending debug object tracking in DebugObjectManagerPlugin

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: llvm/lib/ExecutionEngine/Orc/DebugObjectManagerPlugin.cpp:522
+    // is still in-flight. If we have pending debug objects for such MRs, we
+    // will remove them here.
+    // FIXME: Getting the resource key from a MaterializationResponsibility is
----------------
sgraenitz wrote:
> Can we guaranteed that affected in-flight MRs are still alive at this point? Running the code as is works, but I wonder if it's a robust assumption.
We can guarantee that no MR is trying to register resources with the key at the point that `notifyRemovingResources` is called, provided that keys are never leaked from `withResourceKeyDo` -- see below. :)


================
Comment at: llvm/lib/ExecutionEngine/Orc/DebugObjectManagerPlugin.cpp:530
+    for (const auto &KV : PendingObjs) {
+      if (K == getResourceKey(*KV.first))
+        PendingMRsForKey.push_back(KV.first);
----------------
sgraenitz wrote:
> It's a different issue, but I realized that this might be a dangerous use of resource keys. They are only guaranteed to be valid within the lambda passed into `withResourceKeyDo()` right?
Yep. I missed this on my first read through.

You should always use `MaterializationResponsibility::withResourceKeyDo` to create associations between resources and keys, and never leak the key from inside `withResourceKeyDo`. This system guarantees that the association never ends up in a race with resource removal: Any race becomes ordered as either: ASSOCIATE then REMOVE (normal removal case), or REMOVE then ASSOCIATE (in which case `withResourceKey` do will return a `ResourceTrackerDefunct` error and not run your lambda).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98785