Improve error messages for command that need a stopped process

[jasonmolenda]

Summary

I don't think lldb's error messaging is great when attached to a process -- the command interpreter's commands are asynchronous -- and the user doesn't realize that it is currently executing. They resume execution and they can type more commands while the inferior process is executing. This leads to people getting messages like

(lldb) c
Process 82784 resuming
(lldb) si
error: invalid thread
(lldb) dis
error: Cannot disassemble around the current function without a selected frame.

(lldb) reg read pc
error: invalid thread
(lldb) p/x $pc
error: Process is running. Use 'process interrupt' to pause execution.
(lldb)

That last one was good, but the rest are mystifying unless you work on lldb and know how the command requirements are expressed/tested.

This patch changes the error messages, and for the disassembler, adds cases where we have a Process and where we don't (in a way that is messy but all 3 instances print slightly different error messages).

There may be suggestions or other ideas for how these could be phrased. I avoided the term "live process" because I was thinking of corefile debugging, but "stopped process" may make people think "well I killed the process, it's stopped now, why can't I disassemble" lol. I'm trying to think less like an lldb programmer and more like an lldb user, and I think these terms get the point across:

(lldb) c
Process 82784 resuming
(lldb) si
error: Command requires a process which is currently stopped.
(lldb) dis
error: Cannot disassemble around the current function without the process being stopped.
(lldb) reg read pc
error: Command requires a process which is currently stopped.
(lldb)

lldb/test/Shell/Commands/command-disassemble.s will need to be updated with the final messaging too, before this could be landed. But I suspect there may be some suggestions for edits.

[JDevlieghere]

Nit: can we use a colon instead of dash? We have a bunch of places already where we're "concatenating" errors with a colon so I'd like to keep that consistent.

[jasonmolenda]

In D120594#3346722, @JDevlieghere wrote:

Nit: can we use a colon instead of dash? We have a bunch of places already where we're "concatenating" errors with a colon so I'd like to keep that consistent.

Using a dash is definitely the kind of thing I would do, but I don't think I did here? I'm really not wedded to my specific wording in this patch.

[jasonmolenda]

In D120594#3346730, @jasonmolenda wrote:

In D120594#3346722, @JDevlieghere wrote:

Nit: can we use a colon instead of dash? We have a bunch of places already where we're "concatenating" errors with a colon so I'd like to keep that consistent.

Using a dash is definitely the kind of thing I would do, but I don't think I did here? I'm really not wedded to my specific wording in this patch.

Ah sorry, I missed them down in the CommandObjectDisassemler changes.

[jasonmolenda]

Integrate Jonas' suggestion, update the disassemble shell test to match the new error messages.

[JDevlieghere]

LGTM

lldb/include/lldb/Interpreter/CommandObject.h

bool ParseOptionsAndNotify(Args &args, CommandReturnObject &result,
                              OptionGroupOptions &group_options,
                              ExecutionContext &exe_ctx);
 
   virtual const char *GetInvalidTargetDescription() {
     return "invalid target, create a target using the 'target create' command";
   }
 
   virtual const char *GetInvalidProcessDescription() {
-    return "invalid process";
+    return "Command requires a current process.";
   }
 
-  virtual const char *GetInvalidThreadDescription() { return "invalid thread"; }
+  virtual const char *GetInvalidThreadDescription() {
+    return "Command requires a process which is currently stopped.";
+  }
 
-  virtual const char *GetInvalidFrameDescription() { return "invalid frame"; }
+  virtual const char *GetInvalidFrameDescription() {
+    return "Command requires a process, which is currently stopped.";
+  }
 
   virtual const char *GetInvalidRegContextDescription() {
-    return "invalid frame, no registers";
+    return "invalid frame, no registers, command requires a process which is "
+           "currently stopped.";
   }
 
   // This is for use in the command interpreter, when you either want the
   // selected target, or if no target is present you want to prime the dummy
   // target with entities that will be copied over to new targets.
   Target &GetSelectedOrDummyTarget(bool prefer_dummy = false);
   Target &GetSelectedTarget();
   Target &GetDummyTarget();

lldb/source/Commands/CommandObjectDisassemble.cpp

