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

[Charles Saternos via Phabricator via llvm-commits]

ncharlie added inline comments.


================
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);
     }
----------------
dexonsmith wrote:
> 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?
> Notice that in MDOperand, MetadataTracking is sometimes called with an Owner argument.

Yep, I can confirm that the only places I've seen the `Owner` argument used is in `MDNode::setOperand`. It's also used in `MetadataAsValue::track` (though I don't think I've seen that happen in my test cases).

> 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?

Yeah - this placeholder is only used for distinct nodes (the only place the they're created is when an attempt to get a distinct and resolved metadata fails: https://reviews.llvm.org/diffusion/L/browse/llvm/trunk/lib/Bitcode/Reader/MetadataLoader.cpp;303994$926)




================
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);
----------------
dexonsmith wrote:
> 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`.
`Use = nullptr` is more direct and shorter, but they both accomplish the same goal. I can have it call `untrack` instead if you want.


================
Comment at: test/Linker/Inputs/lazy-load-temporaries-cleanup.b.ll:5
+
+!llvm.asan.globals = !{!0, !2, !4, !6, !8, !10, !12}
+
----------------
dexonsmith wrote:
> Does this need to be `!llvm.asan.globals`, or can you make it `!named` and then prune the arguments to something minimal?
I can use `!named` but I don't think I can simplify the number of arguments any further, otherwise lazy loading isn't triggered.


https://reviews.llvm.org/D33513