Skip to content

[lldb][DataFormatter] unordered_map: account for new libc++ __hash_node layout #68574

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

Merged
merged 4 commits into from
Oct 13, 2023

Conversation

Michael137
Copy link
Member

Since D101206 the __hash_node::__value_ member is wrapped in an anonymous union. ValueObject::GetChildMemberWithName doesn't see through the union.

This patch accounts for this possible new layout by getting a handle to the union before doing the by-name __value_ lookup.

@llvmbot
Copy link
Member

llvmbot commented Oct 9, 2023

@llvm/pr-subscribers-lldb

Changes

Since D101206 the __hash_node::__value_ member is wrapped in an anonymous union. ValueObject::GetChildMemberWithName doesn't see through the union.

This patch accounts for this possible new layout by getting a handle to the union before doing the by-name __value_ lookup.


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

1 Files Affected:

  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp (+10-2)
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
index 14776cdf808157d..03619385b7d9968 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
@@ -162,10 +162,18 @@ lldb::ValueObjectSP lldb_private::formatters::
       if (!node_sp || error.Fail())
           return nullptr;
 
-      value_sp = node_sp->GetChildMemberWithName("__value_");
       hash_sp = node_sp->GetChildMemberWithName("__hash_");
-      if (!value_sp || !hash_sp)
+      if (!hash_sp)
         return nullptr;
+
+      value_sp = node_sp->GetChildMemberWithName("__value_");
+      if (!value_sp) {
+        // Newer libc++ versions wrap the `__value_` in an anonymous union.
+        auto anon_union = node_sp->GetChildAtIndex(2);
+        value_sp = anon_union->GetChildMemberWithName("__value_");
+        if (!value_sp)
+          return nullptr;
+      }
     }
     m_elements_cache.push_back(
         {value_sp.get(), hash_sp->GetValueAsUnsigned(0)});


value_sp = node_sp->GetChildMemberWithName("__value_");
if (!value_sp) {
// Newer libc++ versions wrap the `__value_` in an anonymous union.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be useful to specify when this was introduced in libc++ in case this logic gets updated again in the future for an even newer libc++.

Copy link
Member Author

@Michael137 Michael137 Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

Let me know if latest comment provides enough context/info (since the libc++ change hasn't landed yet I used the phab ID instead of commit hash)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely better. I assume phabricator will be read-only mode for quite a while to come, so I'm not too worried about it being difficult to find. We should probably update it with a commit hash once it lands though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the commit hash

…de layout

Since D101206 (`ba79fb2e1ff7130cde02fbbd325f0f96f8a522ca`) the
`__hash_node::__value_` member is wrapped in an anonymous union.
`ValueObject::GetChildMemberWithName` doesn't see through the union.

This patch accounts for this possible new layout by getting a handle
to the union before doing the by-name `__value_` lookup.
@Michael137 Michael137 force-pushed the bugfix/lldb-map-formatter-fix branch from 8ff81e2 to 33475b1 Compare October 13, 2023 15:06
@github-actions
Copy link

github-actions bot commented Oct 13, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@Michael137 Michael137 merged commit 7493d45 into llvm:main Oct 13, 2023
Michael137 added a commit to Michael137/llvm-project that referenced this pull request May 4, 2024
…de layout (llvm#68574)

Since D101206 (`ba79fb2e1ff7130cde02fbbd325f0f96f8a522ca`) the `__hash_node::__value_`
member is wrapped in an anonymous union. `ValueObject::GetChildMemberWithName` doesn't see
through the union.

This patch accounts for this possible new layout by getting a handle to
the union before doing the by-name `__value_` lookup.

(cherry picked from commit 7493d45)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants