[lldb] Add a "diagnostics dump" command

[JDevlieghere]

Summary

Add a "diagnostics dump" command to, as the name implies, dump the diagnostics to disk. The goal of this command is to let the user generate the diagnostics in case of an issue that doesn't cause the debugger to crash. This is also useful for testing.

[DavidSpickett]

lldb/source/Commands/CommandObjectDiagnostics.cpp

86

AppendError sets the status for you now.

108

Is "log" correct here?

lldb/source/Commands/Options.td

348

Worth clarifying whether the path can be absolute or relative to the current working dir?

I'd guess either works.

lldb/source/Utility/Diagnostics.cpp

50

You std::move'd some errors above does that apply here?

(I'm not sure exactly what requires it and what doesn't)

61

Silly question, is it intentional that we write out files after saying they have been written?

Of course it makes sense to print something up front, otherwise the error here would come out of the blue, but perhaps "LLDB diagnostics will be written to..." instead?

(this is probably better changed in that earlier patch not sure if that went in yet)

lldb/test/Shell/Diagnostics/TestDump.test

2

"correct directory and creates a new one if needed." ?

[clayborg]

lldb/source/Commands/CommandObjectDiagnostics.cpp

85

Do we want to just create a temp directory if the user didn't specify one here and them emit the path to where things were saved instead of erroring out?

lldb/source/Utility/Diagnostics.cpp

46–47

How are we dumping these to the directory the user specifies the directory in "diagnostic dump"? Below we create a unique directory. Seems like this API should take a "Optional<FileSpec> directory" parameter and if it doesn't have a value, then create a unique directory?

[JDevlieghere]

Address code review feedback

lldb/source/Commands/CommandObjectDiagnostics.cpp

85

Yes, that's exactly what we do. If we get here we were unable to create either of them.

lldb/source/Commands/Options.td

348

Given that either works I think we don't need to be explicit about it.

lldb/source/Utility/Diagnostics.cpp

50

toString takes ownership of the error, so it needs to be moved here. Errors cannot be copied, so they always need to be moved, except when RVO kicks in.

61

Yes, that was @labath's suggestion because when we call this from the signal handler, we might crash again in the process and wouldn't print anything at all.

[DavidSpickett]

lldb/source/Utility/Diagnostics.cpp

61

Maybe I can be clearer:

LLDB diagnostics written to <...>
Please include the directory content when filing a bug report
<we crash during writing the files>

So the files haven't been written, or at least they are in an undefined state, but the output looks like we finished writing them. So will be written is more accurate. If I see this in a test log file I'm going to wonder did it crash writing them or did it already write them, which means I'd have to reproduce locally. With the sequence of events clearer, I can at least check obvious things like a full disk before needing to do that.

Small thing but it's a few words might save some annoyance down the road.

(printing up front I agree with, that part is good)

[JDevlieghere]

Update wording

lldb/source/Utility/Diagnostics.cpp

61

Makes sense!

[DavidSpickett]

LGTM

[stella.stamenova]

The newly added test is failing on the windows buildbot: https://lab.llvm.org/buildbot/#/builders/83/builds/25446

lldb/include/lldb/Utility/Diagnostics.h

 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);
+  bool Dump(llvm::raw_ostream &stream, const FileSpec &dir);
+  /// @}
 
   using Callback = std::function<llvm::Error(const FileSpec &)>;
 
   void AddCallback(Callback callback);
 
   static Diagnostics &Instance();
   static void Initialize();
   static void Terminate();
 
+  /// Create a unique diagnostic directory.
+  static llvm::Expected<FileSpec> CreateUniqueDirectory();
+
 private:
   static llvm::Optional<Diagnostics> &InstanceImpl();
 
   llvm::SmallVector<Callback, 4> m_callbacks;
   std::mutex m_callbacks_mutex;
 };
 
 } // namespace lldb_private
 
 #endif

