[lldb] Fix module name tab completion

[labath]

Module names can be matched either by a full path or just their basename. The completion machinery tried to do both, but had several bugs:

• it always inserted the basename as a completion candidate, even if the string being completed was a full path
• due to FileSpec canonicalization, it lost information about trailing slashes (it treated "lib/" as "lib", even though it's clear the former was trying to complete a directory name)
• due to both of the previous issues, the completion candidates could end up being shorter than the string being completed, which caused crashes (string out of range errors) when attempting to substitute the results.

This patch rewrites to logic to remove these kinds of issues:

• basename and full path completion are handled separately
• full path completion is attempted always, basename only if the input string does not contain a slash
• the code remembers both the canonical and original spelling or the completed argument. The canonical arg is used for matching, while the original spelling is used for completion. This way "/foo///.//b" can still match "/foo/bar", but it will complete to "/foo///.//bar".

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

Module names can be matched either by a full path or just their basename. The completion machinery tried to do both, but had several bugs:

• it always inserted the basename as a completion candidate, even if the string being completed was a full path
• due to FileSpec canonicalization, it lost information about trailing slashes (it treated "lib/<TAB>" as "lib<TAB>", even though it's clear the former was trying to complete a directory name)
• due to both of the previous issues, the completion candidates could end up being shorter than the string being completed, which caused crashes (string out of range errors) when attempting to substitute the results.

This patch rewrites to logic to remove these kinds of issues:

• basename and full path completion are handled separately
• full path completion is attempted always, basename only if the input string does not contain a slash
• the code remembers both the canonical and original spelling or the completed argument. The canonical arg is used for matching, while the original spelling is used for completion. This way "/foo///.//b<TAB>" can still match "/foo/bar", but it will complete to "/foo///.//bar".

---

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

2 Files Affected:

• (modified) lldb/source/Commands/CommandCompletions.cpp (+37-21)
• (modified) lldb/test/API/functionalities/completion/TestCompletion.py (+35)

diff --git a/lldb/source/Commands/CommandCompletions.cpp b/lldb/source/Commands/CommandCompletions.cpp
index 16078a92ab5fe..baa9dda1f68e6 100644
--- a/lldb/source/Commands/CommandCompletions.cpp
+++ b/lldb/source/Commands/CommandCompletions.cpp
@@ -6,7 +6,9 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 
 #include "lldb/Breakpoint/Watchpoint.h"
@@ -262,9 +264,26 @@ class ModuleCompleter : public Completer {
 public:
   ModuleCompleter(CommandInterpreter &interpreter, CompletionRequest &request)
       : Completer(interpreter, request) {
-    FileSpec partial_spec(m_request.GetCursorArgumentPrefix());
-    m_file_name = partial_spec.GetFilename().GetCString();
-    m_dir_name = partial_spec.GetDirectory().GetCString();
+    llvm::StringRef request_str = m_request.GetCursorArgumentPrefix();
+    // We can match the full path, or the file name only. The full match will be
+    // attempted always, the file name match only if the request does not
+    // contain a path separator.
+
+    // Preserve both the path as spelled by the user (used for completion) and
+    // the canonical version (used for matching).
+    m_spelled_path = request_str;
+    m_canonical_path = FileSpec(m_spelled_path).GetPath();
+    if (!m_spelled_path.empty() &&
+        llvm::sys::path::is_separator(m_spelled_path.back()) &&
+        !llvm::StringRef(m_canonical_path).ends_with(m_spelled_path.back())) {
+      m_canonical_path += m_spelled_path.back();
+    }
+
+    bool has_separator = llvm::find_if(request_str, [](char c) {
+                           return llvm::sys::path::is_separator(c);
+                         }) != request_str.end();
+    m_file_name =
+        has_separator ? llvm::sys::path::get_separator() : request_str;
   }
 
   lldb::SearchDepth GetDepth() override { return lldb::eSearchDepthModule; }
@@ -273,22 +292,18 @@ class ModuleCompleter : public Completer {
                                           SymbolContext &context,
                                           Address *addr) override {
     if (context.module_sp) {
-      const char *cur_file_name =
-          context.module_sp->GetFileSpec().GetFilename().GetCString();
-      const char *cur_dir_name =
-          context.module_sp->GetFileSpec().GetDirectory().GetCString();
-
-      bool match = false;
-      if (m_file_name && cur_file_name &&
-          strstr(cur_file_name, m_file_name) == cur_file_name)
-        match = true;
-
-      if (match && m_dir_name && cur_dir_name &&
-          strstr(cur_dir_name, m_dir_name) != cur_dir_name)
-        match = false;
-
-      if (match) {
-        m_request.AddCompletion(cur_file_name);
+      // Attempt a full path match.
+      std::string cur_path = context.module_sp->GetFileSpec().GetPath();
+      llvm::StringRef cur_path_view = cur_path;
+      if (cur_path_view.consume_front(m_canonical_path))
+        m_request.AddCompletion((m_spelled_path + cur_path_view).str());
+
+      // And a file name match.
+      if (m_file_name) {
+        llvm::StringRef cur_file_name =
+            context.module_sp->GetFileSpec().GetFilename().GetStringRef();
+        if (cur_file_name.starts_with(*m_file_name))
+          m_request.AddCompletion(cur_file_name);
       }
     }
     return Searcher::eCallbackReturnContinue;
@@ -297,8 +312,9 @@ class ModuleCompleter : public Completer {
   void DoCompletion(SearchFilter *filter) override { filter->Search(*this); }
 
 private:
-  const char *m_file_name;
-  const char *m_dir_name;
+  std::optional<llvm::StringRef> m_file_name;
+  llvm::StringRef m_spelled_path;
+  std::string m_canonical_path;
 
   ModuleCompleter(const ModuleCompleter &) = delete;
   const ModuleCompleter &operator=(const ModuleCompleter &) = delete;
diff --git a/lldb/test/API/functionalities/completion/TestCompletion.py b/lldb/test/API/functionalities/completion/TestCompletion.py
index 15aeaf8d0e897..15e326cdca2f9 100644
--- a/lldb/test/API/functionalities/completion/TestCompletion.py
+++ b/lldb/test/API/functionalities/completion/TestCompletion.py
@@ -906,3 +906,38 @@ def test_ambiguous_command(self):
     def test_ambiguous_subcommand(self):
         """Test completing a subcommand of an ambiguous command"""
         self.complete_from_to("settings s ta", [])
+
+    def test_shlib_name(self):
+        self.build()
+        target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+        self.assertTrue(target, VALID_TARGET)
+        self.registerSharedLibrariesWithTarget(target, ["shared"])
+
+        basenames = []
+        paths = []
+        for m in target.modules:
+          basenames.append(m.file.basename)
+          paths.append(m.file.fullpath)
+
+        # To see all the diffs
+        self.maxDiff = None
+
+        # An empty string completes to everything
+        self.completions_match("target symbols add -s ", basenames+paths)
+
+        # Base name completion
+        self.completions_match("target symbols add -s a.", ["a.out"])
+
+        # Full path completion
+        prefix = os.path.commonpath(paths)
+        self.completions_match("target symbols add -s '" + prefix, paths)
+
+        # Full path, but ending with a path separator
+        prefix_sep = prefix + os.path.sep
+        self.completions_match("target symbols add -s '" + prefix_sep, paths)
+
+        # The completed path should match the spelling of the input, so if the
+        # input contains a double separator, so should the completions.
+        prefix_sep_sep = prefix_sep + os.path.sep
+        paths_sep = [ prefix_sep_sep +p[len(prefix_sep):] for p in paths]
+        self.completions_match("target symbols add -s '" + prefix_sep_sep, paths_sep)

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[DavidSpickett]

LGTM

[Michael137]

Looks like this is breaking the macOS CI: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/lldb-cmake/2534/execution/node/97/log

======================================================================
FAIL: test_shlib_name (TestCompletion.CommandLineCompletionTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake/llvm-project/lldb/test/API/functionalities/completion/TestCompletion.py", line 926, in test_shlib_name
    self.completions_match("target symbols add -s ", basenames + paths)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2219, in completions_match
    self.assertCountEqual(
AssertionError: Element counts were not equal:
First has 1, Second has 0:  'dyld'
First has 2, Second has 1:  'libc++.1.dylib'
First has 1, Second has 0:  '/usr/lib/dyld' : List of returned completion is wrong
Config=x86_64-/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake/lldb-build/bin/clang
======================================================================
FAIL: test_shlib_name (TestCompletion.CommandLineCompletionTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2017, in tearDown
    Base.tearDown(self)
  File "/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1132, in tearDown
    self.assertEqual(lldb.SBModule.GetNumberAllocatedModules(), 0)
AssertionError: 1 != 0
Config=x86_64-/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake/lldb-build/bin/clang
----------------------------------------------------------------------
Ran 86 tests in 37.855s

FAILED (failures=2, skipped=1)

Mind taking a look @labath ?

[labath]

Mind taking a look @labath ?

I think I know how to fix this, just a sec.