Author: Walter Erquinigo Date: 2020-04-03T19:15:56-07:00 New Revision: ca47ac3d5f6f8483d330c96a63f1cd862e667856
URL: https://github.com/llvm/llvm-project/commit/ca47ac3d5f6f8483d330c96a63f1cd862e667856 DIFF: https://github.com/llvm/llvm-project/commit/ca47ac3d5f6f8483d330c96a63f1cd862e667856.diff LOG: [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 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 2784279579f0..3b3f43d07293 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) { + 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; - 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)); - } + // Sort and then erase in reverse so indexes are always valid + llvm::sort(remove_indexes.begin(), remove_indexes.end()); + for (auto index : llvm::reverse(remove_indexes)) + m_path_mappings.Remove(index, m_notify_changes); + NotifyValueChanged(); } else { error.SetErrorString("remove operation takes one or more array index"); } diff --git a/lldb/test/API/functionalities/source-map/TestTargetSourceMap.py b/lldb/test/API/functionalities/source-map/TestTargetSourceMap.py index c9800e6f199e..6457c766813e 100644 --- a/lldb/test/API/functionalities/source-map/TestTargetSourceMap.py +++ b/lldb/test/API/functionalities/source-map/TestTargetSourceMap.py @@ -42,67 +42,93 @@ def assertBreakpointWithSourceMap(src_path): 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) _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
