[lldb] Fallback to comparing modules by their canonical paths

[charles-zablit]

Module::MatchesModuleSpec compares 2 modules paths using their FileSpec. If one of the modules is a symlink (or a subst drive on Windows), that comparison will fail even if the modules are the same. This causes breakpoints to be resolved twice.

This patch adds the FileSpecsResolveToSameFile method as a fallback to the Module::MatchesModuleSpec comparison. If the comparison fails, try to compare the module using their real path (canonicalized paths). It also adds a regression test.

This is a problem in Swiftlang on Windows where we run our tests with a subst drive. The impacted tests are:

• lang/c/{array_types,function_types,set_values,enum_types,anonymous,forward,local_variables}
• lang/cpp/{class_types,inlines}
• commands/{apropos/with-process,command/nested_alias,memory/write}
• functionalities/{memory/find,dead-strip,memory/holes}
• commands/expression/entry-bp

[llvmorg-github-actions]

@llvm/pr-subscribers-lldb

Author: Charles Zablit (charles-zablit)

Changes

Module::MatchesModuleSpec compares 2 modules paths using their FileSpec. If one of the modules is a symlink (or a subst drive on Windows), that comparison will fail even if the modules are the same. This causes breakpoints to be resolved twice.

This patch adds the FileSpecsResolveToSameFile method as a fallback to the Module::MatchesModuleSpec comparison. If the comparison fails, try to compare the module using their real path (canonicalized paths). It also adds a regression test.

---

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

4 Files Affected:

• (modified) lldb/source/Core/Module.cpp (+23-1)
• (added) lldb/test/API/functionalities/breakpoint/breakpoint_via_symlink/Makefile (+3)
• (added) lldb/test/API/functionalities/breakpoint/breakpoint_via_symlink/TestBreakpointViaSymlink.py (+70)
• (added) lldb/test/API/functionalities/breakpoint/breakpoint_via_symlink/main.c (+3)

diff --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
index dcdfd67d3e303..1493c0291c572 100644
--- a/lldb/source/Core/Module.cpp
+++ b/lldb/source/Core/Module.cpp
@@ -54,6 +54,7 @@
 #endif
 
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/DJB.h"
 #include "llvm/Support/FileSystem.h"
@@ -1455,6 +1456,25 @@ bool Module::SetLoadAddress(Target &target, lldb::addr_t value,
   return false;
 }
 
