[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

623 ↗

(On Diff #268760)

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/source/Commands/CommandObjectProcess.cpp

void HandleOptionArgumentCompletion(
         }
       } break;
 
       case 'P':
         CommandCompletions::InvokeCommonCompletionCallbacks(
             interpreter, CommandCompletions::eProcessPluginCompletion, request,
             nullptr);
         break;
+
+      case 'p':
+        PlatformSP platform_sp(interpreter.GetPlatform(true));
+        if (!platform_sp)
+          return;
+        ProcessInstanceInfoList process_infos;
+        ProcessInstanceInfoMatch match_info;
+        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) {
+          const llvm::StringRef process_name =
+              process_infos[i].GetNameAsStringRef();
+          if (process_name.empty())
+            continue;
+          request.TryCompleteCurrentArg(
+              std::to_string(process_infos[i].GetProcessID()), process_name);
+        }
+        break;
       }
     }
 
     // Instance variables to hold the values for command options.
 
     ProcessAttachInfo attach_info;
   };
 

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

 """
 Test the lldb command line completion mechanism.
 """
 
 
 
 import os
+from multiprocessing import Process
 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')
 
+    def test_process_attach_dash_p(self):
+        def test():
+            while True:
+                pass
+        
+        p = Process(target=test)
+        p.daemon = True
+        p.start()
+        self.complete_from_to('process attach -p ', [str(p.pid)])
+        p.terminate()
+
     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"