[LLDB]Provide clearer error message for invalid commands. #111891

oontvoo · 2024-10-10T18:52:38Z

Sometimes users (esp. gdb-longtime users) accidentally use GDB syntax, such as breakpoint foo, and they would get an error message from LLDB saying simply Invalid command "breakpoint foo", which is not very helpful.

This change provides additional suggestions to help correcting the mistake.

Sometimes users (esp. gdb-longtime users) accidentally use GDB syntax, such as `breakpoint foo`, and they would get an error message from LLDB saying simply `Invalid command "breakpoint foo"`, which is not very helpful. This change provides additional suggestions to help correcting the mistake.

llvmbot · 2024-10-10T18:53:19Z

@llvm/pr-subscribers-lldb

Author: Vy Nguyen (oontvoo)

Changes

Sometimes users (esp. gdb-longtime users) accidentally use GDB syntax, such as breakpoint foo, and they would get an error message from LLDB saying simply Invalid command "breakpoint foo", which is not very helpful.

This change provides additional suggestions to help correcting the mistake.

Full diff: https://github.com/llvm/llvm-project/pull/111891.diff

3 Files Affected:

(modified) lldb/include/lldb/Interpreter/CommandObjectMultiword.h (+2)
(modified) lldb/source/Commands/CommandObjectMultiword.cpp (+32-10)
(modified) lldb/test/Shell/Commands/command-breakpoint-col.test (+2)

diff --git a/lldb/include/lldb/Interpreter/CommandObjectMultiword.h b/lldb/include/lldb/Interpreter/CommandObjectMultiword.h
index bceb7f0e51edb6..cee118c3f454b5 100644
--- a/lldb/include/lldb/Interpreter/CommandObjectMultiword.h
+++ b/lldb/include/lldb/Interpreter/CommandObjectMultiword.h
@@ -70,6 +70,8 @@ class CommandObjectMultiword : public CommandObject {
     return m_subcommand_dict;
   }
 
+  std::string GetTopSubcommands(int count);
+
   CommandObject::CommandMap m_subcommand_dict;
   bool m_can_be_removed;
 };
