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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 2 inline comments as done.
JDevlieghere added inline comments.


================
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)
----------------
clayborg wrote:
> 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
The file will get overwritten. My reasoning was that maybe you generated the statistics and then you were like oh wait an error showed up after I did a step, let me regenerate them. Alternatives are:

 - Erroring out if the file exists
 - Erroring out if any file exists in the diagnostic dir

I prefer that because we can check it once and give an actionable error to the user. Let me know what you think. 


================
Comment at: lldb/source/Target/Statistics.cpp:309
+
+    stats_stream << ReportStatistics(*debugger_sp, nullptr);
+  }
----------------
clayborg wrote:
> 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.
I thought almost everything in the statistics was computed upfront, but if that's not the case then we need to be careful. I picked the statistics as an example of some data that can be emitted here. I don't want to block the broader infrastructure on that so I'll hoist it into a separate patch, similar to the ones for the logs. 


================
Comment at: lldb/source/Utility/Diagnostics.cpp:43
+  std::lock_guard<std::mutex> guard(m_callbacks_mutex);
+  m_callbacks.push_back(callback);
+}
----------------
DavidSpickett wrote:
> DavidSpickett wrote:
> > Is it worth adding an assert that the callback is not already in the list?
> > 
> > (and below, that the callback is in fact in the list)
> Is this still relevant?
Not anymore, you cannot compare std::functions. 


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

https://reviews.llvm.org/D134991