[Lldb-commits] [lldb] ef8c684 - [source maps] fix source mapping when there are multiple matching rules

[Walter Erquinigo via lldb-commits]

Author: Walter Erquinigo
Date: 2021-07-23T17:53:12-07:00
New Revision: ef8c6849a235e97b8b981e0f998d430fdbd7bc2a

URL: https://github.com/llvm/llvm-project/commit/ef8c6849a235e97b8b981e0f998d430fdbd7bc2a
DIFF: https://github.com/llvm/llvm-project/commit/ef8c6849a235e97b8b981e0f998d430fdbd7bc2a.diff

LOG: [source maps] fix source mapping when there are multiple matching rules

D104406 introduced an error in which, if there are multiple matchings rules for a given path, lldb was only checking for the validity in the filesystem of the first match instead of looking exhaustively one by one until a valid file is found.

Besides that, a call to consume_front was being done incorrectly, as it was modifying the input, which renders subsequent matches incorrect.

I added a test that checks for both cases.

Differential Revision: https://reviews.llvm.org/D106723

Added: 
    

Modified: 
    lldb/include/lldb/Target/PathMappingList.h
    lldb/source/Target/PathMappingList.cpp
    lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py

Removed: 
    


################################################################################
diff  --git a/lldb/include/lldb/Target/PathMappingList.h b/lldb/include/lldb/Target/PathMappingList.h
index 46d7a427d3071..d788d120c47e9 100644
--- a/lldb/include/lldb/Target/PathMappingList.h
+++ b/lldb/include/lldb/Target/PathMappingList.h
@@ -72,9 +72,19 @@ class PathMappingList {
   /// \param[in] path
   ///     The original source file path to try and remap.
   ///
+  /// \param[in] only_if_exists
+  ///     If \b true, besides matching \p path with the remapping rules, this
+  ///     tries to check with the filesystem that the remapped file exists. If
+  ///     no valid file is found, \b None is returned. This might be expensive,
+  ///     specially on a network.
+  ///
+  ///     If \b false, then the existence of the returned remapping is not
+  ///     checked.
+  ///
   /// \return
   ///     The remapped filespec that may or may not exist on disk.
-  llvm::Optional<FileSpec> RemapPath(llvm::StringRef path) const;
+  llvm::Optional<FileSpec> RemapPath(llvm::StringRef path,
+                                     bool only_if_exists = false) const;
   bool RemapPath(const char *, std::string &) const = delete;
 
   bool ReverseRemapPath(const FileSpec &file, FileSpec &fixed) const;

diff  --git a/lldb/source/Target/PathMappingList.cpp b/lldb/source/Target/PathMappingList.cpp
index 86b0ad5f933ff..b660c310ef31a 100644
--- a/lldb/source/Target/PathMappingList.cpp
+++ b/lldb/source/Target/PathMappingList.cpp
@@ -165,12 +165,17 @@ static void AppendPathComponents(FileSpec &path, llvm::StringRef components,
 }
 
 llvm::Optional<FileSpec>
-PathMappingList::RemapPath(llvm::StringRef path) const {
-  if (m_pairs.empty() || path.empty())
+PathMappingList::RemapPath(llvm::StringRef mapping_path,
+                           bool only_if_exists) const {
+  if (m_pairs.empty() || mapping_path.empty())
     return {};
   LazyBool path_is_relative = eLazyBoolCalculate;
+
   for (const auto &it : m_pairs) {
-    auto prefix = it.first.GetStringRef();
+    llvm::StringRef prefix = it.first.GetStringRef();
+    // We create a copy of mapping_path because StringRef::consume_from
+    // effectively modifies the instance itself.
+    llvm::StringRef path = mapping_path;
     if (!path.consume_front(prefix)) {
       // Relative paths won't have a leading "./" in them unless "." is the
       // only thing in the relative path so we need to work around "."
@@ -190,7 +195,8 @@ PathMappingList::RemapPath(llvm::StringRef path) const {
     auto orig_style = FileSpec::GuessPathStyle(prefix).getValueOr(
         llvm::sys::path::Style::native);
     AppendPathComponents(remapped, path, orig_style);
-    return remapped;
+    if (!only_if_exists || FileSystem::Instance().Exists(remapped))
+      return remapped;
   }
   return {};
 }
@@ -211,11 +217,9 @@ bool PathMappingList::ReverseRemapPath(const FileSpec &file, FileSpec &fixed) co
   return false;
 }
 
-
 llvm::Optional<FileSpec> PathMappingList::FindFile(const FileSpec &orig_spec) const {
-  if (auto remapped = RemapPath(orig_spec.GetPath()))
-    if (FileSystem::Instance().Exists(*remapped))
-      return remapped;
+  if (auto remapped = RemapPath(orig_spec.GetPath(), /*only_if_exists=*/true))
+    return remapped;
 
   return {};
 }

diff  --git a/lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py b/lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py
index 5c129cb91825a..626a6536d6b96 100644
--- a/lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py
+++ b/lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py
@@ -26,6 +26,12 @@ def setUp(self):
     @skipIfWindows
     @skipIfRemote
     def test_source_map(self):
+        """
+        This test simulates building two files in a folder, and then moving
+        each source to a 
diff erent folder. Then, the debug session is started
+        with the corresponding source maps to have breakpoints and frames
+        working.
+        """
         self.build_and_create_debug_adaptor()
 
         other_basename = 'other-copy.c'
@@ -87,6 +93,16 @@ def test_source_map(self):
         self.assertEqual(other_basename, breakpoint['source']['name'])
         self.assertEqual(new_other_path, breakpoint['source']['path'])
 
+        # now we check the stack trace making sure that we got mapped source paths
+        frames = self.vscode.request_stackTrace()['body']['stackFrames']
+
+        self.assertEqual(frames[0]['source']['name'], other_basename)
+        self.assertEqual(frames[0]['source']['path'], new_other_path)
+
+        self.assertEqual(frames[1]['source']['name'], self.main_basename)
+        self.assertEqual(frames[1]['source']['path'], new_main_path)
+
+
     @skipIfWindows
     @skipIfRemote
     def test_set_and_clear(self):