diff --git a/lldb/source/Commands/CommandObjectMultiword.cpp b/lldb/source/Commands/CommandObjectMultiword.cpp
index 4efa5652a71703..f7bb58187895b4 100644
--- a/lldb/source/Commands/CommandObjectMultiword.cpp
+++ b/lldb/source/Commands/CommandObjectMultiword.cpp
@@ -194,28 +194,50 @@ void CommandObjectMultiword::Execute(const char *args_string,
 
   std::string error_msg;
   const size_t num_subcmd_matches = matches.GetSize();
-  if (num_subcmd_matches > 0)
+  if (num_subcmd_matches > 0) {
     error_msg.assign("ambiguous command ");
-  else
-    error_msg.assign("invalid command ");
-
-  error_msg.append("'");
-  error_msg.append(std::string(GetCommandName()));
-  error_msg.append(" ");
-  error_msg.append(std::string(sub_command));
-  error_msg.append("'.");
+    error_msg.append("'");
+    error_msg.append(std::string(GetCommandName()));
+    error_msg.append(" ");
+    error_msg.append(std::string(sub_command));
+    error_msg.append("'.");
 
-  if (num_subcmd_matches > 0) {
     error_msg.append(" Possible completions:");
     for (const std::string &match : matches) {
       error_msg.append("\n\t");
       error_msg.append(match);
     }
+  } else {
+    // Rather than simply complaining about the invalid (sub) command,
+    // try to offer some alternatives.
+    // This is especially useful for cases where the user types something
+    // seamingly trivial, such as `breakpoint foo`.
+    error_msg.assign(
+        llvm::Twine("'" + sub_command + "' is not a valid subcommand of \"" +
+                    GetCommandName() + "\". Valid subcommands are " +
+                    GetTopSubcommands(/*count=*/5) + ". Use \"help " +
+                    GetCommandName() + "\" to find out more.")
+            .str());
   }
   error_msg.append("\n");
   result.AppendRawError(error_msg.c_str());
 }
 
+std::string CommandObjectMultiword::GetTopSubcommands(int count) {
+  if (m_subcommand_dict.empty())
+    return "<NONE>";
+  std::string buffer = "{";
+  CommandMap::iterator pos;
+  for (pos = m_subcommand_dict.begin();
+       pos != m_subcommand_dict.end() && count > 0; ++pos, --count) {
+    buffer.append("'");
+    buffer.append(pos->first);
+    buffer.append("',");
+  }
+  buffer.append("...}");
+  return buffer;
+}
+
 void CommandObjectMultiword::GenerateHelpText(Stream &output_stream) {
   // First time through here, generate the help text for the object and push it
   // to the return result object as well
diff --git a/lldb/test/Shell/Commands/command-breakpoint-col.test b/lldb/test/Shell/Commands/command-breakpoint-col.test
index 65c1e220794303..fecb773d2b4c66 100644
--- a/lldb/test/Shell/Commands/command-breakpoint-col.test
+++ b/lldb/test/Shell/Commands/command-breakpoint-col.test
@@ -3,8 +3,10 @@
 # RUN: %clang_host -g -O0 %S/Inputs/main.c -o %t.out
 # RUN: %lldb -b -o 'help breakpoint set' -o 'breakpoint set -f main.c -l 2 -u 21' %t.out | FileCheck %s --check-prefix HELP --check-prefix CHECK
 # RUN: %lldb -b -o 'help _regexp-break' -o 'b main.c:2:21' %t.out | FileCheck %s --check-prefix HELP-REGEX --check-prefix CHECK
+# RUN: not %lldb -b -o 'breakpoint foo' %t.out -o exit 2>&1 | FileCheck %s --check-prefix ERROR-MSG
 # HELP: -u <column> ( --column <column> )
 # HELP: Specifies the column number on which to set this breakpoint.
 # HELP-REGEX: _regexp-break <filename>:<linenum>:<colnum>
 # HELP-REGEX: main.c:12:21{{.*}}Break at line 12 and column 21 of main.c
 # CHECK: at main.c:2:21
+# ERROR-MSG: 'foo' is not a valid subcommand of "breakpoint". Valid subcommands are {'clear','command','delete','disable','enable',...}. Use "help breakpoint" to find out more.

JDevlieghere · 2024-10-10T22:19:40Z

lldb/source/Commands/CommandObjectMultiword.cpp

+  std::string buffer = "{";
+  CommandMap::iterator pos;
+  for (pos = m_subcommand_dict.begin();
+       pos != m_subcommand_dict.end() && count > 0; ++pos, --count) {
+    buffer.append("'");
+    buffer.append(pos->first);
+    buffer.append("',");
+  }
+  buffer.append("...}");


Can this return something that feels more like natural language? By that I mean dropping the opening and closing braces, adding a space after the comma and maybe even dropping the single quotes.

IIUC, this will also always print ... even if there are say two subcommands and count was 3, which seems needlessly confusing. We should only append this if there are actually more commands that weren't printed.

The programmer in me really wants to make this representation unambiguous, but I think Jonas is right -- the quotes and braces especially would just be noise. Instead of ..., maybe put "and others" or something like that.

jimingham · 2024-10-10T23:33:38Z

lldb/include/lldb/Interpreter/CommandObjectMultiword.h

@@ -70,6 +70,8 @@ class CommandObjectMultiword : public CommandObject {
    return m_subcommand_dict;
  }

+  std::string GetTopSubcommands(int count);


Can we call this GetSubcommandsUpTo(int count) or something like that. "TopSubcommands" sounds like you are returning the most popular ones.

Also, maybe size_t not int since this can't be negative.

This is definitely a topic for another patch, but I actually think it would be really cool if this actually returned the first N most popular commands. :) Then we could even have help breakpoint start off with the most popular subcommand (definitely set) instead of clear (which I don't think I've ever used).

Will rename this to Jim's suggestion.
I like Pavel's idea of having this returning the popular ones, but not sure how we'd determine that here (other than hard-coding the list somewhere)

It would have to be really clear that's what we were doing, however. If you saw this list come out, then were curious and did help breakpoint and the list came out in a different order that would just look like we were being weird.

Yeah, I was basically thinking just some predetermined order (our estimate of a command usage). I don't want to derail this patch with it though, I just found the remark humorous and couldn't resist the temptation.

P.S: Actually, didn't end up renaming it Jim's suggestion, but to something else, hopefully less confusing, which is "GetSubcommandsHint" - whether it returns a top 5 sub-commands or whatever order is transparent to caller.
(Then we can change the content in later patch as needed)

Sure, that's fine too.

labath · 2024-10-11T07:15:41Z

lldb/include/lldb/Interpreter/CommandObjectMultiword.h

@@ -70,6 +70,8 @@ class CommandObjectMultiword : public CommandObject {
    return m_subcommand_dict;
  }

+  std::string GetTopSubcommands(int count);


This is definitely a topic for another patch, but I actually think it would be really cool if this actually returned the first N most popular commands. :) Then we could even have help breakpoint start off with the most popular subcommand (definitely set) instead of clear (which I don't think I've ever used).

labath · 2024-10-11T07:18:52Z

lldb/source/Commands/CommandObjectMultiword.cpp

+  std::string buffer = "{";
+  CommandMap::iterator pos;
+  for (pos = m_subcommand_dict.begin();
+       pos != m_subcommand_dict.end() && count > 0; ++pos, --count) {
+    buffer.append("'");
+    buffer.append(pos->first);
+    buffer.append("',");
+  }
+  buffer.append("...}");


