[Lldb-commits] [PATCH] D106723: [source maps] fix source mapping when there are multiple matching rules

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 361390.
wallace added a comment.

last nit


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106723/new/

https://reviews.llvm.org/D106723

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


Index: lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py
===================================================================
--- lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py
+++ lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py
@@ -26,6 +26,12 @@
     @skipIfWindows
     @skipIfRemote
     def test_source_map(self):
+        """
+        This test simulates building two files in a folder, and then moving
+        each source to a different 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 @@
         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):
Index: lldb/source/Target/PathMappingList.cpp
===================================================================
--- lldb/source/Target/PathMappingList.cpp
+++ lldb/source/Target/PathMappingList.cpp
@@ -165,12 +165,17 @@
 }
 
 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 @@
     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 @@
   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 {};
 }
Index: lldb/include/lldb/Target/PathMappingList.h
===================================================================
--- lldb/include/lldb/Target/PathMappingList.h
+++ lldb/include/lldb/Target/PathMappingList.h
@@ -72,9 +72,19 @@
   /// \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;

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits