[lldb] common completion for process pids and process names

[MrHate]

Summary

1. Added two common completions: ProcessIDs and ProcessNames, which are refactored from their original dedicated option completions;
2. Removed the dedicated option completion functions of process attach and platform process attach, so that they can use arg-type-bound common completions instead;
3. Bound eArgTypePid to the pid completion, eArgTypeProcessName to the process name completion in CommandObject.cpp;
4. Added a related test case.

[MrHate]

Wrap pid completion as a common completion.

[JDevlieghere]

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

101

Why is this necessary?

105

If self.complete_from_to fails and raises and exception, the process potentially won't terminate. Can you wrap this in a try-except-finally? You should be able to just re-raise the exception in the except clause.

[MrHate]

1. For the necessity it's because lldb does filter out its own pid and the pid of the process it already attached to, so I create a process here to provide an available pid which is known to us and able to be found by the completion implementation.
2. For the child process termination problem, with the process flag value daemon set to True, this child process should be terminated automatically if this test process is killed due to any exception.

If I did misunderstand your meaning or my solution is not considerate, please be in more detail, thx!

[teemperor]

So just to give some additional explanation for the Process thing: LLDB doesn't provide the pid for itself when listing process to attach to (which would just break LLDB if the user actually attaches) and there isn't really any other pid we know to exist on the target system (we could speculate 1 is always running, but that seems a bit sketchy). And a program which we already stopped is apparently also not in the list (we can't double attach). So this test has to launch some process that we know is waiting for us and that hasn't an attached LLDB. @MrHate I think this should be documented in the test.

Beside that this seems alright to me beside some minor nits.

lldb/source/Commands/CommandCompletions.cpp

627

You can do a C++11 for loop over the process_infos (it's just a std::vector).

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

99

Could this just be some kind of sleep or something else that doesn't actively use CPU?

[MrHate]

1. modified for loop;
2. replaced pass with time.sleep(1) so that the test process won't take up CPU.

[teemperor]

LGTM, but I have some minor things. I would do them for you when merging but I just want to make sure you see them so I'll send this review back to you :)

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

96

This test probably won't work in remote test suite runs (which is essentially when LLDB's test suit is run but all the target programs are on another machine. Usually that's used for testing debugging on a phone). Mainly because the process we create here will be created on the system where the test runs and not the remote machine. But I also don't think there is a nice way to create a dummy process that we can attach to on a remote machine, so I think this is fine. But you should add a @skipIfRemote to the test that we don't see the test failures when someone runs the tests on a remote machine.

So like this:

@skipIfRemote
def test_process_attach_dash_p(self):

(You probably already saw the @skipIf... markers in LLDB tests. Those are just used to disable the test on certain configurations where they are not supposed to pass).

101

Can you add a comment that this is done because we need to have a process that we can always attach to (and that LLDB itself and an already attached process is out of question because we can't attach to those).

[MrHate]

1. added @skipIfRemote to the test case;
2. added the comment explaining the process creation.

[MrHate]

clang-formatted.

[MrHate]

• Set daemon to True on creating process, so that it will be killed with the test case ending (which was set to False by mistake).

[MrHate]

• Allow processes with empty names appear in the completion list.

[MrHate]

• Refactored the whole patch to provide two common completions: pid and process name.

[teemperor]

LGTM now

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

105

Why daemon=True should probably be documented. I can do that when merging.

[MrHate]

1. Rebased the patch with the latest master branch from Github;
2. In test, uses the pid got from psutil.pids()[0] instead of the one generated by creating a new process in case the pickle error.

[MrHate]

• Refactored the test with spawnSubprocess and modified main.cpp.

lldb/include/lldb/Interpreter/CommandCompletions.h

enum CommonCompletionTypes {
     eSettingsNameCompletion = (1u << 5),
     ePlatformPluginCompletion = (1u << 6),
     eArchitectureCompletion = (1u << 7),
     eVariablePathCompletion = (1u << 8),
     eRegisterCompletion = (1u << 9),
     eBreakpointCompletion = (1u << 10),
     eBreakpointNameCompletion = (1u << 11),
     eProcessPluginCompletion = (1u << 12),
+    eProcessIDCompletion = (1u << 13),
+    eProcessNameCompletion = (1u << 14),
     // 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 << 13)
+    eCustomCompletion = (1u << 15)
   };
 
   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,
public:
 
   static void BreakpointNames(CommandInterpreter &interpreter,
                               CompletionRequest &request,
                               SearchFilter *searcher);
 
   static void ProcessPluginNames(CommandInterpreter &interpreter,
                                  CompletionRequest &request,
                                  SearchFilter *searcher);
+
+  static void ProcessIDs(CommandInterpreter &interpreter,
+                         CompletionRequest &request, SearchFilter *searcher);
+
+  static void ProcessNames(CommandInterpreter &interpreter,
+                           CompletionRequest &request, SearchFilter *searcher);
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_INTERPRETER_COMMANDCOMPLETIONS_H

lldb/source/Commands/CommandCompletions.cpp

const CommonCompletionElement common_completions[] = {
       {eSettingsNameCompletion, CommandCompletions::SettingsNames},
       {ePlatformPluginCompletion, CommandCompletions::PlatformPluginNames},
       {eArchitectureCompletion, CommandCompletions::ArchitectureNames},
       {eVariablePathCompletion, CommandCompletions::VariablePath},
       {eRegisterCompletion, CommandCompletions::Registers},
       {eBreakpointCompletion, CommandCompletions::Breakpoints},
       {eBreakpointNameCompletion, CommandCompletions::BreakpointNames},
       {eProcessPluginCompletion, CommandCompletions::ProcessPluginNames},
+      {eProcessIDCompletion, CommandCompletions::ProcessIDs},
+      {eProcessNameCompletion, CommandCompletions::ProcessNames},
       {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 (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::ProcessIDs(CommandInterpreter &interpreter,
+                                    CompletionRequest &request,
+                                    SearchFilter *searcher) {
+  lldb::PlatformSP platform_sp(interpreter.GetPlatform(true));
+  if (!platform_sp)
+    return;
+  ProcessInstanceInfoList process_infos;
+  ProcessInstanceInfoMatch match_info;
+  platform_sp->FindProcesses(match_info, process_infos);
+  for (const ProcessInstanceInfo &info : process_infos)
+    request.TryCompleteCurrentArg(std::to_string(info.GetProcessID()),
+                                  info.GetNameAsStringRef());
+}
+
+void CommandCompletions::ProcessNames(CommandInterpreter &interpreter,
+                                      CompletionRequest &request,
+                                      SearchFilter *searcher) {
+  lldb::PlatformSP platform_sp(interpreter.GetPlatform(true));
+  if (!platform_sp)
+    return;
+  ProcessInstanceInfoList process_infos;
+  ProcessInstanceInfoMatch match_info;
+  platform_sp->FindProcesses(match_info, process_infos);
+  for (const ProcessInstanceInfo &info : process_infos)
+    request.TryCompleteCurrentArg(info.GetNameAsStringRef());
+}

lldb/source/Commands/CommandObjectPlatform.cpp

CommandObjectPlatformProcessInfo(CommandInterpreter &interpreter)
     arg.push_back(pid_args);
 
     // Push the data for the first argument into the m_arguments vector.
     m_arguments.push_back(arg);
   }
 
   ~CommandObjectPlatformProcessInfo() override = default;
 
+  void
+  HandleArgumentCompletion(CompletionRequest &request,
+                           OptionElementVector &opt_element_vector) override {
+    CommandCompletions::InvokeCommonCompletionCallbacks(
+        GetCommandInterpreter(), CommandCompletions::eProcessIDCompletion,
+        request, nullptr);
+  }
+
 protected:
   bool DoExecute(Args &args, CommandReturnObject &result) override {
     Target *target = GetDebugger().GetSelectedTarget().get();
     PlatformSP platform_sp;
     if (target) {
       platform_sp = target->GetPlatform();
     }
     if (!platform_sp) {
public:
     void OptionParsingStarting(ExecutionContext *execution_context) override {
       attach_info.Clear();
     }
 
     llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
       return llvm::makeArrayRef(g_platform_process_attach_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;
-
-      // We are only completing the name option for now...
-
-      // Are we in the name?
-      if (GetDefinitions()[opt_defs_index].short_option != 'n')
-        return;
-
-      // Look to see if there is a -P argument provided, and if so use that
-      // plugin, otherwise use the default plugin.
-
-      const char *partial_name = nullptr;
-      partial_name = request.GetParsedLine().GetArgumentAtIndex(opt_arg_pos);
-
-      PlatformSP platform_sp(interpreter.GetPlatform(true));
-      if (!platform_sp)
-        return;
-
-      ProcessInstanceInfoList process_infos;
-      ProcessInstanceInfoMatch match_info;
-      if (partial_name) {
-        match_info.GetProcessInfo().GetExecutableFile().SetFile(
-            partial_name, FileSpec::Style::native);
-        match_info.SetNameMatchType(NameMatch::StartsWith);
-      }
-      platform_sp->FindProcesses(match_info, process_infos);
-      const uint32_t num_matches = process_infos.size();
-      if (num_matches == 0)
-        return;
-
-      for (uint32_t i = 0; i < num_matches; ++i) {
-        request.AddCompletion(process_infos[i].GetNameAsStringRef());
-      }
-      return;
-    }
-
     // Options table: Required for subclasses of Options.
 
     static OptionDefinition g_option_table[];
 
     // Instance variables to hold the values for command options.
 
     ProcessAttachInfo attach_info;
   };

lldb/source/Commands/CommandObjectProcess.cpp

public:
     void OptionParsingStarting(ExecutionContext *execution_context) override {
       attach_info.Clear();
     }
 
     llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
       return llvm::makeArrayRef(g_process_attach_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 'n': {
-        // Look to see if there is a -P argument provided, and if so use that
-        // plugin, otherwise use the default plugin.
-
-        const char *partial_name = nullptr;
-        partial_name = request.GetParsedLine().GetArgumentAtIndex(opt_arg_pos);
-
-        PlatformSP platform_sp(interpreter.GetPlatform(true));
-        if (!platform_sp)
-          return;
-        ProcessInstanceInfoList process_infos;
-        ProcessInstanceInfoMatch match_info;
-        if (partial_name) {
-          match_info.GetProcessInfo().GetExecutableFile().SetFile(
-              partial_name, FileSpec::Style::native);
-          match_info.SetNameMatchType(NameMatch::StartsWith);
-        }
-        platform_sp->FindProcesses(match_info, process_infos);
-        const size_t num_matches = process_infos.size();
-        if (num_matches == 0)
-          return;
-        for (size_t i = 0; i < num_matches; ++i) {
-          request.AddCompletion(process_infos[i].GetNameAsStringRef());
-        }
-      } break;
-
-      case 'P':
-        CommandCompletions::InvokeCommonCompletionCallbacks(
-            interpreter, CommandCompletions::eProcessPluginCompletion, request,
-            nullptr);
-        break;
-      }
-    }
-
-    // Instance variables to hold the values for command options.
-
     ProcessAttachInfo attach_info;
   };
 
   CommandObjectProcessAttach(CommandInterpreter &interpreter)
       : CommandObjectProcessLaunchOrAttach(
             interpreter, "process attach", "Attach to a process.",
             "process attach <cmd-options>", 0, "attach"),
         m_options() {}

lldb/source/Interpreter/CommandObject.cpp

CommandObject::ArgumentTableEntry CommandObject::g_arguments_data[] = {
     { eArgTypeNumLines, "num-lines", CommandCompletions::eNoCompletion, { nullptr, false }, "The number of lines to use." },
     { eArgTypeNumberPerLine, "number-per-line", CommandCompletions::eNoCompletion, { nullptr, false }, "The number of items per line to display." },
     { eArgTypeOffset, "offset", CommandCompletions::eNoCompletion, { nullptr, false }, "Help text goes here." },
     { eArgTypeOldPathPrefix, "old-path-prefix", CommandCompletions::eNoCompletion, { nullptr, false }, "Help text goes here." },
     { eArgTypeOneLiner, "one-line-command", CommandCompletions::eNoCompletion, { nullptr, false }, "A command that is entered as a single line of text." },
     { eArgTypePath, "path", CommandCompletions::eDiskFileCompletion, { nullptr, false }, "Path." },
     { eArgTypePermissionsNumber, "perms-numeric", CommandCompletions::eNoCompletion, { nullptr, false }, "Permissions given as an octal number (e.g. 755)." },
     { eArgTypePermissionsString, "perms=string", CommandCompletions::eNoCompletion, { nullptr, false }, "Permissions given as a string value (e.g. rw-r-xr--)." },
-    { eArgTypePid, "pid", CommandCompletions::eNoCompletion, { nullptr, false }, "The process ID number." },
+    { eArgTypePid, "pid", CommandCompletions::eProcessIDCompletion, { nullptr, false }, "The process ID number." },
     { eArgTypePlugin, "plugin", CommandCompletions::eProcessPluginCompletion, { nullptr, false }, "Help text goes here." },
-    { eArgTypeProcessName, "process-name", CommandCompletions::eNoCompletion, { nullptr, false }, "The name of the process." },
+    { eArgTypeProcessName, "process-name", CommandCompletions::eProcessNameCompletion, { nullptr, false }, "The name of the process." },
     { eArgTypePythonClass, "python-class", CommandCompletions::eNoCompletion, { nullptr, false }, "The name of a Python class." },
     { eArgTypePythonFunction, "python-function", CommandCompletions::eNoCompletion, { nullptr, false }, "The name of a Python function." },
     { eArgTypePythonScript, "python-script", CommandCompletions::eNoCompletion, { nullptr, false }, "Source code written in Python." },
     { eArgTypeQueueName, "queue-name", CommandCompletions::eNoCompletion, { nullptr, false }, "The name of the thread queue." },
     { eArgTypeRegisterName, "register-name", CommandCompletions::eNoCompletion, { RegisterNameHelpTextCallback, true }, nullptr },
     { eArgTypeRegularExpression, "regular-expression", CommandCompletions::eNoCompletion, { nullptr, false }, "A POSIX-compliant extended regular expression." },
     { eArgTypeRunArgs, "run-args", CommandCompletions::eNoCompletion, { nullptr, false }, "Arguments to be passed to the target program when it starts executing." },
     { eArgTypeRunMode, "run-mode", CommandCompletions::eNoCompletion, { nullptr, false }, "Help text goes here." },

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

 """
 Test the lldb command line completion mechanism.
 """
 
 
-
 import os
+from multiprocessing import Process
+import psutil
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbplatform
 from lldbsuite.test import lldbutil
 
 
 class CommandLineCompletionTestCase(TestBase):
class CommandLineCompletionTestCase(TestBase):
 
     def test_process_plugin_completion(self):
         subcommands = ['attach -P', 'connect -p', 'launch -p']
 
         for subcommand in subcommands:
             self.complete_from_to('process ' + subcommand + ' mac',
                                   'process ' + subcommand + ' mach-o-core')
 
+    @skipIfRemote
+    def test_common_completion_process_pid_and_name(self):
+        # The LLDB process itself and the process already attached to are both
+        # ignored by the process discovery mechanism, thus we need to create a
+        # process known to us here.
+        def test():
+            while True:
+                time.sleep(1)
+
+        p = Process(target=test, daemon=True)
+        p.start()
+
+        self.complete_from_to('process attach -p ', [str(p.pid)])
+        self.complete_from_to('platform process attach -p ', [str(p.pid)])
+        self.complete_from_to('platform process info ', [str(p.pid)])
+
+        pname = psutil.Process(p.pid).name()
+        self.complete_from_to('process attach -n ', [str(pname)])
+        self.complete_from_to('platform process attach -n ', [str(pname)])
+
     def test_process_signal(self):
         # The tab completion for "process signal"  won't work without a running process.
         self.complete_from_to('process signal ',
                               'process signal ')
 
         # Test with a running process.
         self.build()
         self.main_source = "main.cpp"