-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[lldb] Fix unordered-map data formatter for const types #156033
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
Conversation
@llvm/pr-subscribers-lldb Author: Ebuka Ezike (da-viper) Changestest was failing because in some version of llvm-project/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp Lines 370 to 384 in 20dd053
llvm-project/libcxx/include/__cxx03/unordered_map Lines 749 to 757 in c659a3b
Full diff: https://github.com/llvm/llvm-project/pull/156033.diff 1 Files Affected:
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered_map-iterator/TestDataFormatterStdUnorderedMap.py b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered_map-iterator/TestDataFormatterStdUnorderedMap.py
index d2382373f4810..95b0395ebc486 100644
--- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered_map-iterator/TestDataFormatterStdUnorderedMap.py
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/unordered_map-iterator/TestDataFormatterStdUnorderedMap.py
@@ -16,6 +16,12 @@ def check_ptr_or_ref(self, var_name: str):
pair = var.GetChildAtIndex(0)
self.assertTrue(pair)
+ # std::unordered_map previously stores the actual key/value pair
+ # in __hash_value_type::__cc_ (or previously __cc).
+ if pair.GetNumChildren() == 1:
+ pair = pair.GetChildAtIndex(0)
+ self.assertTrue(pair)
+
self.assertEqual(pair.GetChildAtIndex(0).summary, '"Hello"')
self.assertEqual(pair.GetChildAtIndex(1).summary, '"World"')
@@ -29,6 +35,12 @@ def check_ptr_ptr(self, var_name: str):
pair = ptr.GetChildAtIndex(0)
self.assertTrue(pair)
+ # std::unordered_map previously stores the actual key/value pair
+ # in __hash_value_type::__cc_ (or previously __cc).
+ if pair.GetNumChildren() == 1:
+ pair = pair.GetChildAtIndex(0)
+ self.assertTrue(pair)
+
self.assertEqual(pair.GetChildAtIndex(0).summary, '"Hello"')
self.assertEqual(pair.GetChildAtIndex(1).summary, '"World"')
@@ -113,7 +125,6 @@ def do_test_ptr(self):
Test that pointers to std::unordered_map are formatted correctly.
"""
- self.build()
(self.target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
self, "Stop here", lldb.SBFileSpec("main.cpp", False)
)
|
Can you point me to the buildbot that this is failing on? We should probably fix the formatter to support the layout, as opposed to fixing up the test |
There is no build bot failure for this yet. The formatter works fine if it is used from |
this happens on ubuntu 25.10 and fedora 42
|
Hmmm that's surprising. @jimingham might know off the top. I'll have a look at what's going on here. It'd be nice if we didn't have to adjust the test itself |
Whether to prefer synthetic values for a ValueObject is controlled by the general setting |
But also note that if the synthetic child creation fails, then we will silently fall back to handing out the "real" children. So this one looks more like synthetic child generation is failing for some reason. |
The script code is not an exact equivalent of the "frame var" expression. Notice how in the "frame var", you explicitly dereference the object, while in the script, you call GetChildAtIndex directly on the pointer value. The "frame var" expression would be more similar to That doesn't quite explain why is this failing, it might give us a clue about what could be happening. I suspect the problem here is that the map data formatter is just misbehaving when given a pointer value, and the implementation happens to end up returning the parent of the actual pair value. According to grep the |
@da-viper since you can repro this issue locally, mind confirming @labath's theory (#156033 (comment))? |
This works in this case. >>> lldb.frame.FindVariable("ptr1").Dereference().GetChildAtIndex(0)
(std::pair<const std::basic_string<char>, std::basic_string<char> >) [0] = (first = "Hello", second = "World") |
What I'm a bit confused about is why the map even contains a |
Maybe because the binary is built in c++03 mode, so it uses the c++03 implementation of the map? The c++ library doesn't have to be old, the member is still present at HEAD |
Ah right yes. If it's built explicitly in c++03 in our test-suite that would also be confusing to me |
Ok so I looked at this with @da-viper offline. This is an existing issue with formatting specifically
The So TL;DR, this is not a regression. This just didn't work prior to #143501, but we never noticed cause there was no test for it. It would still be interesting to know why this fails when using older libc++. I think @da-viper is investigating further why our unordered_map formatter breaks on const pointers. A separate question, should we allow running the libc++ API test category without a locally built libc++ in the first place? |
ca31a05
to
134d45f
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
It was failing because the |
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.
Can we do this in a separate PR?
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.
LGTM
Could you update the PR description and title before merging? Particularly, this will close #146040
Unfortunately won't have a good way of testing this but our support for older layouts is best effort. I once tried writing a libcxx-simulator
test for std::map
, but that still required us to reach into libc++ internals, which wasn't very robust. So decided not to test the old layouts that way.
the type that is checked in isUnordered may be const qualified.
12c872a
to
09ad3b0
Compare
the type that is checked in isUnordered may be const qualified.
…lang versions Fixed in #156033
…for older Clang versions Fixed in llvm/llvm-project#156033
…lang versions Fixed in llvm#156033
test was failing because in some version of
libc++
there is an extra struct on the stored pair in map. And it did not use the synthetic child.llvm-project/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
Lines 370 to 384 in 20dd053
llvm-project/libcxx/include/__cxx03/unordered_map
Lines 749 to 757 in c659a3b