[PATCH] D71059: [LLD][ELF] Add time-trace to ELF LLD (1/2)

[Arthur O'Dwyer via Phabricator via llvm-commits]

Quuxplusone added inline comments.


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:254
 void timeTraceProfilerCleanup() {
   delete TimeTraceProfilerInstance;
+  std::lock_guard<std::mutex> Lock(Mu);
----------------
Can `TimeTraceProfilerInstance` ever actually be non-null here?
If so, are you at all worried that this unguarded access in `timeTraceProfilerCleanup()` might race with the guarded access in `timeTraceProfilerFinishThread()`?


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:263
+  std::lock_guard<std::mutex> Lock(Mu);
+  ThreadTimeTraceProfilerInstances.push_back(TimeTraceProfilerInstance);
   TimeTraceProfilerInstance = nullptr;
----------------
Once this `new`'ed pointer gets into the vector, I don't see any codepath that ever calls `delete` on the pointers in the vector — `timeTraceProfilerCleanup()` just calls `clear()` and drops them on the floor without deleting. Am I missing something?


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

https://reviews.llvm.org/D71059