From: Walter Erquinigo Date: Fri, 23 Jul 2021 23:21:05 +0000 (-0700) Subject: [source maps] fix source mapping when there are multiple matching rules X-Git-Tag: llvmorg-14-init~401 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=ef8c6849a235e97b8b981e0f998d430fdbd7bc2a;p=platform%2Fupstream%2Fllvm.git [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 --- diff --git a/lldb/include/lldb/Target/PathMappingList.h b/lldb/include/lldb/Target/PathMappingList.h index 46d7a42..d788d12 100644 --- a/lldb/include/lldb/Target/PathMappingList.h +++ b/lldb/include/lldb/Target/PathMappingList.h @@ -72,9 +72,19 @@ public: /// \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 RemapPath(llvm::StringRef path) const; + llvm::Optional 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 86b0ad5..b660c310 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 -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 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 5c129cb9..626a653 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 @@ class TestVSCode_setBreakpoints(lldbvscode_testcase.VSCodeTestCaseBase): @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 @@ class TestVSCode_setBreakpoints(lldbvscode_testcase.VSCodeTestCaseBase): 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):