lldb/source/Commands/CMakeLists.txt

 lldb_tablegen(CommandOptions.inc -gen-lldb-option-defs
   SOURCE Options.td
   TARGET LLDBOptionsGen)
 
 add_lldb_library(lldbCommands
   CommandCompletions.cpp
   CommandObjectApropos.cpp
   CommandObjectBreakpoint.cpp
   CommandObjectBreakpointCommand.cpp
   CommandObjectCommands.cpp
+  CommandObjectDiagnostics.cpp
   CommandObjectDisassemble.cpp
   CommandObjectExpression.cpp
   CommandObjectFrame.cpp
   CommandObjectGUI.cpp
   CommandObjectHelp.cpp
   CommandObjectLanguage.cpp
   CommandObjectLog.cpp
   CommandObjectMemory.cpp

lldb/source/Commands/CommandObjectDiagnostics.h

+//===-- CommandObjectDiagnostics.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_SOURCE_COMMANDS_COMMANDOBJECTDIAGNOSTICS_H
+#define LLDB_SOURCE_COMMANDS_COMMANDOBJECTDIAGNOSTICS_H
+
+#include "lldb/Interpreter/CommandObjectMultiword.h"
+
+namespace lldb_private {
+
+class CommandObjectDiagnostics : public CommandObjectMultiword {
+public:
+  CommandObjectDiagnostics(CommandInterpreter &interpreter);
+  ~CommandObjectDiagnostics() override;
+
+private:
+  CommandObjectDiagnostics(const CommandObjectDiagnostics &) = delete;
+  const CommandObjectDiagnostics &
+  operator=(const CommandObjectDiagnostics &) = delete;
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_SOURCE_COMMANDS_COMMANDOBJECTDIAGNOSTICS_H

lldb/source/Commands/CommandObjectDiagnostics.cpp

+//===-- CommandObjectDiagnostics.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 "CommandObjectDiagnostics.h"
+#include "lldb/Host/OptionParser.h"
+#include "lldb/Interpreter/CommandOptionArgumentTable.h"
+#include "lldb/Interpreter/CommandReturnObject.h"
+#include "lldb/Interpreter/OptionArgParser.h"
+#include "lldb/Interpreter/OptionValueEnumeration.h"
+#include "lldb/Interpreter/OptionValueUInt64.h"
+#include "lldb/Interpreter/Options.h"
+#include "lldb/Utility/Diagnostics.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+#define LLDB_OPTIONS_diagnostics_dump
+#include "CommandOptions.inc"
+
+class CommandObjectDiagnosticsDump : public CommandObjectParsed {
+public:
+  // Constructors and Destructors
+  CommandObjectDiagnosticsDump(CommandInterpreter &interpreter)
+      : CommandObjectParsed(interpreter, "diagnostics dump",
+                            "Dump diagnostics to disk", nullptr) {}
+
+  ~CommandObjectDiagnosticsDump() override = default;
+
+  class CommandOptions : public Options {
+  public:
+    CommandOptions() = default;
+
+    ~CommandOptions() override = default;
+
+    Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
+                          ExecutionContext *execution_context) override {
+      Status error;
+      const int short_option = m_getopt_table[option_idx].val;
+
+      switch (short_option) {
+      case 'd':
+        directory.SetDirectory(option_arg);
+        break;
+      default:
+        llvm_unreachable("Unimplemented option");
+      }
+      return error;
+    }
+
+    void OptionParsingStarting(ExecutionContext *execution_context) override {
+      directory.Clear();
+    }
+
+    llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
+      return llvm::makeArrayRef(g_diagnostics_dump_options);
+    }
+
+    FileSpec directory;
+  };
+
+  Options *GetOptions() override { return &m_options; }
+
+protected:
+  llvm::Expected<FileSpec> GetDirectory() {
+    if (m_options.directory) {
+      auto ec =
+          llvm::sys::fs::create_directories(m_options.directory.GetPath());
+      if (ec)
+        return llvm::errorCodeToError(ec);
+      return m_options.directory;
+    }
+    return Diagnostics::CreateUniqueDirectory();
+  }
+
+  bool DoExecute(Args &args, CommandReturnObject &result) override {
+    llvm::Expected<FileSpec> directory = GetDirectory();
+
+    if (!directory) {
+      result.AppendError(llvm::toString(directory.takeError()));
+      return result.Succeeded();
+    }
+
+    llvm::Error error = Diagnostics::Instance().Create(*directory);
+    if (error) {
+      result.AppendErrorWithFormat("failed to write diagnostics to %s",
+                                   directory->GetPath().c_str());
+      result.AppendError(llvm::toString(std::move(error)));
+      return result.Succeeded();
+    }
+
+    result.GetOutputStream() << "diagnostics written to " << *directory << '\n';
+
+    result.SetStatus(eReturnStatusSuccessFinishResult);
+    return result.Succeeded();
+  }
+
+  CommandOptions m_options;
+};
+
+CommandObjectDiagnostics::CommandObjectDiagnostics(
+    CommandInterpreter &interpreter)
+    : CommandObjectMultiword(interpreter, "diagnostics",
+                             "Commands controlling LLDB diagnostics.",
+                             "diagnostics <subcommand> [<command-options>]") {
+  LoadSubCommand(
+      "dump", CommandObjectSP(new CommandObjectDiagnosticsDump(interpreter)));
+}
+
+CommandObjectDiagnostics::~CommandObjectDiagnostics() = default;

lldb/source/Commands/Options.td

def disassemble_options_line : Option<"line", "l">, Group<6>,
     "there is debug line table information, else disassemble around the pc.">;
   def disassemble_options_address : Option<"address", "a">, Group<7>,
     Arg<"AddressOrExpression">,
     Desc<"Disassemble function containing this address.">;
   def disassemble_options_force : Option<"force", "\\x01">, Groups<[2,3,4,5,7]>,
     Desc<"Force disassembly of large functions.">;
 }
 
+let Command = "diagnostics dump" in {
+  def diagnostics_dump_directory : Option<"directory", "d">, Group<1>,
+    Arg<"Path">, Desc<"Dump the diagnostics to the given directory.">;
+}
+
 let Command = "expression" in {
   def expression_options_all_threads : Option<"all-threads", "a">,
     Groups<[1,2]>, Arg<"Boolean">, Desc<"Should we run all threads if the "
     "execution doesn't complete on one thread.">;
   def expression_options_ignore_breakpoints : Option<"ignore-breakpoints", "i">,
     Groups<[1,2]>, Arg<"Boolean">,
     Desc<"Ignore breakpoint hits while running expressions">;
   def expression_options_timeout : Option<"timeout", "t">, Groups<[1,2]>,

lldb/source/Interpreter/CommandInterpreter.cpp

 #include <limits>
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "Commands/CommandObjectApropos.h"
 #include "Commands/CommandObjectBreakpoint.h"
 #include "Commands/CommandObjectCommands.h"
+#include "Commands/CommandObjectDiagnostics.h"
 #include "Commands/CommandObjectDisassemble.h"
 #include "Commands/CommandObjectExpression.h"
 #include "Commands/CommandObjectFrame.h"
 #include "Commands/CommandObjectGUI.h"
 #include "Commands/CommandObjectHelp.h"
 #include "Commands/CommandObjectLanguage.h"
 #include "Commands/CommandObjectLog.h"
 #include "Commands/CommandObjectMemory.h"
#define REGISTER_COMMAND_OBJECT(NAME, CLASS)                                   \
   m_command_dict[NAME] = std::make_shared<CLASS>(*this);
 
 void CommandInterpreter::LoadCommandDictionary() {
   LLDB_SCOPED_TIMER();
 
   REGISTER_COMMAND_OBJECT("apropos", CommandObjectApropos);
   REGISTER_COMMAND_OBJECT("breakpoint", CommandObjectMultiwordBreakpoint);
   REGISTER_COMMAND_OBJECT("command", CommandObjectMultiwordCommands);
+  REGISTER_COMMAND_OBJECT("diagnostics", CommandObjectDiagnostics);
   REGISTER_COMMAND_OBJECT("disassemble", CommandObjectDisassemble);
   REGISTER_COMMAND_OBJECT("expression", CommandObjectExpression);
   REGISTER_COMMAND_OBJECT("frame", CommandObjectMultiwordFrame);
   REGISTER_COMMAND_OBJECT("gui", CommandObjectGUI);
   REGISTER_COMMAND_OBJECT("help", CommandObjectHelp);
   REGISTER_COMMAND_OBJECT("log", CommandObjectLog);
   REGISTER_COMMAND_OBJECT("memory", CommandObjectMemory);
   REGISTER_COMMAND_OBJECT("platform", CommandObjectPlatform);

lldb/source/Utility/Diagnostics.cpp

 
 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) {
+  Expected<FileSpec> diagnostics_dir = CreateUniqueDirectory();
+  if (!diagnostics_dir) {
     stream << "unable to create diagnostic dir: "
-           << toString(errorCodeToError(ec)) << '\n';
+           << toString(diagnostics_dir.takeError()) << '\n';
     return false;
   }
 
-  stream << "LLDB diagnostics written to " << diagnostics_dir << "\n";
+  return Dump(stream, *diagnostics_dir);
+}
+
+bool Diagnostics::Dump(raw_ostream &stream, const FileSpec &dir) {
+  stream << "LLDB diagnostics will be written to " << dir.GetPath() << "\n";
   stream << "Please include the directory content when filing a bug report\n";
 
-  Error error = Create(FileSpec(diagnostics_dir.str()));
+  Error error = Create(dir);
   if (error) {
     stream << toString(std::move(error)) << '\n';
     return false;
   }
 
   return true;
 }
 
+llvm::Expected<FileSpec> Diagnostics::CreateUniqueDirectory() {
+  SmallString<128> diagnostics_dir;
+  std::error_code ec =
+      sys::fs::createUniqueDirectory("diagnostics", diagnostics_dir);
+  if (ec)
+    return errorCodeToError(ec);
+  return FileSpec(diagnostics_dir.str());
+}
+
 Error Diagnostics::Create(const FileSpec &dir) {
   for (Callback c : m_callbacks) {
     if (Error err = c(dir))
       return err;
   }
   return Error::success();
 }

lldb/test/Shell/Diagnostics/TestDump.test

+# Check that the diagnostics dump command uses the correct directory and
+# creates one if needed.
+
+# Dump to an existing directory.
+# RUN: rm -rf %t.existing
+# RUN: mkdir -p %t.existing
+# RUN: %lldb -o 'diagnostics dump -d %t.existing'
+# RUN: file %t.existing | FileCheck %s
+
+# Dump to a non-existing directory.
+# RUN: rm -rf %t.nonexisting
+# RUN: %lldb -o 'diagnostics dump -d %t.nonexisting'
+# RUN: file %t.nonexisting | FileCheck %s
+
+# CHECK: : directory