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

[Jonas Devlieghere via Phabricator via lldb-commits]

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


================
Comment at: lldb/include/lldb/Utility/Log.h:231
                    llvm::StringRef function, const char *format,
-                   Args &&... args) {
+                   Args &&...args) {
     Format(file, function,
----------------
DavidSpickett wrote:
> These seem unrelated but not doing any harm.
This was unintentional, probably my editor removing trailing whitespace and clang-format kicking in because the line got changed.


================
Comment at: lldb/source/API/SBDebugger.cpp:222
 
+static void DumpDiagnostics(void* cookie) {
+  Diagnostics::Instance().Dump(llvm::errs());
----------------
DavidSpickett wrote:
> What is `cookie` used for? (or rather, used elsewhere)
It's like the `baton` we sometimes pass around, just a way to pass additional data to your signal handler. 


================
Comment at: lldb/source/Utility/Diagnostics.cpp:54
+bool Diagnostics::Dump(raw_ostream &stream) {
+  SmallString<128> diagnostics_dir;
+  std::error_code ec =
----------------
labath wrote:
> I am not sure how common this is, but I have recently seen (not in lldb, but another app) a bug, which essentially caused two threads to crash at once (one SEGV, one ABRT). In those situations, you probably don't want to crash-printing routines to race with each other. You can consider putting a (global) mutex in this function, or something like that. (unless the llvm function takes care of that already).
LLVM uses an atomic flag to make sure an exception handler is only run once. 


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

https://reviews.llvm.org/D134991