Skip to content

Conversation

AngryLoki
Copy link
Contributor

@AngryLoki AngryLoki commented Aug 16, 2025

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

@AngryLoki AngryLoki requested a review from a team as a code owner August 16, 2025 17:59
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 16, 2025
@llvmbot
Copy link
Member

llvmbot commented Aug 16, 2025

@llvm/pr-subscribers-libcxx

Author: Sv. Lockal (AngryLoki)

Changes

This 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 std::unordered_map does not guarantee ordering.

The test now compares sorted lists for std::unordered_map and adds subtests for std::set/std::unordered_set.


Full diff: https://github.com/llvm/llvm-project/pull/153969.diff

2 Files Affected:

  • (modified) libcxx/test/libcxx/gdb/gdb_pretty_printer_test.py (+10-3)
  • (modified) libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp (+16-10)
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]");

@AngryLoki AngryLoki force-pushed the unordered-mi-gdb-test branch from b4097e3 to fa46bfa Compare August 16, 2025 18:01
Copy link
Contributor

@philnik777 philnik777 left a 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.

@mgorny
Copy link
Member

mgorny commented Aug 21, 2025

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.

@philnik777
Copy link
Contributor

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 vector to get sorted either.

@mgorny
Copy link
Member

mgorny commented Aug 21, 2025

So how do you suggest that we fix the test failure? Remove the test entirely?

@philnik777
Copy link
Contributor

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.

@mgorny
Copy link
Member

mgorny commented Aug 21, 2025

But the container is a std::unordered_map, so the order is undefined and depends on implementation. I suppose it could technically depend on the platform as well.

@philnik777
Copy link
Contributor

But the container is a std::unordered_map, so the order is undefined and depends on implementation. I suppose it could technically depend on the platform as well.

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.

@mgorny
Copy link
Member

mgorny commented Aug 21, 2025

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.

@AngryLoki
Copy link
Contributor Author

AngryLoki commented Aug 21, 2025

I guess I understand the idea.
Test subapplication (built with some stdlib) has std::unordered_{map,set} hashmap.
GDB provides its dump (prints buckets in order they are filled). Then test main application (built with the same stdlib) also creates std::unordered_{map,set}, iterates through the buckets. As long as hash table does not use hash randomization (as in Python, which has hash randomization), the order within the same stdlib (exactly the same version) should be the same.
I'll update this PR tomorrow.

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.

@AngryLoki AngryLoki force-pushed the unordered-mi-gdb-test branch from fa46bfa to e70ad41 Compare August 22, 2025 19:48
@mgorny mgorny requested a review from philnik777 August 26, 2025 05:33
Copy link
Contributor

@philnik777 philnik777 left a 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.

@AngryLoki AngryLoki force-pushed the unordered-mi-gdb-test branch from 384b5ce to d4675f9 Compare August 26, 2025 09:49
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
@AngryLoki AngryLoki force-pushed the unordered-mi-gdb-test branch from d4675f9 to 4d0fcdf Compare August 26, 2025 09:52
@AngryLoki
Copy link
Contributor Author

AngryLoki commented Aug 26, 2025

Uhh, sorry for the mess. For some reason I was sure that during initial implementation std::unordered_map dump was 3-2-1, not 1-2-3. But I an not able to reproduce this now (on godbolt or my system): all libc++ versions output 1-2-3. No idea what was that: either I mistested it in initial implementation (switched to old gdb?) or I'm missing something else (can hardening flags change unordered_map behavior? From godbolt looks like no).

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 gdb.execute_mi function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[libcxx] libcxx/gdb/gdb_pretty_printer_test.sh.cpp failure
4 participants