[lldb] Omit --show-globals in help target var

[felipepiovezan]

This option doesn't exist. It is currently displayed by help target var due to a bug introduced by 41ae8e7 in 2018.

Some code for target var and frame var is shared, and some hard-code constants are used in order to filter out options that belong only to frame var. However, the aforementioned commit failed to update these constants properly. This patch addresses the issue by having a single place where the filtering of options needs to be done.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

This option doesn't exist. It is currently displayed by help target var due to a bug introduced by 41ae8e7 in 2018.

Some code for target var and frame var is shared, and some hard-code constants are used in order to filter out options that belong only to frame var. However, the aforementioned commit failed to update these constants properly. This patch addresses the issue by having a single place where the filtering of options needs to be done.

---

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

2 Files Affected:

• (modified) lldb/source/Interpreter/OptionGroupVariable.cpp (+12-14)
• (modified) lldb/test/API/functionalities/target_var/TestTargetVar.py (+10)

diff --git a/lldb/source/Interpreter/OptionGroupVariable.cpp b/lldb/source/Interpreter/OptionGroupVariable.cpp
index 0e35a641361b82..99644b3f423c81 100644
--- a/lldb/source/Interpreter/OptionGroupVariable.cpp
+++ b/lldb/source/Interpreter/OptionGroupVariable.cpp
@@ -50,6 +50,11 @@ static constexpr OptionDefinition g_variable_options[] = {
      "Specify a summary string to use to format the variable output."},
 };
 
+static constexpr auto g_num_frame_options = 4;
+static const auto g_variable_options_noframe =
+    llvm::ArrayRef<OptionDefinition>(g_variable_options)
+        .drop_front(g_num_frame_options);
+
 static Status ValidateNamedSummary(const char *str, void *) {
   if (!str || !str[0])
     return Status("must specify a valid named summary");
@@ -77,9 +82,9 @@ OptionGroupVariable::SetOptionValue(uint32_t option_idx,
                                     llvm::StringRef option_arg,
                                     ExecutionContext *execution_context) {
   Status error;
-  if (!include_frame_options)
-    option_idx += 3;
-  const int short_option = g_variable_options[option_idx].short_option;
+  llvm::ArrayRef<OptionDefinition> variable_options =
+      include_frame_options ? g_variable_options : g_variable_options_noframe;
+  const int short_option = variable_options[option_idx].short_option;
   switch (short_option) {
   case 'r':
     use_regex = true;
@@ -128,16 +133,9 @@ void OptionGroupVariable::OptionParsingStarting(
   summary_string.Clear();
 }
 
-#define NUM_FRAME_OPTS 3
-
 llvm::ArrayRef<OptionDefinition> OptionGroupVariable::GetDefinitions() {
-  auto result = llvm::ArrayRef(g_variable_options);
-  // Show the "--no-args", "--no-locals" and "--show-globals" options if we are
-  // showing frame specific options
-  if (include_frame_options)
-    return result;
-
-  // Skip the "--no-args", "--no-locals" and "--show-globals" options if we are
-  // not showing frame specific options (globals only)
-  return result.drop_front(NUM_FRAME_OPTS);
+  // Show the "--no-args", "--no-recognized-args", "--no-locals" and
+  // "--show-globals" options if we are showing frame specific options
+  return include_frame_options ? g_variable_options
+                               : g_variable_options_noframe;
 }
diff --git a/lldb/test/API/functionalities/target_var/TestTargetVar.py b/lldb/test/API/functionalities/target_var/TestTargetVar.py
index a0f3663f036510..54b7b77b6773ce 100644
--- a/lldb/test/API/functionalities/target_var/TestTargetVar.py
+++ b/lldb/test/API/functionalities/target_var/TestTargetVar.py
@@ -15,6 +15,16 @@ class targetCommandTestCase(TestBase):
     def testTargetVarExpr(self):
         self.build()
         lldbutil.run_to_name_breakpoint(self, "main")
+        self.expect(
+            "help target variable",
+            substrs=[
+                "--no-args",
+                "--no-recognized-args",
+                "--no-locals",
+                "--show-globals",
+            ],
+            matching=False,
+        )
         self.expect("target variable i", substrs=["i", "42"])
         self.expect(
             "target variable var", patterns=["\(incomplete \*\) var = 0[xX](0)*dead"]

[bulbazord]

Big fan of centralizing the option parsing here. I had one question about automating another portion of this, but otherwise LGTM.

[bulbazord]

Is this number computable? Or does it need to be hand-maintained?

[felipepiovezan]

Sadly it seems we need to hand-maintain it. The basic thing we need to maintain is "here is an array of options for target var, and here is an array of options for frame var", and one array is a suffix of the other. And they have to be arrays, because of the indexing that is done on them.
Given all of that, I don't think there is a clean way to avoid having to maintain this constant, but the hope is that with the new approach it becomes harder to make the mistake that introduced this bug.