[source maps] Ensure all valid source maps are added instead of failing with the...

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

diff --git a/lldb/source/Interpreter/OptionValuePathMappings.cpp b/lldb/source/Interpreter/OptionValuePathMappings.cpp
index ebff5c4..2784279 100644 (file)
--- 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 ac03d80..c9800e6 100644 (file)
--- 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 @@ class TestTargetSourceMap(TestBase):
         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)