[source maps] Fix remove, insert-after and replace

[wallace]

Summary

In this diff of mine D77186 I introduce a bug in the replace operation, where I was failing fast by mistake.
Besides, a similar problem existed in the insert-after operation, where it was failing fast.

Finally, the remove operation was wrong, as it was not using the indices provided by the users.

I fixed those issues and added some tests account for cases with multiple elements in these requests.

[wallace]

fix typos

I think the quarantine is reducing my IQ overall...

[labath]

lldb/source/Interpreter/OptionValuePathMappings.cpp

184–185

sorting on reverse iterators causes some head-scratching. I think it would be simpler to just sort normally, and then iterate in reverse (for (int /*not auto*/ index: llvm::reverse(remove_indexes)) ...)

[wallace]

address comments`

lldb/source/Interpreter/OptionValuePathMappings.cpp

 //===-- OptionValuePathMappings.cpp ---------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
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) {
+        for (size_t i = 1; i < argc; idx++, 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(
                 "%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 "
                            "one or more path pairs");
if (argc >= 3 && (((argc - 1) & 1) == 0)) {
             changed = true;
             idx++;
           } else {
             std::string previousError =
                 error.Fail() ? std::string(error.AsCString()) + "\n" : "";
             error.SetErrorStringWithFormat(
                 "%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 "
                            "one or more path pairs");
     }
     break;
 
   case eVarSetOperationRemove:
     if (argc > 0) {
       std::vector<int> remove_indexes;
-      bool all_indexes_valid = true;
-      size_t i;
-      for (i = 0; all_indexes_valid && i < argc; ++i) {
-        const int idx =
+      for (size_t i = 0; i < argc; ++i) {
+        int idx =
             StringConvert::ToSInt32(args.GetArgumentAtIndex(i), INT32_MAX);
-        if (idx == INT32_MAX)
-          all_indexes_valid = false;
-        else
+        if (idx < 0 || idx >= (int)m_path_mappings.GetSize()) {
+          error.SetErrorStringWithFormat(
+              "invalid array index '%s', aborting remove operation",
+              args.GetArgumentAtIndex(i));
+          break;
+        } else
           remove_indexes.push_back(idx);
       }
 
-      if (all_indexes_valid) {
-        size_t num_remove_indexes = remove_indexes.size();
-        if (num_remove_indexes) {
-          // Sort and then erase in reverse so indexes are always valid
-          llvm::sort(remove_indexes.begin(), remove_indexes.end());
-          for (size_t j = num_remove_indexes - 1; j < num_remove_indexes; ++j) {
-            m_path_mappings.Remove(j, m_notify_changes);
-          }
-        }
-        NotifyValueChanged();
-      } else {
-        error.SetErrorStringWithFormat(
-            "invalid array index '%s', aborting remove operation",
-            args.GetArgumentAtIndex(i));
-      }
-    } else {
+      llvm::sort(remove_indexes.rbegin(), remove_indexes.rend());
+      for (auto index : remove_indexes)
+        m_path_mappings.Remove(index, m_notify_changes);
       error.SetErrorString("remove operation takes one or more array index");
     }
     break;
 
   case eVarSetOperationInvalid:
     error = OptionValue::SetValueFromString(value, op);
     break;
   }
   return error;
 }
 
 lldb::OptionValueSP OptionValuePathMappings::DeepCopy() const {
   return OptionValueSP(new OptionValuePathMappings(*this));
 }

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

def test_source_map(self):
         # 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")
 
+        valid_path = os.path.dirname(src_dir)
+        valid_path2 = os.path.dirname(valid_path)
         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),
+            'settings set target.source-map . "%s" . "%s" . "%s" . "%s' \
+                % (invalid_path, src_dir, invalid_path2, valid_path),
             substrs=[
-                'the replacement path doesn\'t exist: "%s"' % (invalid_path),
+                'error: 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)],
+            substrs=[
+                '[0] "." -> "%s"' % (src_dir),
+                '[1] "." -> "%s"' % (valid_path),
+            ],
         )
         assertBreakpointWithSourceMap(src_path)
 
-        # Index 0 is the valid mapping, and modifying it to an invalid one should have no effect
+        # Attempts to replace an index to an invalid mapping should have no effect.
+        # Modifications to valid mappings should work.
         self.expect(
-            'settings replace target.source-map 0 . "%s"' % (invalid_path),
-            substrs=['error: the replacement path doesn\'t exist: "%s"' % (invalid_path)],
+            'settings replace target.source-map 0 . "%s" . "%s"' % (invalid_path, valid_path2),
+            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)]
+            substrs=[
+                '[0] "." -> "%s"' % (src_dir),
+                '[1] "." -> "%s"' % (valid_path2),
+            ]
         )
         assertBreakpointWithSourceMap(src_path)
 
-        # Let's clear and add the mapping in with insert-after
+        # Let's clear and add the mapping back 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",
+            substrs=['[0] "." -> "%s"' % (valid_path2)],
         )
-        
-        # 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)],
+            'settings insert-after target.source-map 0 . "%s" . "%s" . "%s"' \
+                % (invalid_path, invalid_path2, src_dir),
+            substrs=[
+                'error: 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"' % (another_valid_path)]
+            substrs=[
+                '[0] "." -> "%s"' % (valid_path2),
+                '[1] "." -> "%s"' % (src_dir),
+            ]
         )
 
-        # Let's clear and add the mapping in with append
-        self.expect('settings remove target.source-map 0')
+        # Let's clear using remove and add the mapping in with append
+        self.runCmd('settings remove target.source-map 1')
         self.expect(
             'settings show target.source-map',
-            endstr="target.source-map (path-map) =\n",
+            substrs=[
+                '[0] "." -> "%s"' % (valid_path2),
+            ]
         )
-
+        self.runCmd('settings clear target.source-map')
         self.expect(
-            'settings append target.source-map . "%s" . "%s"' % (invalid_path, src_dir),
-            substrs=['error: the replacement path doesn\'t exist: "%s"' % (invalid_path)],
+            'settings append target.source-map . "%s" . "%s" . "%s"' % (invalid_path, src_dir, invalid_path2),
+            substrs=[
+                'error: 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)