[Lldb-commits] [lldb] ca47ac3 - [source maps] Fix remove, insert-after and replace
Walter Erquinigo via lldb-commits
lldb-commits at lists.llvm.org
Fri Apr 3 19:16:30 PDT 2020
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)
More information about the lldb-commits
mailing list