CommandObjectDisassemble::GetContainingAddressRanges() {
 
   if (llvm::Error err = CheckRangeSize(ranges[0], "the function"))
     return std::move(err);
   return ranges;
 }
 
 llvm::Expected<std::vector<AddressRange>>
 CommandObjectDisassemble::GetCurrentFunctionRanges() {
+  Process *process = m_exe_ctx.GetProcessPtr();
   StackFrame *frame = m_exe_ctx.GetFramePtr();
   if (!frame) {
+    if (process) {
+      return llvm::createStringError(
+          llvm::inconvertibleErrorCode(),
+          "Cannot disassemble around the current "
+          "function without the process being stopped.\n");
+    } else {
       return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                      "Cannot disassemble around the current "
-                                   "function without a selected frame.\n");
+                                     "function without a selected frame: "
+                                     "no currently running process.\n");
+    }
   }
   SymbolContext sc(
       frame->GetSymbolContext(eSymbolContextFunction | eSymbolContextSymbol));
   AddressRange range;
   if (sc.function)
     range = sc.function->GetAddressRange();
   else if (sc.symbol && sc.symbol->ValueIsAddress()) {
     range = {sc.symbol->GetAddress(), sc.symbol->GetByteSize()};
   } else
     range = {frame->GetFrameCodeAddress(), default_disasm_byte_size};
 
   if (llvm::Error err = CheckRangeSize(range, "the current function"))
     return std::move(err);
   return std::vector<AddressRange>{range};
 }
 
 llvm::Expected<std::vector<AddressRange>>
 CommandObjectDisassemble::GetCurrentLineRanges() {
+  Process *process = m_exe_ctx.GetProcessPtr();
   StackFrame *frame = m_exe_ctx.GetFramePtr();
   if (!frame) {
+    if (process) {
+      return llvm::createStringError(
+          llvm::inconvertibleErrorCode(),
+          "Cannot disassemble around the current "
+          "function without the process being stopped.\n");
+    } else {
       return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                      "Cannot disassemble around the current "
-                                   "line without a selected frame.\n");
+                                     "line without a selected frame: "
+                                     "no currently running process.\n");
+    }
   }
 
   LineEntry pc_line_entry(
       frame->GetSymbolContext(eSymbolContextLineEntry).line_entry);
   if (pc_line_entry.IsValid())
     return std::vector<AddressRange>{pc_line_entry.range};
 
   // No line entry, so just disassemble around the current pc
CommandObjectDisassemble::GetNameRanges(CommandReturnObject &result) {
   }
   if (range_errs)
     result.AppendWarning(toString(std::move(range_errs)));
   return ranges;
 }
 
 llvm::Expected<std::vector<AddressRange>>
 CommandObjectDisassemble::GetPCRanges() {
+  Process *process = m_exe_ctx.GetProcessPtr();
   StackFrame *frame = m_exe_ctx.GetFramePtr();
   if (!frame) {
+    if (process) {
+      return llvm::createStringError(
+          llvm::inconvertibleErrorCode(),
+          "Cannot disassemble around the current "
+          "function without the process being stopped.\n");
+    } else {
       return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                      "Cannot disassemble around the current "
-                                   "PC without a selected frame.\n");
+                                     "PC without a selected frame: "
+                                     "no currently running process.\n");
+    }
   }
 
   if (m_options.num_instructions == 0) {
     // Disassembling at the PC always disassembles some number of
     // instructions (not the whole function).
     m_options.num_instructions = default_disasm_num_ins;
   }
   return std::vector<AddressRange>{{frame->GetFrameCodeAddress(), 0}};

lldb/test/Shell/Commands/command-disassemble.s

 # REQUIRES: x86
 
 # RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux %s -o %t
 # RUN: %lldb %t -o "settings set interpreter.stop-command-source-on-error false" \
 # RUN:   -o "settings set stop-disassembly-max-size 8000" \
 # RUN:   -s %S/Inputs/command-disassemble.lldbinit -o exit 2>&1 | FileCheck %s
 
 # CHECK:      (lldb) disassemble
-# CHECK-NEXT: error: Cannot disassemble around the current function without a selected frame.
+# CHECK-NEXT: error: Cannot disassemble around the current function without a selected frame: no currently running process.
 # CHECK-NEXT: (lldb) disassemble --line
-# CHECK-NEXT: error: Cannot disassemble around the current line without a selected frame.
+# CHECK-NEXT: error: Cannot disassemble around the current line without a selected frame: no currently running process.
 # CHECK-NEXT: (lldb) disassemble --frame
-# CHECK-NEXT: error: Cannot disassemble around the current function without a selected frame.
+# CHECK-NEXT: error: Cannot disassemble around the current function without a selected frame: no currently running process.
 # CHECK-NEXT: (lldb) disassemble --pc
-# CHECK-NEXT: error: Cannot disassemble around the current PC without a selected frame.
+# CHECK-NEXT: error: Cannot disassemble around the current PC without a selected frame: no currently running process.
 # CHECK-NEXT: (lldb) disassemble --start-address 0x0
 # CHECK-NEXT: command-disassemble.s.tmp`foo:
 # CHECK-NEXT: command-disassemble.s.tmp[0x0] <+0>:   int    $0x10
 # CHECK-NEXT: command-disassemble.s.tmp[0x2] <+2>:   int    $0x11
 # CHECK-NEXT: command-disassemble.s.tmp[0x4] <+4>:   int    $0x12
 # CHECK-NEXT: command-disassemble.s.tmp[0x6] <+6>:   int    $0x13
 # CHECK-NEXT: command-disassemble.s.tmp[0x8] <+8>:   int    $0x14
 # CHECK-NEXT: command-disassemble.s.tmp[0xa] <+10>:  int    $0x15