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

[Stefan Gränitz via Phabricator via llvm-commits]

sgraenitz marked 2 inline comments as done.
sgraenitz added a comment.

Thanks for elaborating.



================
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
----------------
lhames wrote:
> 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. :)
It turned out that I don't need to worry about pending debug objects here, which renders my question irrelevant.


================
Comment at: llvm/lib/ExecutionEngine/Orc/DebugObjectManagerPlugin.cpp:530
+    for (const auto &KV : PendingObjs) {
+      if (K == getResourceKey(*KV.first))
+        PendingMRsForKey.push_back(KV.first);
----------------
lhames wrote:
> 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).
Fixing this in D98863


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98785