+// A module's stored path can be an alias of the same on-disk file that a lookup
+// spec refers to:
+//   - Windows: a subst/mapped drive or a symbolic link
+//   - POSIX: a symbolic link
+// In those cases, MatchesModuleSpec fails even though both name the same file.
+// This is a fall back which compares the canonicalized real paths.
+static bool FileSpecsResolveToSameFile(const FileSpec &pattern,
+                                       const FileSpec &file) {
+  if (!pattern.GetDirectory() || !file)
+    return false;
+  FileSystem &fs = FileSystem::Instance();
+  llvm::SmallString<256> pattern_real, file_real;
+  if (fs.GetRealPath(pattern.GetPath(), pattern_real) ||
+      fs.GetRealPath(file.GetPath(), file_real))
+    return false;
+  return FileSpec::Equal(FileSpec(pattern_real), FileSpec(file_real),
+                         /*full=*/true);
+}
+
 bool Module::MatchesModuleSpec(const ModuleSpec &module_ref) {
   const UUID &uuid = module_ref.GetUUID();
 
@@ -1465,7 +1485,9 @@ bool Module::MatchesModuleSpec(const ModuleSpec &module_ref) {
 
   const FileSpec &file_spec = module_ref.GetFileSpec();
   if (!FileSpec::Match(file_spec, m_file) &&
-      !FileSpec::Match(file_spec, m_platform_file))
+      !FileSpec::Match(file_spec, m_platform_file) &&
+      !FileSpecsResolveToSameFile(file_spec, m_file) &&
+      !FileSpecsResolveToSameFile(file_spec, m_platform_file))
     return false;
 
   const FileSpec &platform_file_spec = module_ref.GetPlatformFileSpec();
diff --git a/lldb/test/API/functionalities/breakpoint/breakpoint_via_symlink/Makefile b/lldb/test/API/functionalities/breakpoint/breakpoint_via_symlink/Makefile
new file mode 100644
index 0000000000000..10495940055b6
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/breakpoint_via_symlink/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/test/API/functionalities/breakpoint/breakpoint_via_symlink/TestBreakpointViaSymlink.py b/lldb/test/API/functionalities/breakpoint/breakpoint_via_symlink/TestBreakpointViaSymlink.py
new file mode 100644
index 0000000000000..defb23cc16f37
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/breakpoint_via_symlink/TestBreakpointViaSymlink.py
@@ -0,0 +1,70 @@
+"""
+Test that creating a target through a symlinked (or otherwise aliased)
+executable does not create a duplicate module at launch.
+
+When a target is created via a path that is not the file's canonical path (a
+symlink, or on Windows a subst/mapped drive), the running process reports its
+*real* image path at launch. Module matching needs to compare the canonicalized
+paths.
+"""
+
+import os
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestBreakpointViaSymlink(TestBase):
+    NO_DEBUG_INFO_TESTCASE = True
+
+    @skipIfRemote
+    def test_breakpoint_resolves_once_via_symlink(self):
+        self.build()
+        real_exe = self.getBuildArtifact("a.out")
+
+        # A symlink whose real path differs from the path used to create the
+        # target.
+        link_exe = self.getBuildArtifact("a.out.symlink")
+        try:
+            if os.path.lexists(link_exe):
+                os.remove(link_exe)
+            os.symlink(real_exe, link_exe)
+        except (OSError, NotImplementedError) as e:
+            # Windows needs Developer Mode / SeCreateSymbolicLinkPrivilege.
+            self.skipTest("could not create a symlink: %s" % e)
+        self.addTearDownHook(lambda: os.remove(link_exe))
+        self.assertNotEqual(os.path.realpath(link_exe), link_exe)
+
+        target = self.dbg.CreateTarget(link_exe)
+        self.assertTrue(target, VALID_TARGET)
+
+        bkpt = target.BreakpointCreateBySourceRegex(
+            "break here", lldb.SBFileSpec("main.c")
+        )
+        # Resolves against the preloaded module: exactly one location.
+        self.assertEqual(
+            bkpt.GetNumLocations(), 1, "one breakpoint location before launch"
+        )
+
+        process = target.LaunchSimple(None, None, self.get_process_working_directory())
+        self.assertState(process.GetState(), lldb.eStateStopped)
+
+        # After launch the running image may report its real path. With module
+        # matching canonicalizing paths, the loaded module is the one lldb already
+        # preloaded, so the breakpoint keeps exactly one resolved location.
+        self.assertEqual(bkpt.GetNumLocations(), 1)
+        self.assertEqual(bkpt.GetNumResolvedLocations(), 1)
+
+        # The executable appears as a single module (no duplicate). The module
+        # keeps the path it was created with (the symlink), so compare by real
+        # path.
+        exe_real = os.path.realpath(real_exe)
+        matches = [
+            m
+            for m in target.module_iter()
+            if os.path.realpath(m.GetFileSpec().fullpath) == exe_real
+        ]
+        self.assertEqual(
+            len(matches), 1, "exactly one executable module, got %d" % len(matches)
+        )
diff --git a/lldb/test/API/functionalities/breakpoint/breakpoint_via_symlink/main.c b/lldb/test/API/functionalities/breakpoint/breakpoint_via_symlink/main.c
new file mode 100644
index 0000000000000..9ba28de2ae01c
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/breakpoint_via_symlink/main.c
@@ -0,0 +1,3 @@
+int main(int argc, char **argv) {
+  return 0; // break here
+}

[jimingham]

Why is this function asymmetrical w.r.t. inputs that don't have directories? It makes it seem like the pattern and file arguments are somehow treated differently. But if the file doesn't have a directory, GetRealPath(file.GetPath(),...) will fail, and you will still get false.

That makes sense, if either of the files don't have directories, they can't resolve to anything so you can't make a statement about whether they resolve to the same thing.

That would make me wonder - if I had to used it - which one I should pass for the first and second arguments, but IRL that doesn't make any difference.

[charles-zablit]

I fixed the asymmetry, it did not add anything to the table.

[jimingham]

You have to be careful never to change the way the user spelled the module for the main binary, since some tricky Unix programs work by symlinking a bunch of executables to the same one and switching the behavior off the value of argv[0].

With your change, if you have a binary called foo and a symlink to it called bar and we first made a target with foo, then made a new target with bar - the second target will now reuse the Module we read in for foo. But we still have to ensure that when the second target is run, we pass argv0 of bar not foo. I can't tell whether this change will cause us to make that mistake, but it opens the window for it so we should test that that still works if we don't already.

[charles-zablit]

You have to be careful never to change the way the user spelled the module for the main binary, since some tricky Unix programs work by symlinking a bunch of executables to the same one and switching the behavior off the value of argv[0].

With your change, if you have a binary called foo and a symlink to it called bar and we first made a target with foo, then made a new target with bar - the second target will now reuse the Module we read in for foo. But we still have to ensure that when the second target is run, we pass argv0 of bar not foo. I can't tell whether this change will cause us to make that mistake, but it opens the window for it so we should test that that still works if we don't already.

This did introduce a regression. I added a test to catch it. The fix was to resolve the path the user used in Target.cpp and preserve it, but I don't think that's a strong solution and could break elsewhere.