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

[wallace]

Summary

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.

[wallace]

Fixed another bug and added tests

[clayborg]

lldb/include/lldb/Target/PathMappingList.h

75

"only_if_exists" be a better name for "check_filesystem"

lldb/source/Target/PathMappingList.cpp

167–168

Would "only_if_exists" be a better name for "check_filesystem"?

220–221

"only_if_exists" be a better name for "check_filesystem

[wallace]

As a note, the test is an lldb-vscode test, because I already have a set up for source maps in this kind of scenario, from breakpoints to frames, so it was easy.

[wallace]

improve the variable name

[wallace]

last nit

lldb/include/lldb/Target/PathMappingList.h

public:
   ///
   /// Remaps \a path if any source remappings match. This function
   /// does NOT stat the file system so it can be used in tight loops
   /// where debug info is being parsed.
   ///
   /// \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;
 
   /// Finds a source file given a file spec using the path remappings.
   ///
   /// Tries to resolve \a orig_spec by checking the path remappings.
   /// It makes sure the file exists by checking with the file system,

lldb/source/Target/PathMappingList.cpp

static void AppendPathComponents(FileSpec &path, llvm::StringRef components,
     auto e = llvm::sys::path::end(components);
     while (component != e &&
         llvm::sys::path::is_separator(*component->data(), style))
       ++component;
     for (; component != e; ++component)
       path.AppendPathComponent(*component);
 }
 
 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 "."
       // carefully.
       if (prefix != ".")
         continue;
       // We need to figure out if the "path" argument is relative. If it is,
       // then we should remap, else skip this entry.
       if (path_is_relative == eLazyBoolCalculate) {
         path_is_relative =
             FileSpec(path).IsRelative() ? eLazyBoolYes : eLazyBoolNo;
       }
       if (!path_is_relative)
         continue;
     }
     FileSpec remapped(it.second.GetStringRef());
     auto orig_style = FileSpec::GuessPathStyle(prefix).getValueOr(
         llvm::sys::path::Style::native);
     AppendPathComponents(remapped, path, orig_style);
+    if (!only_if_exists || FileSystem::Instance().Exists(remapped))
       return remapped;
   }
   return {};
 }
 
 bool PathMappingList::ReverseRemapPath(const FileSpec &file, FileSpec &fixed) const {
   std::string path = file.GetPath();
   llvm::StringRef path_ref(path);
   for (const auto &it : m_pairs) {
     if (!path_ref.consume_front(it.second.GetStringRef()))
       continue;
     auto orig_file = it.first.GetStringRef();
     auto orig_style = FileSpec::GuessPathStyle(orig_file).getValueOr(
         llvm::sys::path::Style::native);
     fixed.SetFile(orig_file, orig_style);
     AppendPathComponents(fixed, path_ref, orig_style);
     return true;
   }
   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 {};
 }
 
 bool PathMappingList::Replace(ConstString path,
                               ConstString new_path, bool notify) {
   uint32_t idx = FindIndexForPath(path);
   if (idx < m_pairs.size()) {

lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py

def setUp(self):
         lldbvscode_testcase.VSCodeTestCaseBase.setUp(self)
 
         self.main_basename = 'main-copy.cpp'
         self.main_path = os.path.realpath(self.getBuildArtifact(self.main_basename))
 
     @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'
         other_path = self.getBuildArtifact(other_basename)
 
         source_folder = os.path.dirname(self.main_path)
 
         new_main_folder = os.path.join(source_folder, 'moved_main')
def test_source_map(self):
         response = self.vscode.request_setBreakpoints(new_other_path, [other_line])
         breakpoints = response['body']['breakpoints']
         breakpoint = breakpoints[0]
         self.assertEqual(breakpoint['line'], other_line)
         self.assertTrue(breakpoint['verified'])
         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):
         '''Tests setting and clearing source file and line breakpoints.
            This packet is a bit tricky on the debug adaptor side since there
            is no "clearBreakpoints" packet. Source file and line breakpoints
            are set by sending a "setBreakpoints" packet with a source file
            specified and zero or more source lines. If breakpoints have been