Remove `bugreport` command

[JDevlieghere]

Summary

The bugreport command exists to create domain-specific bug reports. Currently it has one implementation for filing bugs on the unwinder. As far as we can tell, it has never been of use. Although not exactly the same as the reproducers, it's a bit confusing to have two parallel command trees for (kind of) the same thing.

[davide]

I wanted to remove it a while ago, but Jason told me he found this useful, so I would wait for his opinion.

[davide]

Other than that, I have no objections.

[clayborg]

IMHO: we should keep this command and expand its abilities to help report stepping issues, expression issues and more. Why? Getting good bug reports from users is quite hard. Allowing them to type "bugreport step --step-in" or "bugreport step --step-over" would be really nice. Right now I send people a python script that enables logging, dumps the code in and around the source line with disassembly, and creates a zip file with the current object file and optional debug info file (dSYM or external symbol file).

Happy to hear what others have to say though.

[JDevlieghere]

In D65469#1608278, @clayborg wrote:

IMHO: we should keep this command and expand its abilities to help report stepping issues, expression issues and more. Why? Getting good bug reports from users is quite hard. Allowing them to type "bugreport step --step-in" or "bugreport step --step-over" would be really nice. Right now I send people a python script that enables logging, dumps the code in and around the source line with disassembly, and creates a zip file with the current object file and optional debug info file (dSYM or external symbol file).

What's the benefit of having this instead of having the user generate a reproducer?

[clayborg]

In D65469#1608631, @JDevlieghere wrote:

In D65469#1608278, @clayborg wrote:

IMHO: we should keep this command and expand its abilities to help report stepping issues, expression issues and more. Why? Getting good bug reports from users is quite hard. Allowing them to type "bugreport step --step-in" or "bugreport step --step-over" would be really nice. Right now I send people a python script that enables logging, dumps the code in and around the source line with disassembly, and creates a zip file with the current object file and optional debug info file (dSYM or external symbol file).

What's the benefit of having this instead of having the user generate a reproducer?

I am guessing the reproducer wouldn't help us with any of the user files. We often need to see the binaries, or enable some logging when the user repros the bug. Users don't often attach all of the files that are needed, nor do they know what logs to enable and how to attach them. I am not up to date on exactly what reproducers do, but I would guess that they don't save off just the required files or enable any logging and attach logs?

[JDevlieghere]

In D65469#1608653, @clayborg wrote:

In D65469#1608631, @JDevlieghere wrote:

In D65469#1608278, @clayborg wrote:

IMHO: we should keep this command and expand its abilities to help report stepping issues, expression issues and more. Why? Getting good bug reports from users is quite hard. Allowing them to type "bugreport step --step-in" or "bugreport step --step-over" would be really nice. Right now I send people a python script that enables logging, dumps the code in and around the source line with disassembly, and creates a zip file with the current object file and optional debug info file (dSYM or external symbol file).

What's the benefit of having this instead of having the user generate a reproducer?

I am guessing the reproducer wouldn't help us with any of the user files. We often need to see the binaries, or enable some logging when the user repros the bug. Users don't often attach all of the files that are needed, nor do they know what logs to enable and how to attach them. I am not up to date on exactly what reproducers do, but I would guess that they don't save off just the required files or enable any logging and attach logs?

That's exactly what the reproducers do actually. They capture user interaction, the GDB remote packets and any files used by the debugger. They currently don't enable logs, because you can debug LLDB during reproducer replay, but that would be trivial to include as well.

[jingham]

The thing the "bugreport unwind" adds is that it runs a handful of commands that gather data useful in diagnosing unwind problems. That's useful when the information you need might not be output by the session in which the bug appeared.

The same thing could be achieved by having "reproducer generate" take arguments that the user can specify to automate gathering more data that would be relevant to a particular kind of bug. So "bugreport unwind" -> "reproducer generate unwind" which would run the same extra commands that "bugreport unwind" does (the results will get captured by the same mechanism that's catching all the other session output) and then the reproducer would be generated.

[jingham]

If we don't want to forget that there was specific useful info, we could gate this patch on implementing the "reproducer generate <FEATURE>" feature. That should be easy to add if we're clear this this is the right design for reproducers. But I don't think that "bugreport unwind" was all that much used, so it might be better to design this at leisure.

[JDevlieghere]

In D65469#1608737, @jingham wrote:

If we don't want to forget that there was specific useful info, we could gate this patch on implementing the "reproducer generate <FEATURE>" feature. That should be easy to add if we're clear this this is the right design for reproducers. But I don't think that "bugreport unwind" was all that much used, so it might be better to design this at leisure.

Until the reproducers prove to be insufficient to capture a particular bug, I don't plan to invest in this.

[teemperor]

What's the state of this? Because thi is in my review queue and it has no associated test, so either this moves forward and we remove bugreport or we keep bugreport and I'll put it on my list of things that need tests :)

[jasonmolenda]

We don't need to keep this command around. It was originated by Tamas in 2015,
https://reviews.llvm.org/D10868
but never got used for anything - Tamas was working on unwind things back then iirc, and may it looks like he thought it would be useful for that. On Darwin systems we do ship with a python command for diagnosing unwind problems ("script import lldb.diagnose"; "diagnose-unwind") which can be used when the current backtrace looks like it may be missing stack frames, or truncate early. The unwinder hasn't seen a lot of active development for years, so we don't have cause to use this very often.

I think removing this is fine. The reproducers will be a great way to include everything needed to repro a bug that a user sees. Not everyone can produce a bug report with all of their binaries etc, but the bugreport command suffers from the obvious problem that it can only gather state after something bad has happened. (as Jim notes in the original phab)

lldb/packages/Python/lldbsuite/test/functionalities/completion/TestCompletion.py

def test_command_argument_completion(self):
         self.complete_from_to("watchpoint set variable foo --watch ", ["read", "write", "read_write"])
         self.complete_from_to("watchpoint set variable foo --watch w", "watchpoint set variable foo --watch write")
         self.complete_from_to('watchpoint set variable foo -w read_', 'watchpoint set variable foo -w read_write')
 
     def test_completion_description_commands(self):
         """Test descriptions of top-level command completions"""
         self.check_completion_with_desc("", [
             ["command", "Commands for managing custom LLDB commands."],
-            ["bugreport", "Commands for creating domain-specific bug reports."]
+            ["breakpoint", "Commands for operating on breakpoints (see 'help b' for shorthand.)"]
         ])
 
         self.check_completion_with_desc("pl", [
             ["platform", "Commands to manage and create platforms."],
             ["plugin", "Commands for managing LLDB plugins."]
         ])
 
         # Just check that this doesn't crash.

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
-  CommandObjectBugreport.cpp
   CommandObjectCommands.cpp
   CommandObjectDisassemble.cpp
   CommandObjectExpression.cpp
   CommandObjectFrame.cpp
   CommandObjectGUI.cpp
   CommandObjectHelp.cpp
   CommandObjectLog.cpp
   CommandObjectMemory.cpp

lldb/source/Commands/CommandObjectBugreport.h

-//===-- CommandObjectBugreport.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 liblldb_CommandObjectBugreport_h_
-#define liblldb_CommandObjectBugreport_h_
-
-#include "lldb/Interpreter/CommandObjectMultiword.h"
-
-namespace lldb_private {
-
-// CommandObjectMultiwordBugreport
-
-class CommandObjectMultiwordBugreport : public CommandObjectMultiword {
-public:
-  CommandObjectMultiwordBugreport(CommandInterpreter &interpreter);
-
-  ~CommandObjectMultiwordBugreport() override;
-};
-
-} // namespace lldb_private
-
-#endif // liblldb_CommandObjectBugreport_h_

lldb/source/Commands/CommandObjectBugreport.cpp

