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

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

dexonsmith added a comment.

The current patch seems to be missing context on Phab (`-U999999`).  Please remember to add it on the next iteration.



================
Comment at: include/llvm/IR/Metadata.h:1280
     *Use = MD;
+    if(*Use)
+      MetadataTracking::track(*Use);
----------------
Nit: should be a space between `if` and `(*Use)`.  Running clang-format on your diff should clean this up.  See the script at the bottom of the docs:
https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting


================
Comment at: include/llvm/IR/Metadata.h:1282
+      MetadataTracking::track(*Use);
     Use = nullptr;
   }
----------------
Please at least add a comment saying that this is equivalent to `MetadataTracking::untrack`.

I do weakly prefer the `untrack` and `assert` combination, because it's easier to scan the code looking for balanced track/untrack calls.  Also easier to add logging to track/untrack to sort out what's going on when there's a bug.  I guess my point is it improves how clear the code is globally.

But it's a weak preference, and `Use = nullptr` is certainly more clear locally.  What do others think?


================
Comment at: tools/llvm-dis/llvm-dis.cpp:145-147
   std::unique_ptr<Module> M =
       ExitOnErr(getOwningLazyBitcodeModule(std::move(MB), Context,
+                                           /*ShouldLazyLoadMetadata=*/true, true));
----------------
I haven't thought about whether this should be hardcoded (likely depends on whether we're losing important coverage in the `false` case -- have you looked at that?), but there should certainly be a `/*What is this doing?=*/` comment.


https://reviews.llvm.org/D33513