[lldb] tab completion for breakpoint names

[MrHate]

Summary

1. created a common completion for breakpoint names;
2. bound the breakpoint name common completion with eArgTypeBreakpointName;
3. implemented the dedicated completion for breakpoint read -N.

[teemperor]

Maybe I'm missing something, but this seems to be two very different independent patches in one (one for br read completion and one for br set which don't depend on each other)? If yes, then please split these up in the future. It's usually less work to review smaller patches (and it's just good practice to do so).

Just some minor points, beside that this LGTM.

@JDevlieghere Can you double check that the structured data use in this patch is fine?

lldb/source/Commands/CommandCompletions.cpp

631

Some small code style adjustments here:

for (const std::string &name : name_list)
  request.TryCompleteCurrentArg(name);

(no curly braces around the single statement body and using a reference because that's how LLVM is usually doing this).

lldb/source/Commands/CommandObjectBreakpoint.cpp

2115

Could you make this just an llvm::Optional<FileSpec> file_spec and then set that in the loop. Then you can drop the success variable.

2163

If you drop success than this can just be if (!bkpt_dict->GetValueForKeyAsArray("Names", names_array))

lldb/test/API/functionalities/completion/TestCompletion.py

654

Do we need the os.getcwd() here? This should work without if I understand correctly (also it avoids the whole issue of spaces in the path breaking this command).

654

There should also be a test for a invalid JSON file and one where the prefix couldn't be found (e.g., try completing "n" to nothing and "m" to "mm").

660

We just landed some improvements to this upstream, so this can now be self.assertSuccess(bp1.AddNameWithErrorHandling("nn")).

[MrHate]

1. replaced std::string with const std::string& in for loop;
2. refactored the success segment with llvm::Optional<FileSpec> file_spec;
3. added invalidation check in test.

[JDevlieghere]

lldb/include/lldb/Interpreter/CommandCompletions.h

106

Please clang-format the patch.

lldb/source/Commands/CommandObjectBreakpoint.cpp

2100–2172

While you're here you might as well remove this useless comment :-)

2116

Please convert these to StringRef and use its comparison operator.

2126

There's nothing wrong with using getValue() but the consensus across LLVM and LLDB is to use *file_spec.

lldb/test/API/functionalities/completion/breakpoints.json

35

As pointed out by phab.

[JDevlieghere]

lldb/source/Commands/CommandObjectBreakpoint.cpp

2133

The structured data looks good to me. I'm not convinced this is where that logic should live, but given the current implementation where all that stuff is implemented in the corresponding data structures, I think that's fine.

[MrHate]

1. clang-formatted related files;
2. refactored line 2100 with StringRef;
3. replaced files_pec.hasValue() to file_spec, file_spec.getValue() to *file_spec;
4. removed an useless comment;
5. added a new line at the end of breakpoints.json.

[JDevlieghere]

LGTM

lldb/include/lldb/Interpreter/CommandCompletions.h

enum CommonCompletionTypes {
     eProcessPluginCompletion = (1u << 11),
     eDisassemblyFlavorCompletion = (1u << 12),
     eTypeLanguageCompletion = (1u << 13),
     eFrameIndexCompletion = (1u << 14),
     eModuleUUIDCompletion = (1u << 15),
     eStopHookIDCompletion = (1u << 16),
     eThreadIndexCompletion = (1u << 17),
     eWatchPointIDCompletion = (1u << 18),
+    eBreakpointNameCompletion = (1u << 19),
     // This item serves two purposes.  It is the last element in the enum, so
     // you can add custom enums starting from here in your Option class. Also
     // if you & in this bit the base code will not process the option.
-    eCustomCompletion = (1u << 19)
+    eCustomCompletion = (1u << 20)
   };
 
   static bool InvokeCommonCompletionCallbacks(
       CommandInterpreter &interpreter, uint32_t completion_mask,
       lldb_private::CompletionRequest &request, SearchFilter *searcher);
 
   // These are the generic completer functions:
   static void DiskFiles(CommandInterpreter &interpreter,
static void VariablePath(CommandInterpreter &interpreter,
                            CompletionRequest &request, SearchFilter *searcher);
 
   static void Registers(CommandInterpreter &interpreter,
                         CompletionRequest &request, SearchFilter *searcher);
 
   static void Breakpoints(CommandInterpreter &interpreter,
                           CompletionRequest &request, SearchFilter *searcher);
 
+  static void BreakpointNames(CommandInterpreter &interpreter,
+                              CompletionRequest &request,
+                              SearchFilter *searcher);
+
   static void ProcessPluginNames(CommandInterpreter &interpreter,
                                  CompletionRequest &request,
                                  SearchFilter *searcher);
 
   static void DisassemblyFlavors(CommandInterpreter &interpreter,
                                  CompletionRequest &request,
                                  SearchFilter *searcher);
 

lldb/source/Commands/CommandCompletions.cpp

const CommonCompletionElement common_completions[] = {
       {eBreakpointCompletion, CommandCompletions::Breakpoints},
       {eProcessPluginCompletion, CommandCompletions::ProcessPluginNames},
       {eDisassemblyFlavorCompletion, CommandCompletions::DisassemblyFlavors},
       {eTypeLanguageCompletion, CommandCompletions::TypeLanguages},
       {eFrameIndexCompletion, CommandCompletions::FrameIndexes},
       {eStopHookIDCompletion, CommandCompletions::StopHookIDs},
       {eThreadIndexCompletion, CommandCompletions::ThreadIndexes},
       {eWatchPointIDCompletion, CommandCompletions::WatchPointIDs},
+      {eBreakpointNameCompletion, CommandCompletions::BreakpointNames},
       {eNoCompletion, nullptr} // This one has to be last in the list.
   };
 
   for (int i = 0;; i++) {
     if (common_completions[i].type == eNoCompletion)
       break;
     else if ((common_completions[i].type & completion_mask) ==
                  common_completions[i].type &&
for (size_t i = 0; i < num_breakpoints; ++i) {
     const size_t colon_pos = bp_info.find_first_of(':');
     if (colon_pos != llvm::StringRef::npos)
       bp_info = bp_info.drop_front(colon_pos + 2);
 
     request.TryCompleteCurrentArg(std::to_string(bp->GetID()), bp_info);
   }
 }
 
+void CommandCompletions::BreakpointNames(CommandInterpreter &interpreter,
+                                         CompletionRequest &request,
+                                         SearchFilter *searcher) {
+  lldb::TargetSP target = interpreter.GetDebugger().GetSelectedTarget();
+  if (!target)
+    return;
+
+  std::vector<std::string> name_list;
+  target->GetBreakpointNames(name_list);
+
+  for (const std::string &name : name_list)
+    request.TryCompleteCurrentArg(name);
+}
+
 void CommandCompletions::ProcessPluginNames(CommandInterpreter &interpreter,
                                             CompletionRequest &request,
                                             SearchFilter *searcher) {
   PluginManager::AutoCompleteProcessName(request.GetCursorArgumentPrefix(),
                                          request);
 }
-
 void CommandCompletions::DisassemblyFlavors(CommandInterpreter &interpreter,
                                             CompletionRequest &request,
                                             SearchFilter *searcher) {
   // Currently the only valid options for disassemble -F are default, and for
   // Intel architectures, att and intel.
   static const char *flavors[] = {"default", "att", "intel"};
   for (const char *flavor : flavors) {
     request.TryCompleteCurrentArg(flavor);

lldb/source/Commands/CommandObjectBreakpoint.cpp

void OptionParsingStarting(ExecutionContext *execution_context) override {
       m_filename.clear();
       m_names.clear();
     }
 
     llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
       return llvm::makeArrayRef(g_breakpoint_read_options);
     }
 
-    // Instance variables to hold the values for command options.
+    void HandleOptionArgumentCompletion(
+        CompletionRequest &request, OptionElementVector &opt_element_vector,
+        int opt_element_index, CommandInterpreter &interpreter) override {
+      int opt_arg_pos = opt_element_vector[opt_element_index].opt_arg_pos;
+      int opt_defs_index = opt_element_vector[opt_element_index].opt_defs_index;
+
+      switch (GetDefinitions()[opt_defs_index].short_option) {
+      case 'f':
+        CommandCompletions::InvokeCommonCompletionCallbacks(
+            interpreter, CommandCompletions::eDiskFileCompletion, request,
+            nullptr);
+        break;
+
+      case 'N':
+        llvm::Optional<FileSpec> file_spec;
+        const llvm::StringRef dash_f("-f");
+        for (int arg_idx = 0; arg_idx < opt_arg_pos; arg_idx++) {
+          if (dash_f == request.GetParsedLine().GetArgumentAtIndex(arg_idx)) {
+            file_spec.emplace(
+                request.GetParsedLine().GetArgumentAtIndex(arg_idx + 1));
+            break;
+          }
+        }
+        if (!file_spec)
+          return;
+
+        FileSystem::Instance().Resolve(*file_spec);
+        Status error;
+        StructuredData::ObjectSP input_data_sp =
+            StructuredData::ParseJSONFromFile(*file_spec, error);
+        if (!error.Success())
+          return;
+
+        StructuredData::Array *bkpt_array = input_data_sp->GetAsArray();
+        if (!bkpt_array)
+          return;
+
+        const size_t num_bkpts = bkpt_array->GetSize();
+        for (size_t i = 0; i < num_bkpts; i++) {
+          StructuredData::ObjectSP bkpt_object_sp =
+              bkpt_array->GetItemAtIndex(i);
+          if (!bkpt_object_sp)
+            return;
+
+          StructuredData::Dictionary *bkpt_dict =
+              bkpt_object_sp->GetAsDictionary();
+          if (!bkpt_dict)
+            return;
+
+          StructuredData::ObjectSP bkpt_data_sp =
+              bkpt_dict->GetValueForKey(Breakpoint::GetSerializationKey());
+          if (!bkpt_data_sp)
+            return;
+
+          bkpt_dict = bkpt_data_sp->GetAsDictionary();
+          if (!bkpt_dict)
+            return;
+
+          StructuredData::Array *names_array;
+
+          if (!bkpt_dict->GetValueForKeyAsArray("Names", names_array))
+            return;
+
+          size_t num_names = names_array->GetSize();
+
+          for (size_t i = 0; i < num_names; i++) {
+            llvm::StringRef name;
+            if (names_array->GetItemAtIndexAsString(i, name))
+              request.TryCompleteCurrentArg(name);
+          }
+        }
+      }
+    }
 
     std::string m_filename;
     std::vector<std::string> m_names;
   };
 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     Target &target = GetSelectedOrDummyTarget();

lldb/source/Interpreter/CommandObject.cpp

CommandObject::ArgumentTableEntry CommandObject::g_arguments_data[] = {
     { eArgTypeAddress, "address", CommandCompletions::eNoCompletion, { nullptr, false }, "A valid address in the target program's execution space." },
     { eArgTypeAddressOrExpression, "address-expression", CommandCompletions::eNoCompletion, { nullptr, false }, "An expression that resolves to an address." },
     { eArgTypeAliasName, "alias-name", CommandCompletions::eNoCompletion, { nullptr, false }, "The name of an abbreviation (alias) for a debugger command." },
     { eArgTypeAliasOptions, "options-for-aliased-command", CommandCompletions::eNoCompletion, { nullptr, false }, "Command options to be used as part of an alias (abbreviation) definition.  (See 'help commands alias' for more information.)" },
     { eArgTypeArchitecture, "arch", CommandCompletions::eArchitectureCompletion, { arch_helper, true }, "The architecture name, e.g. i386 or x86_64." },
     { eArgTypeBoolean, "boolean", CommandCompletions::eNoCompletion, { nullptr, false }, "A Boolean value: 'true' or 'false'" },
     { eArgTypeBreakpointID, "breakpt-id", CommandCompletions::eNoCompletion, { BreakpointIDHelpTextCallback, false }, nullptr },
     { eArgTypeBreakpointIDRange, "breakpt-id-list", CommandCompletions::eNoCompletion, { BreakpointIDRangeHelpTextCallback, false }, nullptr },
-    { eArgTypeBreakpointName, "breakpoint-name", CommandCompletions::eNoCompletion, { BreakpointNameHelpTextCallback, false }, nullptr },
+    { eArgTypeBreakpointName, "breakpoint-name", CommandCompletions::eBreakpointNameCompletion, { BreakpointNameHelpTextCallback, false }, nullptr },
     { eArgTypeByteSize, "byte-size", CommandCompletions::eNoCompletion, { nullptr, false }, "Number of bytes to use." },
     { eArgTypeClassName, "class-name", CommandCompletions::eNoCompletion, { nullptr, false }, "Then name of a class from the debug information in the program." },
     { eArgTypeCommandName, "cmd-name", CommandCompletions::eNoCompletion, { nullptr, false }, "A debugger command (may be multiple words), without any options or arguments." },
     { eArgTypeCount, "count", CommandCompletions::eNoCompletion, { nullptr, false }, "An unsigned integer." },
     { eArgTypeDirectoryName, "directory", CommandCompletions::eDiskDirectoryCompletion, { nullptr, false }, "A directory name." },
     { eArgTypeDisassemblyFlavor, "disassembly-flavor", CommandCompletions::eDisassemblyFlavorCompletion, { nullptr, false }, "A disassembly flavor recognized by your disassembly plugin.  Currently the only valid options are \"att\" and \"intel\" for Intel targets" },
     { eArgTypeDescriptionVerbosity, "description-verbosity", CommandCompletions::eNoCompletion, { nullptr, false }, "How verbose the output of 'po' should be." },
     { eArgTypeEndAddress, "end-address", CommandCompletions::eNoCompletion, { nullptr, false }, "Help text goes here." },

lldb/test/API/functionalities/completion/TestCompletion.py

def test_complete_breakpoint_with_ids(self):
                                   ['1',
                                    '2'])
         
         for subcommand in subcommands:
             self.complete_from_to('breakpoint ' + subcommand + ' 1 ',
                                   ['1',
                                    '2'])
 
+    def test_complete_breakpoint_with_names(self):
+        self.build()
+        target = self.dbg.CreateTarget(self.getBuildArtifact('a.out'))
+        self.assertTrue(target, VALID_TARGET)
+
+        # test breakpoint read dedicated
+        self.complete_from_to('breakpoint read -N ', 'breakpoint read -N ')
+        self.complete_from_to('breakpoint read -f breakpoints.json -N ', ['mm'])
+        self.complete_from_to('breakpoint read -f breakpoints.json -N n', 'breakpoint read -f breakpoints.json -N n')
+        self.complete_from_to('breakpoint read -f breakpoints_invalid.json -N ', 'breakpoint read -f breakpoints_invalid.json -N ')
+
+        # test common breapoint name completion
+        bp1 = target.BreakpointCreateByName('main', 'a.out')
+        self.assertTrue(bp1)
+        self.assertEqual(bp1.GetNumLocations(), 1)
+        self.complete_from_to('breakpoint set -N n', 'breakpoint set -N n')
+        self.assertTrue(bp1.AddNameWithErrorHandling("nn"))
+        self.complete_from_to('breakpoint set -N ', 'breakpoint set -N nn')

lldb/test/API/functionalities/completion/breakpoints.json

+[
+    {
+        "Breakpoint": {
+            "BKPTOptions": {
+                "AutoContinue": false,
+                "ConditionText": "",
+                "EnabledState": true,
+                "IgnoreCount": 0,
+                "OneShotState": false
+            },
+            "BKPTResolver": {
+                "Options": {
+                    "NameMask": [
+                        56
+                    ],
+                    "Offset": 0,
+                    "SkipPrologue": true,
+                    "SymbolNames": [
+                        "main"
+                    ]
+                },
+                "Type": "SymbolName"
+            },
+            "Hardware": false,
+            "Names": [
+                "mm"
+            ],
+            "SearchFilter": {
+                "Options": {},
+                "Type": "Unconstrained"
+            }
+        }
+    }
+]

lldb/test/API/functionalities/completion/breakpoints_invalid.json

+[
+    {
+        "Breakpoint": {
+        }
+    }
+]