[lldb] Add diagnostics

[JDevlieghere]

Summary

Around this time last year, I said on the mailing list [1] that I wanted to to transform the reproducers into something that resembles a sysdiagnose on Apple platforms: a collection of files containing a variety of information to help diagnose bugs or troubleshoot issues. This patch adds that framework. Based on lessons learned from the reproducers, I've intentionally tried to keep it small and simple. Different parts of LLDB can register callbacks (this is necessary for layering purposes) that will get called when the diagnostics should be generated.

For this patch, it's hooked up to Greg's statistics, but I have a bunch more things in the pipeline. With the minimal implementation in this patch, when the command line LLDB crashes, it dump the JSON statistics into a temp directory and ask the user to attach them to their bug report.

LLDB diagnostics written to /var/folders/6v/6zkjmd45211_1vxrtbnl0dj00000gn/T/diagnostics-ea34d4
Please include the directory content when filing a bug report

[1] https://lists.llvm.org/pipermail/lldb-dev/2021-September/017045.html

[JDevlieghere]

Fix baseline

[DavidSpickett]

I like the direction, thanks for working on this.

To no one's surprise, I was thinking about testing :)

• Errors related to the filesystem e.g. can't open a path, is too complicated to setup on demand. The code looks to be passing on the error, which is good enough most of the time.
• The callbacks are added in c++, so testing if you add A then remove B and so on doesn't make sense on the same copy of lldb (and upstream will always have the same set of callbacks anyway).
• Dumping statistics uses an existing method, you just make up the filename. So it's the statistics code's responsibility to test what that function does.

Maybe there could be a smoke test that just checks that the dir is made and has some files ending in stats.json in it? I think clang does something like this though they may have a --please-crash option too.

Unrelated - there's maybe a situation where the same set of callbacks are added in a different order, perhaps? But am I correct in thinking that this isn't an issue because these will be writing to different files? Stats has one set of files, logging has another set, etc. So the end result is always a dir of separate files one or more for each thing that registers.

lldb/include/lldb/Utility/Diagnostics.h

30

Can this be private? Code should only use Initialize if I am reading this right.

lldb/include/lldb/Utility/Log.h

231 ↗

(On Diff #464396)

These seem unrelated but not doing any harm.

lldb/source/API/SBDebugger.cpp

222

What is cookie used for? (or rather, used elsewhere)

lldb/source/Utility/Diagnostics.cpp

44

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)

[labath]

Adding @rupprecht, as he might be interested in following this.

I don't want to get too involved in this, but the first thought on my mind is "do you really want to do all this from within a signal handler?". The fact that we're running this code means that we're already in a bad state, and its pretty hard to say how far we will manage to get without triggering another crash. At the very least, I think we should print the crash dump directory as the first order of business, before we start running random callbacks.

There are various ways to solve that, like moving the dumping code to another process, or being very careful about what you run inside the crash handler. The one thing that they all have in common is that they are harder to design/implement that what you have now. So, if you want try this out, and accept the risk that it may not be enough to capture all the crashes you're interested in, then I won't stand in your way...

lldb/include/lldb/Target/Statistics.h

180 ↗

(On Diff #464396)

What's the difference between this class and DebuggerStats above? e.g. if I wanted to add some new method, how would I know where to add it?

lldb/source/Utility/Diagnostics.cpp

55

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).

[JDevlieghere]

In D134991#3830682, @DavidSpickett wrote:

I like the direction, thanks for working on this.

To no one's surprise, I was thinking about testing :)

• Errors related to the filesystem e.g. can't open a path, is too complicated to setup on demand. The code looks to be passing on the error, which is good enough most of the time.
• The callbacks are added in c++, so testing if you add A then remove B and so on doesn't make sense on the same copy of lldb (and upstream will always have the same set of callbacks anyway).
• Dumping statistics uses an existing method, you just make up the filename. So it's the statistics code's responsibility to test what that function does.

Maybe there could be a smoke test that just checks that the dir is made and has some files ending in stats.json in it? I think clang does something like this though they may have a --please-crash option too.

I appreciate you thinking this through. Back in the days of the reproducers, we would generate a reproducer on a crash (similar to what we do here) or through a command (reproducer generate). I think we'll want to a command here too for the case where something went wrong but without crashing LLDB. I think that part should be easy to test and I was planning on adding a test as part of that.

