[Lldb-commits] [PATCH] D134991: [lldb] Add diagnostics

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/source/Target/Statistics.cpp:293
+
+llvm::Error DebuggerStats::Dump(const FileSpec &dir) {
+  for (size_t debugger_idx = 0; debugger_idx < Debugger::GetNumDebuggers();
----------------
Should this return a Expected<std::vector<std::string>> in case the caller wants to know which files were created by this call?


================
Comment at: lldb/source/Target/Statistics.cpp:305
+    std::error_code ec;
+    raw_fd_ostream stats_stream(stat_file.GetPath(), ec, sys::fs::OF_None);
+    if (ec)
----------------
What happens if the file already exists here? Will this return an error? I am guessing we are passing in a temp directory for the logs


================
Comment at: lldb/source/Target/Statistics.cpp:309
+
+    stats_stream << ReportStatistics(*debugger_sp, nullptr);
+  }
----------------
There are many syscalls and OS APIs that aren't supposed to be called during a interrupt handler, and this function might call many of them. Deadlocks are a serious concern here as well since any thread might have a mutex locked that could stop the statistics reporting in its tracks since the program was asynchronously interrupted. If this does happen this process will deadlock forever and not be able to respond. We might need a bool parameter do "ReportStatistics" like "bool during_interrupt" and if this is true, we need to try and lock any module mutexes and if they fail, don't report that module. So the entire ReportStatistics function will need to be checked for potential deadlocks, mostly the Module mutex.


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

https://reviews.llvm.org/D134991