[source maps] Ensure all valid source maps are added instead of failing with the first invalid one

[wallace]

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.

[clayborg]

This will do what the user intends more of the time. Good catch

[labath]

It seems somewhat odd for a command to return error (one of the effects of that for instance is to abort processing of batch scripts), but still perform some changes. It might be more appropriate to call those warnings. However, reporting warnings from here would require some plumbing, and the new behavior is definitely more reasonable than the old one, so I don't think this patch should be blocked on that.

lldb/source/Interpreter/OptionValuePathMappings.cpp

73

does this actually change anything?

118–120

If there are multiple non-existing paths, this will just return the last one right? We should list all of them.

152

does this change anything?

lldb/test/API/functionalities/source-map/TestTargetSourceMap.py

43–44

It would be good to follow this up with a settings show, both to test/document the expected final value of the setting, and also to catch any issues early.

[wallace]

lldb/source/Interpreter/OptionValuePathMappings.cpp

73

Indeed. It's used in line 70 as the index to replace in the replace operation

118–120

good catch

152

Same as above, it's used in line 144

lldb/test/API/functionalities/source-map/TestTargetSourceMap.py

43–44

good one

[wallace]

address comments

[MaskRay]

In D77186#1955451, @wallace wrote:

address comments

@wallace labath made comments after the differential had been approved. So it looks like he may have further questions. It was probably better waiting for an explicit ack instead of being in a haste.

[wallace]

Thanks for the heads up. I think I got a false impression of how comments after accept vs requesting changes work in this repo.

[labath]

In D77186#1955640, @wallace wrote:

Thanks for the heads up. I think I got a false impression of how comments after accept vs requesting changes work in this repo.

The rules on that are somewhat fuzzy, but there has been a recent effort to formalize that. Generally, in a situation like this (where a patch already got one LGTM and so is "green"), I would have used "request changes" if I had major objections and was sure I want to see the patch again. If I only had cosmetic comments/trivial comments, I would have clicked "approve" and trusted you to apply them before committing. Not doing either of those is a sort of a "meh" position, where I'm uncomfortable lgtm-ing something, but I also don't want to reject it (as that's fairly aggressive). In this case, I don't think you have done anything really wrong strictly speaking, but it would have been polite to wait a while so that I have a chance to respond.

lldb/source/Interpreter/OptionValuePathMappings.cpp

73

Yes, but that would have also worked (I think) if you had left the increment in the for loop. My question was why did you need to move the increment into the loop body.

lldb/source/Interpreter/OptionValuePathMappings.cpp

if (argc >= 3 && (((argc - 1) & 1) == 0)) {
           StringConvert::ToUInt32(args.GetArgumentAtIndex(0), UINT32_MAX);
       const uint32_t count = m_path_mappings.GetSize();
       if (idx > count) {
         error.SetErrorStringWithFormat(
             "invalid file list index %u, index must be 0 through %u", idx,
             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)) {
             ConstString a(orginal_path);
             ConstString b(replace_path);
             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;
           }
         }
         if (changed)
           NotifyValueChanged();
       }
     } else {
       error.SetErrorString("replace operation takes an array index followed by "
if (argc < 2 || (argc & 1)) {
         const char *replace_path = args.GetArgumentAtIndex(i + 1);
         if (VerifyPathExists(replace_path)) {
           ConstString a(orginal_path);
           ConstString b(replace_path);
           m_path_mappings.Append(a, b, m_notify_changes);
           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)
         NotifyValueChanged();
     }
     break;
 
   case eVarSetOperationInsertBefore:
   case eVarSetOperationInsertAfter:
     // Must be at least one index + 1 pair of paths, and the pair count must be
     // even
     if (argc >= 3 && (((argc - 1) & 1) == 0)) {
       uint32_t idx =
           StringConvert::ToUInt32(args.GetArgumentAtIndex(0), UINT32_MAX);
       const uint32_t count = m_path_mappings.GetSize();
       if (idx > count) {
         error.SetErrorStringWithFormat(
             "invalid file list index %u, index must be 0 through %u", idx,
             count);
       } else {
         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)) {
             ConstString a(orginal_path);
             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;
           }
         }
         if (changed)
           NotifyValueChanged();
       }
     } else {
       error.SetErrorString("insert operation takes an array index followed by "

lldb/test/API/functionalities/source-map/TestTargetSourceMap.py

 import lldb
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test.decorators import *
+import os
 
 
 class TestTargetSourceMap(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     @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")
         yaml_path = os.path.join(src_dir, "a.yaml")
         yaml_base, ext = os.path.splitext(yaml_path)
         obj_path = self.getBuildArtifact("main.o")
         self.yaml2obj(yaml_path, obj_path)
 
         # Create a target with the object file we just created from YAML
         target = self.dbg.CreateTarget(obj_path)
 
         # Set a breakpoint before we remap source and verify that it fails
         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)