-//===-- CommandObjectBugreport.cpp ------------------------------*- 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
-//
-//===----------------------------------------------------------------------===//
-
-#include "CommandObjectBugreport.h"
-
-#include <cstdio>
-
-
-#include "lldb/Interpreter/CommandInterpreter.h"
-#include "lldb/Interpreter/CommandReturnObject.h"
-#include "lldb/Interpreter/OptionGroupOutputFile.h"
-#include "lldb/Target/Thread.h"
-
-using namespace lldb;
-using namespace lldb_private;
-
-// "bugreport unwind"
-
-class CommandObjectBugreportUnwind : public CommandObjectParsed {
-public:
-  CommandObjectBugreportUnwind(CommandInterpreter &interpreter)
-      : CommandObjectParsed(
-            interpreter, "bugreport unwind",
-            "Create a bugreport for a bug in the stack unwinding code.",
-            nullptr),
-        m_option_group(), m_outfile_options() {
-    m_option_group.Append(&m_outfile_options, LLDB_OPT_SET_ALL,
-                          LLDB_OPT_SET_1 | LLDB_OPT_SET_2 | LLDB_OPT_SET_3);
-    m_option_group.Finalize();
-  }
-
-  ~CommandObjectBugreportUnwind() override {}
-
-  Options *GetOptions() override { return &m_option_group; }
-
-protected:
-  bool DoExecute(Args &command, CommandReturnObject &result) override {
-    StringList commands;
-    commands.AppendString("thread backtrace");
-
-    Thread *thread = m_exe_ctx.GetThreadPtr();
-    if (thread) {
-      char command_buffer[256];
-
-      uint32_t frame_count = thread->GetStackFrameCount();
-      for (uint32_t i = 0; i < frame_count; ++i) {
-        StackFrameSP frame = thread->GetStackFrameAtIndex(i);
-        lldb::addr_t pc = frame->GetStackID().GetPC();
-
-        snprintf(command_buffer, sizeof(command_buffer),
-                 "disassemble --bytes --address 0x%" PRIx64, pc);
-        commands.AppendString(command_buffer);
-
-        snprintf(command_buffer, sizeof(command_buffer),
-                 "image show-unwind --address 0x%" PRIx64, pc);
-        commands.AppendString(command_buffer);
-      }
-    }
-
-    const FileSpec &outfile_spec =
-        m_outfile_options.GetFile().GetCurrentValue();
-    if (outfile_spec) {
-
-      uint32_t open_options =
-          File::eOpenOptionWrite | File::eOpenOptionCanCreate |
-          File::eOpenOptionAppend | File::eOpenOptionCloseOnExec;
-
-      const bool append = m_outfile_options.GetAppend().GetCurrentValue();
-      if (!append)
-        open_options |= File::eOpenOptionTruncate;
-
-      StreamFileSP outfile_stream = std::make_shared<StreamFile>();
-      File &file = outfile_stream->GetFile();
-      Status error =
-          FileSystem::Instance().Open(file, outfile_spec, open_options);
-      if (error.Fail()) {
-        auto path = outfile_spec.GetPath();
-        result.AppendErrorWithFormat("Failed to open file '%s' for %s: %s\n",
-                                     path.c_str(), append ? "append" : "write",
-                                     error.AsCString());
-        result.SetStatus(eReturnStatusFailed);
-        return false;
-      }
-
-      result.SetImmediateOutputStream(outfile_stream);
-    }
-
-    CommandInterpreterRunOptions options;
-    options.SetStopOnError(false);
-    options.SetEchoCommands(true);
-    options.SetPrintResults(true);
-    options.SetPrintErrors(true);
-    options.SetAddToHistory(false);
-    m_interpreter.HandleCommands(commands, &m_exe_ctx, options, result);
-
-    return result.Succeeded();
-  }
-
-private:
-  OptionGroupOptions m_option_group;
-  OptionGroupOutputFile m_outfile_options;
-};
-
-#pragma mark CommandObjectMultiwordBugreport
-
-// CommandObjectMultiwordBugreport
-
-CommandObjectMultiwordBugreport::CommandObjectMultiwordBugreport(
-    CommandInterpreter &interpreter)
-    : CommandObjectMultiword(
-          interpreter, "bugreport",
-          "Commands for creating domain-specific bug reports.",
-          "bugreport <subcommand> [<subcommand-options>]") {
-
-  LoadSubCommand(
-      "unwind", CommandObjectSP(new CommandObjectBugreportUnwind(interpreter)));
-}
-
-CommandObjectMultiwordBugreport::~CommandObjectMultiwordBugreport() {}

lldb/source/Interpreter/CommandInterpreter.cpp

 #include <string>
 #include <vector>
 
 #include "CommandObjectScript.h"
 #include "lldb/Interpreter/CommandObjectRegexCommand.h"
 
 #include "Commands/CommandObjectApropos.h"
 #include "Commands/CommandObjectBreakpoint.h"
-#include "Commands/CommandObjectBugreport.h"
 #include "Commands/CommandObjectCommands.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"
void CommandInterpreter::LoadCommandDictionary() {
   static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
   Timer scoped_timer(func_cat, LLVM_PRETTY_FUNCTION);
 
   lldb::ScriptLanguage script_language = m_debugger.GetScriptLanguage();
 
   m_command_dict["apropos"] = CommandObjectSP(new CommandObjectApropos(*this));
   m_command_dict["breakpoint"] =
       CommandObjectSP(new CommandObjectMultiwordBreakpoint(*this));
-  m_command_dict["bugreport"] =
-      CommandObjectSP(new CommandObjectMultiwordBugreport(*this));
   m_command_dict["command"] =
       CommandObjectSP(new CommandObjectMultiwordCommands(*this));
   m_command_dict["disassemble"] =
       CommandObjectSP(new CommandObjectDisassemble(*this));
   m_command_dict["expression"] =
       CommandObjectSP(new CommandObjectExpression(*this));
   m_command_dict["frame"] =
       CommandObjectSP(new CommandObjectMultiwordFrame(*this));