[lldb] Tab completion for process load/unload

[MrHate]

Summary

1. Complete process load with the common disk file completion, so there is not test provided for it;
2. Complete process unload with the tokens of valid loaded images.

Thanks for Raphael's help on the test for process unload.

[teemperor]

I'm overall very happy with this patch. I have one change request though to prevent that we keep copy-pasting checks like if (m_exe_ctx.HasProcessScope()) around. See my inline comment for that. But beside that and the small typo I pointed out this is good to go.

lldb/include/lldb/Target/Process.h

735

Small detail: LLVM comments always have to end with a period '.' like a real sentence. Same with the sentence below.

lldb/source/Commands/CommandObjectProcess.cpp

918

I think we could prevent manual checks like this by checking the flags like eCommandRequiresProcess we set for the object (see the flags set in the constructor). If the command requires a process to be launched to do anything then I guess the same goes for the tab completion.

So I think the right way to do this is to never invoke HandleArgumentCompletion for a CommandObject that has eCommandRequiresProcess set if there current execution context doesn't have a process. I think this should be a separate patch (that doesn't need its own tests, but will be tested by the tests in here).

[MrHate]

Removed local requirements checking. Added periods.

[MrHate]

Maybe we can skip the unified requirements checking now, coz it has got many things to deal with.
Added back the local checking here.

[teemperor]

Can you also add a quick test for process load? Just copy the test_log_dir test but replace the dir completion with file completion (and you can just complete some dummy file, e.g. some empty dummy_file.so you can create in the test directory).

Beside that this LGTM.

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

99

Btw, we can now do self.assertSuccess(err) (which will print a nice error on its own).

[MrHate]

1. added a test case for process load;
2. replaced assertTrue(err.Success()...) with assertSuccess(err).

[JDevlieghere]

lldb/source/Commands/CommandObjectProcess.cpp

918

Spurious newline

993

In D83309 you wrote

if (!m_exe_ctx.HasTargetScope() || request.GetCursorIndex())

and CommandObjectProcessSignal::HandleArgumentCompletion in this file has

if (!m_exe_ctx.HasProcessScope() || request.GetCursorIndex() != 0)

Can we pick one and use (I like if (!m_exe_ctx.HasProcessScope() || request.GetCursorIndex())) and use that everywhere?

1000

Range-based for:

for (lldb::addr_t token : process->GetImageTokens())

[MrHate]

1. replaced request.GetCursorIndex() != 0 with request.GetCursorIndex();
2. renamed the loop variable at line 1005.

[MrHate]

lldb/source/Commands/CommandObjectProcess.cpp

1000

Maybe the loop variable name here led to misunderstanding. Cause I still need the index here, I cannot replace it with range-based for-loop.

[JDevlieghere]

lldb/source/Commands/CommandObjectProcess.cpp

1000

You're right, I missed that. It might still be less code to just have a range-based for-loop and have a counter?

[MrHate]

lldb/source/Commands/CommandObjectProcess.cpp

1000

With range-based for-loop I may code like this:

size_t token_idx = 0;
for (lldb::addr_t token : process->GetImageTokens()) {
  ++token_idx;
  if (token == LLDB_INVALID_IMAGE_TOKEN)
    continue;
  request.TryCompleteCurrentArg(std::to_string(token_idx));
}

The number of variable declaration lines is reduced by one, but there will be a counter increase line added. I think refactoring like this might not work much better. Do you have some suggestions?

[teemperor]

LGTM

lldb/include/lldb/Target/Process.h

public:
   /// \return
   ///     Returns an error object.
   virtual Status ConnectRemote(Stream *strm, llvm::StringRef remote_url);
 
   bool GetShouldDetach() const { return m_should_detach; }
 
   void SetShouldDetach(bool b) { m_should_detach = b; }
 
+  /// Get the image vector for the current process.
+  ///
+  /// \return
+  ///     The constant reference to the member m_image_tokens.
+  const std::vector<lldb::addr_t>& GetImageTokens() { return m_image_tokens; }
+
   /// Get the image information address for the current process.
   ///
   /// Some runtimes have system functions that can help dynamic loaders locate
   /// the dynamic loader information needed to observe shared libraries being
   /// loaded or unloaded. This function is in the Process interface (as
   /// opposed to the DynamicLoader interface) to ensure that remote debugging
   /// can take advantage of this functionality.
   ///