Unrelated - there's maybe a situation where the same set of callbacks are added in a different order, perhaps? But am I correct in thinking that this isn't an issue because these will be writing to different files? Stats has one set of files, logging has another set, etc. So the end result is always a dir of separate files one or more for each thing that registers.

Yeah, in my mind all the callbacks should be orthogonal. If it wasn't for the layering, I'd make it all the responsibility of the Diagnostics class. If someone has a better idea than callbacks please let me know.

In D134991#3830744, @labath wrote:

Adding @rupprecht, as he might be interested in following this.

I don't want to get too involved in this, but the first thought on my mind is "do you really want to do all this from within a signal handler?". The fact that we're running this code means that we're already in a bad state, and its pretty hard to say how far we will manage to get without triggering another crash.

We had a similar issue for the reproducers. In my experience, we mostly got away with it, but as you said, there's no guarantees. It all depends on what exactly what the callback is doing. Maybe that's something that should be controllable: e.g. some callbacks might want to do more/less based on whether they're called from a signal handler vs when they're triggered from a command.

That said, I see the crash case as a "best effort" kind of thing.

At the very least, I think we should print the crash dump directory as the first order of business, before we start running random callbacks.

That's a good point.

There are various ways to solve that, like moving the dumping code to another process, or being very careful about what you run inside the crash handler. The one thing that they all have in common is that they are harder to design/implement that what you have now. So, if you want try this out, and accept the risk that it may not be enough to capture all the crashes you're interested in, then I won't stand in your way...

Yup that's the approach we took for the reproducer. If you remember, we found that copying a bunch of files wasn't the best thing to do in a signal handler (surprise surprise) so we moved that work out of process (but still wrote out the mapping, similar to clang). I think we can take a similar approach here.

[DavidSpickett]

I think that part should be easy to test and I was planning on adding a test as part of that.

Sounds good to me. Nothing needed in this change then.

A command also gives you a chance to get the diagnostics out if running until the actual crash fails to dump them, potentially due to the signal handler issues Pavel is referring to.

Yeah, in my mind all the callbacks should be orthogonal. If it wasn't for the layering, I'd make it all the responsibility of the Diagnostics class. If someone has a better idea than callbacks please let me know.

I do like that they keep the Diagnostics class free of details of the others. Which means you just extend the existing tests for those other classes each time you add a dump function to them.

[JDevlieghere]

lldb/include/lldb/Utility/Log.h

231 ↗

