Author: Walter Erquinigo Date: 2020-04-01T13:01:40-07:00 New Revision: 30350c254106ce25b16b13f05e713ceb2b15ce09
URL: https://github.com/llvm/llvm-project/commit/30350c254106ce25b16b13f05e713ceb2b15ce09 DIFF: https://github.com/llvm/llvm-project/commit/30350c254106ce25b16b13f05e713ceb2b15ce09.diff LOG: [source maps] Ensure all valid source maps are added instead of failing with the first invalid one Summary: Several lldb-vscode users have noticed that when a source map rule is invalid (because a folder doesn't exist anymore), the rest of the source maps from their configurations are not applied. This happens because lldb-vscode executes a single "settings set target.source-map" command with all the source maps and LLDB processes them one by one until one fails. Instead of doing this, we can process in LLDB all the source map rules and apply the valid ones instead of failing fast. Reviewers: clayborg, labath, kusmour, aadsm Subscribers: lldb-commits Tags: #lldb Differential Revision: https://reviews.llvm.org/D77186 Added: Modified: lldb/source/Interpreter/OptionValuePathMappings.cpp lldb/test/API/functionalities/source-map/TestTargetSourceMap.py Removed: ################################################################################ diff --git a/lldb/source/Interpreter/OptionValuePathMappings.cpp b/lldb/source/Interpreter/OptionValuePathMappings.cpp index ebff5c4dca3e..2784279579f0 100644 --- a/lldb/source/Interpreter/OptionValuePathMappings.cpp +++ b/lldb/source/Interpreter/OptionValuePathMappings.cpp @@ -61,7 +61,7 @@ Status OptionValuePathMappings::SetValueFromString(llvm::StringRef value, count); } else { bool changed = false; - for (size_t i = 1; i < argc; i += 2, ++idx) { + for (size_t i = 1; i < argc; i += 2) { const char *orginal_path = args.GetArgumentAtIndex(i); const char *replace_path = args.GetArgumentAtIndex(i + 1); if (VerifyPathExists(replace_path)) { @@ -70,9 +70,13 @@ Status OptionValuePathMappings::SetValueFromString(llvm::StringRef value, if (!m_path_mappings.Replace(a, b, idx, m_notify_changes)) m_path_mappings.Append(a, b, m_notify_changes); changed = true; + idx++; } else { + std::string previousError = + error.Fail() ? std::string(error.AsCString()) + "\n" : ""; error.SetErrorStringWithFormat( - "the replacement path doesn't exist: \"%s\"", replace_path); + "%sthe replacement path doesn't exist: \"%s\"", + previousError.c_str(), replace_path); break; } } @@ -109,9 +113,11 @@ Status OptionValuePathMappings::SetValueFromString(llvm::StringRef value, m_value_was_set = true; changed = true; } else { + std::string previousError = + error.Fail() ? std::string(error.AsCString()) + "\n" : ""; error.SetErrorStringWithFormat( - "the replacement path doesn't exist: \"%s\"", replace_path); - break; + "%sthe replacement path doesn't exist: \"%s\"", + previousError.c_str(), replace_path); } } if (changed) @@ -135,7 +141,7 @@ Status OptionValuePathMappings::SetValueFromString(llvm::StringRef value, bool changed = false; if (op == eVarSetOperationInsertAfter) ++idx; - for (size_t i = 1; i < argc; i += 2, ++idx) { + for (size_t i = 1; i < argc; i += 2) { const char *orginal_path = args.GetArgumentAtIndex(i); const char *replace_path = args.GetArgumentAtIndex(i + 1); if (VerifyPathExists(replace_path)) { @@ -143,9 +149,13 @@ Status OptionValuePathMappings::SetValueFromString(llvm::StringRef value, ConstString b(replace_path); m_path_mappings.Insert(a, b, idx, m_notify_changes); changed = true; + idx++; } else { + std::string previousError = + error.Fail() ? std::string(error.AsCString()) + "\n" : ""; error.SetErrorStringWithFormat( - "the replacement path doesn't exist: \"%s\"", replace_path); + "%sthe replacement path doesn't exist: \"%s\"", + previousError.c_str(), replace_path); break; } } diff --git a/lldb/test/API/functionalities/source-map/TestTargetSourceMap.py b/lldb/test/API/functionalities/source-map/TestTargetSourceMap.py index ac03d80023e4..c9800e6f199e 100644 --- a/lldb/test/API/functionalities/source-map/TestTargetSourceMap.py +++ b/lldb/test/API/functionalities/source-map/TestTargetSourceMap.py @@ -1,6 +1,7 @@ import lldb from lldbsuite.test.lldbtest import * from lldbsuite.test.decorators import * +import os class TestTargetSourceMap(TestBase): @@ -10,6 +11,21 @@ class TestTargetSourceMap(TestBase): @no_debug_info_test def test_source_map(self): """Test target.source-map' functionality.""" + + def assertBreakpointWithSourceMap(src_path): + # Set a breakpoint after we remap source and verify that it succeeds + bp = target.BreakpointCreateByLocation(src_path, 2) + self.assertEquals(bp.GetNumLocations(), 1, + "make sure breakpoint was resolved with map") + + # Now make sure that we can actually FIND the source file using this + # remapping: + retval = lldb.SBCommandReturnObject() + self.dbg.GetCommandInterpreter().HandleCommand("source list -f main.c -l 2", retval) + self.assertTrue(retval.Succeeded(), "source list didn't succeed.") + self.assertNotEqual(retval.GetOutput(), None, "We got no ouput from source list") + self.assertTrue("return" in retval.GetOutput(), "We didn't find the source file...") + # Set the target soure map to map "./" to the current test directory src_dir = self.getSourceDir() src_path = os.path.join(src_dir, "main.c") @@ -25,19 +41,68 @@ def test_source_map(self): bp = target.BreakpointCreateByLocation(src_path, 2) self.assertEquals(bp.GetNumLocations(), 0, "make sure no breakpoints were resolved without map") - src_map_cmd = 'settings set target.source-map . "%s"' % (src_dir) - self.dbg.HandleCommand(src_map_cmd) - # Set a breakpoint after we remap source and verify that it succeeds - bp = target.BreakpointCreateByLocation(src_path, 2) - self.assertEquals(bp.GetNumLocations(), 1, - "make sure breakpoint was resolved with map") - - # Now make sure that we can actually FIND the source file using this - # remapping: - retval = lldb.SBCommandReturnObject() - self.dbg.GetCommandInterpreter().HandleCommand("source list -f main.c -l 2", retval) - self.assertTrue(retval.Succeeded(), "source list didn't succeed.") - self.assertNotEqual(retval.GetOutput(), None, "We got no ouput from source list") - self.assertTrue("return" in retval.GetOutput(), "We didn't find the source file...") + invalid_path = src_dir + "invalid_path" + invalid_path2 = src_dir + "invalid_path2" + + # We make sure the error message contains all the invalid paths + self.expect( + 'settings set target.source-map . "%s" . "%s" . "%s"' % (invalid_path, src_dir, invalid_path2), + substrs=[ + 'the replacement path doesn\'t exist: "%s"' % (invalid_path), + 'the replacement path doesn\'t exist: "%s"' % (invalid_path2), + ], + error=True, + ) + self.expect( + 'settings show target.source-map', + substrs=['[0] "." -> "%s"' % (src_dir)], + ) + assertBreakpointWithSourceMap(src_path) + + # Index 0 is the valid mapping, and modifying it to an invalid one should have no effect + self.expect( + 'settings replace target.source-map 0 . "%s"' % (invalid_path), + substrs=['error: the replacement path doesn\'t exist: "%s"' % (invalid_path)], + error=True, + ) + self.expect( + 'settings show target.source-map', + substrs=['[0] "." -> "%s"' % (src_dir)] + ) + assertBreakpointWithSourceMap(src_path) + + # Let's clear and add the mapping in with insert-after + self.runCmd('settings remove target.source-map 0') + self.expect( + 'settings show target.source-map', + endstr="target.source-map (path-map) =\n", + ) + # We add a valid but useless mapping so that we can use insert-after + another_valid_path = os.path.dirname(src_dir) + self.runCmd('settings set target.source-map . "%s"' % (another_valid_path)) + + self.expect( + 'settings replace target.source-map 0 . "%s"' % (invalid_path), + substrs=['error: the replacement path doesn\'t exist: "%s"' % (invalid_path)], + error=True, + ) + self.expect( + 'settings show target.source-map', + substrs=['[0] "." -> "%s"' % (another_valid_path)] + ) + + # Let's clear and add the mapping in with append + self.expect('settings remove target.source-map 0') + self.expect( + 'settings show target.source-map', + endstr="target.source-map (path-map) =\n", + ) + + self.expect( + 'settings append target.source-map . "%s" . "%s"' % (invalid_path, src_dir), + substrs=['error: the replacement path doesn\'t exist: "%s"' % (invalid_path)], + error=True, + ) + assertBreakpointWithSourceMap(src_path) _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
