-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[libcxx] fix mi_mode_test failure with libc++-21 #153969
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-libcxx Author: Sv. Lockal (AngryLoki) ChangesThis test attempts to compare JSON-serialized objects produced by the GDB pretty printer. While it happened to work in libstdc++ and libc++-20, it fails in libc++-21 because The test now compares sorted lists for Full diff: https://github.com/llvm/llvm-project/pull/153969.diff 2 Files Affected:
diff --git a/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.py b/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.py
index da09092b690c4..30d669e5b00ca 100644
--- a/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.py
+++ b/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.py
@@ -45,7 +45,8 @@ def invoke(self, arg, from_tty):
# Stack frame is:
# 0. StopForDebugger
- # 1. CompareListChildrenToChars, ComparePrettyPrintToChars or ComparePrettyPrintToRegex
+ # 1. CompareListChildrenToChars, CompareListChildrenSortedToChars,
+ # ComparePrettyPrintToChars or ComparePrettyPrintToRegex
# 2. TestCase
compare_frame = gdb.newest_frame().older()
testcase_frame = compare_frame.older()
@@ -57,7 +58,9 @@ def invoke(self, arg, from_tty):
frame_name = compare_frame.name()
if frame_name.startswith("CompareListChildren"):
if has_execute_mi:
- value = self._get_children(compare_frame)
+ value = self._get_children(
+ compare_frame, with_sorting="Sorted" in frame_name
+ )
else:
print("SKIPPED: " + test_loc_str)
return
@@ -91,7 +94,7 @@ def invoke(self, arg, from_tty):
print(str(e))
test_failures += 1
- def _get_children(self, compare_frame):
+ def _get_children(self, compare_frame, *, with_sorting=False):
compare_frame.select()
gdb.execute_mi("-var-create", "value", "*", "value")
r = gdb.execute_mi("-var-list-children", "--simple-values", "value")
@@ -105,8 +108,12 @@ def _get_children(self, compare_frame):
}
for i in range(len(children) // 2)
]
+ if with_sorting:
+ r.sort(key=lambda item: item["key"])
else:
r = [json.loads(el["value"]) for el in children]
+ if with_sorting:
+ r.sort()
return json.dumps(r, sort_keys=True)
def _get_value(self, compare_frame, testcase_frame):
diff --git a/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp b/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp
index f5a878582666b..fe2fcec0c0a9e 100644
--- a/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp
+++ b/libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp
@@ -136,6 +136,12 @@ void CompareListChildrenToChars(TypeToPrint value, const char* expectation) {
StopForDebugger(&value, &expectation);
}
+template <typename TypeToPrint>
+void CompareListChildrenSortedToChars(TypeToPrint value, const char* expectation) {
+ MarkAsLive(value);
+ StopForDebugger(&value, &expectation);
+}
+
namespace example {
struct example_struct {
int a = 0;
@@ -672,19 +678,19 @@ void streampos_test() {
}
void mi_mode_test() {
- std::map<int, std::string> one_two_three_map;
- one_two_three_map.insert({1, "one"});
- one_two_three_map.insert({2, "two"});
- one_two_three_map.insert({3, "three"});
+ std::set<std::string> one_two_three_set{"3", "2", "1"};
+ CompareListChildrenToChars(one_two_three_set, R"(["1", "2", "3"])");
+
+ std::unordered_set<std::string> one_two_three_uset{"3", "2", "1"};
+ CompareListChildrenSortedToChars(one_two_three_uset, R"(["1", "2", "3"])");
+
+ std::map<int, std::string> one_two_three_map{{3, "three"}, {2, "two"}, {1, "one"}};
CompareListChildrenToChars(
one_two_three_map, R"([{"key": 1, "value": "one"}, {"key": 2, "value": "two"}, {"key": 3, "value": "three"}])");
- std::unordered_map<int, std::string> one_two_three_umap;
- one_two_three_umap.insert({3, "three"});
- one_two_three_umap.insert({2, "two"});
- one_two_three_umap.insert({1, "one"});
- CompareListChildrenToChars(
- one_two_three_umap, R"([{"key": 3, "value": "three"}, {"key": 2, "value": "two"}, {"key": 1, "value": "one"}])");
+ std::unordered_map<int, std::string> one_two_three_umap{{3, "three"}, {2, "two"}, {1, "one"}};
+ CompareListChildrenSortedToChars(
+ one_two_three_umap, R"([{"key": 1, "value": "one"}, {"key": 2, "value": "two"}, {"key": 3, "value": "three"}])");
std::deque<int> one_two_three_deque{1, 2, 3};
CompareListChildrenToChars(one_two_three_deque, "[1, 2, 3]");
|
b4097e3
to
fa46bfa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's more important to print the elements in the order they are actually iterated than having a super robust test, so I don't think it's a good idea to sort the elements in the unordered map before printing. We want to stay faithful to what is actually in memory.
I'm sorry but I don't really comprehend what you're suggesting here. |
I'm saying that we shouldn't sort the elements, since that's not what's in the container. You wouldn't expect your |
So how do you suggest that we fix the test failure? Remove the test entirely? |
We should (1) make sure that the order we print is the same as the actual container's order and (2) update the order the test is checking if it's wrong or update the pretty printer to reflect what is in the container. I want that the gdb tests ensure that we're actually printing what we expect, not that we print a permutation of what we expect. |
But the container is a |
We're testing a pretty printer for libc++. How would that ever not be libc++-specific? I don't think the order changes depending on the platform for us, so that's not a problem. |
Well, #153940 is the failure I'm hitting with libc++. It either means that the output does change per platform, or the test isn't covered by buildbots. |
I guess I understand the idea. Edit: to recheck if this test is not skipped (one known skip condition is gdb<14.2), that the minimal version with execute_mi. Edit 2: CI uses Ubuntu 22.04 with gdb 12.1-0 (unless I missed some line), that's why that issue was never caught by CI. |
fa46bfa
to
e70ad41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm incredibly confused. Why is there a serializer and parser now? I feel like I'm missing something here, or maybe we're talking past each other. AFAICT our implementation should print [{"key": 1, "value": "one"}, {"key": 2, "value": "two"}, {"key": 3, "value": "three"}]
for the old test, which was already tested before this patch. This implies to me that the pretty printer is incorrect if this test fails.
384b5ce
to
d4675f9
Compare
Update the reference string to use the same order as in current `std::unordered_map` implementation. The issue was never detected by CI, because related test used `gdb.execute_mi` method, available inly since GDB 14.2 (while CI use GDB 12). Now test provides a polyfill for older GDB releases. Closes llvm#153940
d4675f9
to
4d0fcdf
Compare
Uhh, sorry for the mess. For some reason I was sure that during initial implementation Regarding parser: I added it so that CI with old GDB actually covers the scenario. Without this change CI skips that part of test due to missing |
Update the reference string to use the same order as in current
std::unordered_map
implementation.The issue was never detected by CI, because related test used
gdb.execute_mi
method, available inly since GDB 14.2 (while CI use GDB 12).Now test provides a polyfill for older GDB releases.
Closes #153940