(On Diff #464396)

This was unintentional, probably my editor removing trailing whitespace and clang-format kicking in because the line got changed.

lldb/source/API/SBDebugger.cpp

222

It's like the baton we sometimes pass around, just a way to pass additional data to your signal handler.

lldb/source/Utility/Diagnostics.cpp

55

LLVM uses an atomic flag to make sure an exception handler is only run once.

[JDevlieghere]

Address code review feedback

[JDevlieghere]

Changed my mind (again). I made the Callback a std::function so we can use lambas. I used function pointers originally so we could easily compare and remove callbacks, but there's too many places where this overcomplicates things.

[clayborg]

lldb/source/Target/Statistics.cpp

293 ↗

(On Diff #466549)

Should this return a Expected<std::vector<std::string>> in case the caller wants to know which files were created by this call?

305 ↗

(On Diff #466549)

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

309 ↗

(On Diff #466549)

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.

[DavidSpickett]

lldb/source/Utility/Diagnostics.cpp

44

Is this still relevant?

[JDevlieghere]

lldb/source/Target/Statistics.cpp

305 ↗

(On Diff #466549)

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.

309 ↗

(On Diff #466549)

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.

lldb/source/Utility/Diagnostics.cpp

44

Not anymore, you cannot compare std::functions.

[JDevlieghere]

Split off the statistics

[JDevlieghere]

Ping

[clayborg]

lldb/lldb/include/lldb/Utility/Diagnostics.h

1 ↗

(On Diff #469399)

There are two of these files:

lldb/include/lldb/Utility/Diagnostics.h
lldb/lldb/include/lldb/Utility/Diagnostics.h

Remove this one I assume?

lldb/lldb/source/Utility/Diagnostics.cpp

1 ↗

(On Diff #469399)

Is "lldb/lldb/source/Utility/Diagnostics.cpp" the right directory? Seems wrong

lldb/source/Utility/Diagnostics.cpp

2

Ah, this is the right lldb/source/Utility/Diagnostics.cpp file. Remove the previous lldb/lldb/source/Utility/Diagnostics.cpp incorrect pathed file.

[JDevlieghere]

Remove spurious lldb subdir

lldb/include/lldb/API/SBDebugger.h

public:
   lldb::SBDebugger &operator=(const lldb::SBDebugger &rhs);
 
   static void Initialize();
 
   static lldb::SBError InitializeWithErrorHandling();
 
   static void PrintStackTraceOnError();
 
+  static void PrintDiagnosticsOnError();
+
   static void Terminate();
 
   // Deprecated, use the one that takes a source_init_files bool.
   static lldb::SBDebugger Create();
 
   static lldb::SBDebugger Create(bool source_init_files);
 
   static lldb::SBDebugger Create(bool source_init_files,

lldb/include/lldb/Utility/Diagnostics.h

+//===-- Diagnostics.h -------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_UTILITY_DIAGNOSTICS_H
+#define LLDB_UTILITY_DIAGNOSTICS_H
+
+#include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/Log.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/Support/Error.h"
+
+#include <functional>
+#include <mutex>
+#include <vector>
+
+namespace lldb_private {
+
+/// Diagnostics are a collection of files to help investigate bugs and
+/// troubleshoot issues. Any part of the debugger can register itself with the
+/// help of a callback to emit one or more files into the diagnostic directory.
+class Diagnostics {
+public:
+  Diagnostics();
+  ~Diagnostics();
+
+  /// Gather diagnostics in the given directory.
+  llvm::Error Create(const FileSpec &dir);
+
+  /// Gather diagnostics and print a message to the given output stream.
+  bool Dump(llvm::raw_ostream &stream);
+
+  using Callback = std::function<llvm::Error(const FileSpec &)>;
+
+  void AddCallback(Callback callback);
+
+  static Diagnostics &Instance();
+  static void Initialize();
+  static void Terminate();
+
+private:
+  static llvm::Optional<Diagnostics> &InstanceImpl();
+
+  llvm::SmallVector<Callback, 4> m_callbacks;
+  std::mutex m_callbacks_mutex;
+};
+
+} // namespace lldb_private
+
+#endif

lldb/source/API/SBDebugger.cpp

 #include "lldb/Host/XML.h"
 #include "lldb/Initialization/SystemLifetimeManager.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/OptionArgParser.h"
 #include "lldb/Interpreter/OptionGroupPlatform.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/TargetList.h"
 #include "lldb/Utility/Args.h"
+#include "lldb/Utility/Diagnostics.h"
 #include "lldb/Utility/State.h"
 #include "lldb/Version/Version.h"
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/DynamicLibrary.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/PrettyStackTrace.h"
void SBDebugger::PrintStackTraceOnError() {
   LLDB_INSTRUMENT();
 
   llvm::EnablePrettyStackTrace();
   static std::string executable =
       llvm::sys::fs::getMainExecutable(nullptr, nullptr);
   llvm::sys::PrintStackTraceOnErrorSignal(executable);
 }
 
+static void DumpDiagnostics(void *cookie) {
+  Diagnostics::Instance().Dump(llvm::errs());
+}
+
+void SBDebugger::PrintDiagnosticsOnError() {
+  LLDB_INSTRUMENT();
+
+  llvm::sys::AddSignalHandler(&DumpDiagnostics, nullptr);
+}
+
 void SBDebugger::Terminate() {
   LLDB_INSTRUMENT();
 
   g_debugger_lifetime->Terminate();
 }
 
 void SBDebugger::Clear() {
   LLDB_INSTRUMENT_VA(this);

lldb/source/Core/Debugger.cpp

 #include "lldb/Target/Language.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StructuredDataPlugin.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/TargetList.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Target/ThreadList.h"
 #include "lldb/Utility/AnsiTerminal.h"
+#include "lldb/Utility/Diagnostics.h"
 #include "lldb/Utility/Event.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Listener.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/State.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/StreamString.h"
 

lldb/source/Initialization/SystemInitializerCommon.cpp

 //===-- SystemInitializerCommon.cpp ---------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Initialization/SystemInitializerCommon.h"
 
 #include "Plugins/Process/gdb-remote/ProcessGDBRemoteLog.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/Socket.h"
+#include "lldb/Target/Statistics.h"
+#include "lldb/Utility/Diagnostics.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Timer.h"
 #include "lldb/Version/Version.h"
 
 #if defined(__linux__) || defined(__FreeBSD__) || defined(__NetBSD__)
 #include "Plugins/Process/POSIX/ProcessPOSIXLog.h"
 #endif
 
if (disable_crash_dialog_var &&
     _CrtSetReportFile(_CRT_ASSERT, _CRTDBG_FILE_STDERR);
     _CrtSetReportFile(_CRT_WARN, _CRTDBG_FILE_STDERR);
     _CrtSetReportFile(_CRT_ERROR, _CRTDBG_FILE_STDERR);
   }
 #endif
 
   InitializeLldbChannel();
 
+  Diagnostics::Initialize();
   FileSystem::Initialize();
   HostInfo::Initialize(m_shlib_dir_helper);
 
   llvm::Error error = Socket::Initialize();
   if (error)
     return error;
 
   LLDB_SCOPED_TIMER();

 #if defined(_WIN32)
   ProcessWindowsLog::Terminate();
 #endif
 
   Socket::Terminate();
   HostInfo::Terminate();
   Log::DisableAllLogChannels();
   FileSystem::Terminate();
+  Diagnostics::Terminate();
 }

lldb/source/Utility/CMakeLists.txt

add_lldb_library(lldbUtility
   Broadcaster.cpp
   CompletionRequest.cpp
   Connection.cpp
   ConstString.cpp
   DataBufferHeap.cpp
   DataBufferLLVM.cpp
   DataEncoder.cpp
   DataExtractor.cpp
+  Diagnostics.cpp
   Environment.cpp
   Event.cpp
   FileSpec.cpp
   GDBRemote.cpp
   IOObject.cpp
   LLDBAssert.cpp
   LLDBLog.cpp
   Listener.cpp

lldb/source/Utility/Diagnostics.cpp

+//===-- Diagnostics.cpp ---------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Utility/Diagnostics.h"
+#include "lldb/Utility/LLDBAssert.h"
+
+#include "llvm/Support/Error.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+
+using namespace lldb_private;
+using namespace lldb;
+using namespace llvm;
+
+void Diagnostics::Initialize() {
+  lldbassert(!InstanceImpl() && "Already initialized.");
+  InstanceImpl().emplace();
+}
+
+void Diagnostics::Terminate() {
+  lldbassert(InstanceImpl() && "Already terminated.");
+  InstanceImpl().reset();
+}
+
+Optional<Diagnostics> &Diagnostics::InstanceImpl() {
+  static Optional<Diagnostics> g_diagnostics;
+  return g_diagnostics;
+}
+
+Diagnostics &Diagnostics::Instance() { return *InstanceImpl(); }
+
+Diagnostics::Diagnostics() {}
+
+Diagnostics::~Diagnostics() {}
+
+void Diagnostics::AddCallback(Callback callback) {
+  std::lock_guard<std::mutex> guard(m_callbacks_mutex);
+  m_callbacks.push_back(callback);
+}
+
+bool Diagnostics::Dump(raw_ostream &stream) {
+  SmallString<128> diagnostics_dir;
+  std::error_code ec =
+      sys::fs::createUniqueDirectory("diagnostics", diagnostics_dir);
+  if (ec) {
+    stream << "unable to create diagnostic dir: "
+           << toString(errorCodeToError(ec)) << '\n';
+    return false;
+  }
+
+  stream << "LLDB diagnostics written to " << diagnostics_dir << "\n";
+  stream << "Please include the directory content when filing a bug report\n";
+
+  Error error = Create(FileSpec(diagnostics_dir.str()));
+  if (error) {
+    stream << toString(std::move(error)) << '\n';
+    return false;
+  }
+
+  return true;
+}
+
+Error Diagnostics::Create(const FileSpec &dir) {
+  for (Callback c : m_callbacks) {
+    if (Error err = c(dir))
+      return err;
+  }
+  return Error::success();
+}

lldb/tools/driver/Driver.cpp

int main(int argc, char const *argv[]) {
   }
 
   SBError error = SBDebugger::InitializeWithErrorHandling();
   if (error.Fail()) {
     WithColor::error() << "initialization failed: " << error.GetCString()
                        << '\n';
     return 1;
   }
+
+  // Setup LLDB signal handlers once the debugger has been initialized.
+  SBDebugger::PrintDiagnosticsOnError();
+
   SBHostOS::ThreadCreated("<lldb.driver.main-thread>");
 
   signal(SIGINT, sigint_handler);
 #if !defined(_WIN32)
   signal(SIGPIPE, SIG_IGN);
   signal(SIGWINCH, sigwinch_handler);
   signal(SIGTSTP, sigtstp_handler);
 #endif