wallace created this revision.
wallace added reviewers: labath, clayborg.
Herald added subscribers: lldb-commits, mgrang.
Herald added a project: LLDB.
wallace updated this revision to Diff 254576.
wallace added a comment.
fix typos
I think the quarantine is reducing my IQ overall...
In this diff of mine D77186 <https://reviews.llvm.org/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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D77324
Files:
lldb/source/Interpreter/OptionValuePathMappings.cpp
lldb/test/API/functionalities/source-map/TestTargetSourceMap.py
Index: lldb/test/API/functionalities/source-map/TestTargetSourceMap.py
===================================================================
--- lldb/test/API/functionalities/source-map/TestTargetSourceMap.py
+++ lldb/test/API/functionalities/source-map/TestTargetSourceMap.py
@@ -42,67 +42,93 @@
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)
Index: lldb/source/Interpreter/OptionValuePathMappings.cpp
===================================================================
--- lldb/source/Interpreter/OptionValuePathMappings.cpp
+++ lldb/source/Interpreter/OptionValuePathMappings.cpp
@@ -61,7 +61,7 @@
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 @@
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 @@
error.SetErrorStringWithFormat(
"%sthe replacement path doesn't exist: \"%s\"",
previousError.c_str(), replace_path);
- break;
}
}
if (changed)
@@ -171,32 +168,23 @@
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.rbegin(), remove_indexes.rend());
+ for (auto index : remove_indexes)
+ m_path_mappings.Remove(index, m_notify_changes);
+ NotifyValueChanged();
} else {
error.SetErrorString("remove operation takes one or more array index");
}
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
