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

[Russell Gallop via Phabricator via llvm-commits]

russell.gallop marked 3 inline comments as done.
russell.gallop added inline comments.


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:254
 void timeTraceProfilerCleanup() {
   delete TimeTraceProfilerInstance;
+  std::lock_guard<std::mutex> Lock(Mu);
----------------
Quuxplusone wrote:
> 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()`?
It looks like Phabricator has lost the location of your comments since I updated the patch. This comment was made against timeTraceProfilerCleanup().

> Can TimeTraceProfilerInstance ever actually be non-null here?

Yes, it is the instance used on the main thread (rather than worker threads from the thread pool) so it will be non-null when time-trace is enabled.

> If so, are you at all worried that this unguarded access in timeTraceProfilerCleanup() might race with the guarded access in timeTraceProfilerFinishThread()?

Do you mean unguarded access to TimeTraceProfilerInstance? TimeTraceProfilerInstance does not need guarding as it is not shared, ThreadTimeTraceProfilerInstances does.

With LLVM_ENABLE_THREADS=ON TimeTraceProfilerInstance is thread local so no guard is required. With LLVM_ENABLE_THREADS=OFF there are no other threads calling timeTraceProfilerFinishThread() to race.

I'll add a comment to timeTraceProfilerCleanup() to try and clarify.


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:263
+  std::lock_guard<std::mutex> Lock(Mu);
+  ThreadTimeTraceProfilerInstances.push_back(TimeTraceProfilerInstance);
   TimeTraceProfilerInstance = nullptr;
----------------
Quuxplusone wrote:
> 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?
Ah, you're right. Fixed. Thanks.


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

https://reviews.llvm.org/D71059