[PATCH] D120781: [IRLinker] materialize Functions before moving any

Mon Mar 7 14:36:34 PST 2022

nickdesaulniers added inline comments.

================
Comment at: llvm/lib/Linker/IRMover.cpp:1522-1526
+  for (GlobalValue *GV : Worklist)
+    if (auto *F = dyn_cast<Function>(GV))
+      if (!F->isDeclaration())
+        if (Error E = F->materialize())
+          return E;
----------------
dexonsmith wrote:
> Can `materialize()` add things to the Worklist? If so, I don't think the ranged-based for is safe.
> 
> I also worry if it's sufficient; can the worklist loop below add new functions to the worklist, which haven't been materialized ahead of time?
> Can materialize() add things to the Worklist? If so, I don't think the ranged-based for is safe.

I don't think so (though please do triple check).  `IRLinker::maybeAdd` is the only method that adds to the worklist. That is only called from `IRLinker`'s constructor and `IRLinker::shouldLink`. While `IRLinker::shouldLink` is called from `IRLinker::materialize`, the method I'm calling here is `GlobalValue::materialize` (which through some indirection is a call to `BitcodeReader::materialize`.  While these methods are all called `materialize`, `BitcodeReader` doesn't (AFAICT) have any reference to `IRLinker`'s private `Worklist` member.

To triple check the other calls of `IRLinker::shouldLink`, I see `IRLinker::linkAppendingVarProto` and `IRLinker::linkGlobalValueProto`. The latter may call the former. The former is only called from `IRLinker::materialize`.

So check my work, but it looks to me like the answer to `Can materialize() add things to the Worklist?` is "No; `IRLinker::materialize` can, but I'm not calling that; I'm calling `GlobalValue::materialize`". So I don't think there's an iterator invalidation bug here as implied by the question.

Working on a reply to the second question...stay tuned.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120781