[PATCH] D33513: [ThinLTO] Fix ThinLTO crash while destroying context

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a reviewer: mehdi_amini.
dexonsmith added a comment.

I'm probably a good person to review this, but it's been a while since I've thought this so I might need help to refresh my context.  This use-tracking code is unfortunately rather subtle.

Also, the testcase looks like it could possibly be reduced...



================
Comment at: include/llvm/IR/Metadata.h:716-723
   void track(Metadata *Owner) {
     if (MD) {
       if (Owner)
         MetadataTracking::track(this, *MD, *Owner);
       else
         MetadataTracking::track(MD);
     }
----------------
Notice that in `MDOperand`, `MetadataTracking` is sometimes called with an `Owner` argument.

This seems to only be used when `MDNode::setOperand` is called on a *uniqued* metadata node, or during `MDNode` initialization, when `MDNode::makeUniqued()` is called.

Since these operands are only for *distinct* metadata nodes, what you've done below looks correct... can you confirm you have the same understanding I do?


================
Comment at: include/llvm/IR/Metadata.h:1275-1283
   /// Replace the use of this with MD.
   void replaceUseWith(Metadata *MD) {
     if (!Use)
       return;
     *Use = MD;
+    if(*Use)
+      MetadataTracking::track(*Use);
----------------
I wonder if it would be more or less clear to use:

    MetadataTracking::untrack(this);
    assert(!Use && "Somehow this is still being tracked...");

here, instead of just `Use = nullptr`.


================
Comment at: test/Linker/Inputs/lazy-load-temporaries-cleanup.b.ll:5
+
+!llvm.asan.globals = !{!0, !2, !4, !6, !8, !10, !12}
+
----------------
Does this need to be `!llvm.asan.globals`, or can you make it `!named` and then prune the arguments to something minimal?


Repository:
  rL LLVM

https://reviews.llvm.org/D33513