The programmer in me really wants to make this representation unambiguous, but I think Jonas is right -- the quotes and braces especially would just be noise. Instead of ..., maybe put "and others" or something like that.

labath · 2024-10-11T07:25:05Z

lldb/source/Commands/CommandObjectMultiword.cpp

+    // Rather than simply complaining about the invalid (sub) command,
+    // try to offer some alternatives.


I think this is unnecessarily judgemental. Just say what you're doing (offering alternatives) and why (to help the user).

lldb/test/Shell/Commands/command-breakpoint-col.test

…t set

labath · 2024-10-14T08:42:06Z

lldb/source/Commands/CommandObjectMultiword.cpp

+  const size_t maxCount = 5;
+  size_t i = 0;
+  std::string buffer = " Valid subcommand";
+  buffer.append(m_subcommand_dict.size() > 1 ? "s are:" : "is");


You're missing a space before "is". The output will also be garbled for the case of zero subcommands (I think such a thing can happen if the user registers a custom multiword command (SBCommandInterpreter::AddMultiwordCommand), but does not add any subcommands to it).

You're missing a space before "is"

Done.

The output will also be garbled for the case of zero subcommands

Which output? the error msg as a whole or just this function's output?

unless i'm missing something, if there are zero subcommands, the function would have returned "" on line 223, which means the error message would have been something like "foo" is not a valid subcommand of "breakpoint". Use "help breakpoint" to find out more. (ie., there will be no suggestions on subcommands).

You're right. I missed the empty() check at the beginning of the function.

labath · 2024-10-14T08:43:06Z

lldb/test/Shell/Commands/command-wrong-subcommand-error-msg.test

+# RUN: not %lldb -b -o 'breakpoint foo' %t.out -o exit 2>&1 | FileCheck %s --check-prefix BP-MSG
+# RUN: not %lldb -b -o 'watchpoint set foo' %t.out -o exit 2>&1 | FileCheck %s --check-prefix WP-MSG
+# CHECK: at main.c:2:21
+# BP-MSG: 'foo' is not a valid subcommand of "breakpoint". Valid subcommands are: clear, command, delete, disable, enable, and others. Use "help breakpoint" to find out more.


'foo' is not a valid subcommand of "breakpoint"
It would be nice if we could be consistent about the kind of quotes we use at least within a single sentence.

labath · 2024-10-14T08:45:54Z

lldb/include/lldb/Interpreter/CommandObjectMultiword.h

@@ -70,6 +70,8 @@ class CommandObjectMultiword : public CommandObject {
    return m_subcommand_dict;
  }

+  std::string GetTopSubcommands(int count);


Yeah, I was basically thinking just some predetermined order (our estimate of a command usage). I don't want to derail this patch with it though, I just found the remark humorous and couldn't resist the temptation.

…ng space

oontvoo · 2024-10-14T23:39:17Z

Can I merge this? Or would anyone else like to have another look/comments? Thanks!

Sometimes users (esp. gdb-longtime users) accidentally use GDB syntax, such as `breakpoint foo`, and they would get an error message from LLDB saying simply `Invalid command "breakpoint foo"`, which is not very helpful. This change provides additional suggestions to help correcting the mistake.

oontvoo requested a review from labath October 10, 2024 18:52

oontvoo requested a review from JDevlieghere as a code owner October 10, 2024 18:52

llvmbot added the lldb label Oct 10, 2024

JDevlieghere reviewed Oct 10, 2024

View reviewed changes

jimingham reviewed Oct 10, 2024

View reviewed changes

labath reviewed Oct 11, 2024

View reviewed changes

oontvoo force-pushed the invalid_cmd_msg branch from 70ead3e to 4493bf0 Compare October 11, 2024 14:58

oontvoo added 2 commits October 11, 2024 11:30

addressed review comments: reword error msg and rename helper func

8dfb266

move tests to a new file. added test on nested commands, eg watchpoin…

b01e120

…t set

labath approved these changes Oct 14, 2024

View reviewed changes

addressed review comments: fixed quotes to be consistent. added missi…

2f83fa6

…ng space

JDevlieghere approved these changes Oct 15, 2024

View reviewed changes

oontvoo merged commit 4d78881 into llvm:main Oct 15, 2024
7 checks passed

oontvoo deleted the invalid_cmd_msg branch October 15, 2024 14:15

		// Rather than simply complaining about the invalid (sub) command,
		// try to offer some alternatives.

[LLDB]Provide clearer error message for invalid commands. #111891

[LLDB]Provide clearer error message for invalid commands. #111891

Uh oh!

Conversation

oontvoo commented Oct 10, 2024

Uh oh!

llvmbot commented Oct 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oontvoo Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oontvoo commented Oct 14, 2024

Uh oh!

Uh oh!

Uh oh!

oontvoo Oct 14, 2024 •

edited

Loading