lldb/source/Commands/CommandObjectProcess.cpp

CommandObjectProcessLoad(CommandInterpreter &interpreter)
                             "process load <filename> [<filename> ...]",
                             eCommandRequiresProcess | eCommandTryTargetAPILock |
                                 eCommandProcessMustBeLaunched |
                                 eCommandProcessMustBePaused),
         m_options() {}
 
   ~CommandObjectProcessLoad() override = default;
 
+  void
+  HandleArgumentCompletion(CompletionRequest &request,
+                           OptionElementVector &opt_element_vector) override {
+
+    CommandCompletions::InvokeCommonCompletionCallbacks(
+        GetCommandInterpreter(), CommandCompletions::eDiskFileCompletion,
+        request, nullptr);
+  }
+
   Options *GetOptions() override { return &m_options; }
 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     Process *process = m_exe_ctx.GetProcessPtr();
 
     for (auto &entry : command.entries()) {
       Status error;
CommandObjectProcessUnload(CommandInterpreter &interpreter)
             "Unload a shared library from the current process using the index "
             "returned by a previous call to \"process load\".",
             "process unload <index>",
             eCommandRequiresProcess | eCommandTryTargetAPILock |
                 eCommandProcessMustBeLaunched | eCommandProcessMustBePaused) {}
 
   ~CommandObjectProcessUnload() override = default;
 
+  void
+  HandleArgumentCompletion(CompletionRequest &request,
+                           OptionElementVector &opt_element_vector) override {
+
+    if (request.GetCursorIndex() != 0)
+      return;
+
+    Process *process = m_exe_ctx.GetProcessPtr();
+
+    const std::vector<lldb::addr_t> &tokens = process->GetImageTokens();
+    const size_t token_num = tokens.size();
+    for(size_t token = 0; token < token_num; ++ token) {
+      if (tokens[token] == LLDB_INVALID_IMAGE_TOKEN)
+        continue;
+      request.TryCompleteCurrentArg(std::to_string(token));
+    }
+  }
+
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     Process *process = m_exe_ctx.GetProcessPtr();
 
     for (auto &entry : command.entries()) {
       uint32_t image_token;
       if (entry.ref().getAsInteger(0, image_token)) {
         result.AppendErrorWithFormat("invalid image index argument '%s'",

lldb/test/API/functionalities/completion/Makefile

 CXX_SOURCES := main.cpp
+USE_LIBDL := 1
+
+a.out: lib_shared
+
+lib_shared:
+  $(MAKE) -f $(MAKEFILE_RULES) \
+    DYLIB_ONLY=YES DYLIB_CXX_SOURCES=shared.cpp DYLIB_NAME=shared
 
 include Makefile.rules

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

def test_process_attach_dash_dash_con(self):
             'process attach --con',
             'process attach --continue ')
 
     def test_process_launch_arch(self):
         self.complete_from_to('process launch --arch ',
                               ['mips',
                                'arm64'])
 
+    @skipUnlessPlatform(["linux"])
+    def test_process_unload(self):
+        """Test the completion for "process unload <index>" """
+        # This tab completion should not work without a running process.
+        self.complete_from_to('process unload ',
+                              'process unload ')
+
+        self.build()
+        lldbutil.run_to_source_breakpoint(self, '// Break here', lldb.SBFileSpec("main.cpp"))
+        err = lldb.SBError()
+        self.process().LoadImage(lldb.SBFileSpec(self.getBuildArtifact("libshared.so")), err)
+        self.assertTrue(err.Success(), str(err) + str(self.getBuildArtifact("libshared.so")))
+
+        self.complete_from_to('process unload ',
+                              'process unload 0')
+        
+        self.process().UnloadImage(0)
+        self.complete_from_to('process unload ',
+                              'process unload ')
+
     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"

lldb/test/API/functionalities/completion/shared.cpp

+int foo() {
+  return 123;
+}