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

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.

Reviewers: labath, clayborg

Reviewed By: labath

Subscribers: mgrang, lldb-commits

Tags: #lldb

Differential Revision: https://reviews.llvm.org/D77324

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) {
+        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)) {
@@ -70,14 +70,12 @@ 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(
                 "%sthe replacement path doesn't exist: \"%s\"",
                 previousError.c_str(), replace_path);
-            break;
           }
         }
         if (changed)
@@ -156,7 +154,6 @@ Status OptionValuePathMappings::SetValueFromString(llvm::StringRef value,
             error.SetErrorStringWithFormat(
                 "%sthe replacement path doesn't exist: \"%s\"",
                 previousError.c_str(), replace_path);
-            break;
           }
         }
         if (changed)
@@ -171,32 +168,23 @@ Status OptionValuePathMappings::SetValueFromString(llvm::StringRef value,
   case eVarSetOperationRemove:
     if (argc > 0) {
       std::vector<int> remove_indexes;
-      bool all_indexes_valid = true;
+      for (size_t i = 0; i < argc; ++i) {
-      size_t i;
+        int idx =
-      for (i = 0; all_indexes_valid && i < argc; ++i) {
-        const int idx =
             StringConvert::ToSInt32(args.GetArgumentAtIndex(i), INT32_MAX);
-        if (idx == INT32_MAX)
+        if (idx < 0 || idx >= (int)m_path_mappings.GetSize()) {
-          all_indexes_valid = false;
+          error.SetErrorStringWithFormat(
-        else
+              "invalid array index '%s', aborting remove operation",
+              args.GetArgumentAtIndex(i));
+          break;
+        } else
           remove_indexes.push_back(idx);
       }
 
-      if (all_indexes_valid) {
+      // Sort and then erase in reverse so indexes are always valid
-        size_t num_remove_indexes = remove_indexes.size();
+      llvm::sort(remove_indexes.begin(), remove_indexes.end());
-        if (num_remove_indexes) {
+      for (auto index : llvm::reverse(remove_indexes))
-          // Sort and then erase in reverse so indexes are always valid
+        m_path_mappings.Remove(index, m_notify_changes);
-          llvm::sort(remove_indexes.begin(), remove_indexes.end());
+      NotifyValueChanged();
-          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 {
       error.SetErrorString("remove operation takes one or more array index");
     }

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

@@ -42,67 +42,93 @@ class TestTargetSourceMap(TestBase):
         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),
+            'settings replace target.source-map 0 . "%s" . "%s"' % (invalid_path, valid_path2),
-            substrs=['error: the replacement path doesn\'t exist: "%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)]
+            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),
+            'settings insert-after target.source-map 0 . "%s" . "%s" . "%s"' \
-            substrs=['error: the replacement path doesn\'t exist: "%s"' % (invalid_path)],
+                % (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
+        # Let's clear using remove and add the mapping in with append
-        self.expect('settings remove target.source-map 0')
+        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),
+            '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)],
+            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)