[PATCH] D98785: [Orc] Fix pending debug object tracking in DebugObjectManagerPlugin
Stefan Gränitz via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 18 06:33:27 PDT 2021
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
More information about the llvm-commits
mailing list