[lldb][DataFormatter] Remove support for old std::map layout #97549
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesWe currently supported the layout from pre-2016 (before the layout Being backward compatible would be great but we have no tests that Full diff: https://github.com/llvm/llvm-project/pull/97549.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp
index 44fe294ced722..7eb6a3637acd2 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp
@@ -264,39 +264,33 @@ void lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::GetValueOffset(
if (!node)
return;
CompilerType node_type(node->GetCompilerType());
- uint64_t bit_offset;
- if (node_type.GetIndexOfFieldWithName("__value_", nullptr, &bit_offset) !=
- UINT32_MAX) {
- // Old layout (pre 089a7cc5dea)
- m_skip_size = bit_offset / 8u;
- } else {
- auto ast_ctx = node_type.GetTypeSystem().dyn_cast_or_null<TypeSystemClang>();
- if (!ast_ctx)
- return;
- CompilerType tree_node_type = ast_ctx->CreateStructForIdentifier(
- llvm::StringRef(),
- {{"ptr0", ast_ctx->GetBasicType(lldb::eBasicTypeVoid).GetPointerType()},
- {"ptr1", ast_ctx->GetBasicType(lldb::eBasicTypeVoid).GetPointerType()},
- {"ptr2", ast_ctx->GetBasicType(lldb::eBasicTypeVoid).GetPointerType()},
- {"cw", ast_ctx->GetBasicType(lldb::eBasicTypeBool)},
- {"payload", (m_element_type.GetCompleteType(), m_element_type)}});
- std::string child_name;
- uint32_t child_byte_size;
- int32_t child_byte_offset = 0;
- uint32_t child_bitfield_bit_size;
- uint32_t child_bitfield_bit_offset;
- bool child_is_base_class;
- bool child_is_deref_of_parent;
- uint64_t language_flags;
- auto child_type =
- llvm::expectedToStdOptional(tree_node_type.GetChildCompilerTypeAtIndex(
- nullptr, 4, true, true, true, child_name, child_byte_size,
- child_byte_offset, child_bitfield_bit_size,
- child_bitfield_bit_offset, child_is_base_class,
- child_is_deref_of_parent, nullptr, language_flags));
- if (child_type && child_type->IsValid())
- m_skip_size = (uint32_t)child_byte_offset;
- }
+ auto ast_ctx = node_type.GetTypeSystem().dyn_cast_or_null<TypeSystemClang>();
+ if (!ast_ctx)
+ return;
+
+ CompilerType tree_node_type = ast_ctx->CreateStructForIdentifier(
+ llvm::StringRef(),
+ {{"ptr0", ast_ctx->GetBasicType(lldb::eBasicTypeVoid).GetPointerType()},
+ {"ptr1", ast_ctx->GetBasicType(lldb::eBasicTypeVoid).GetPointerType()},
+ {"ptr2", ast_ctx->GetBasicType(lldb::eBasicTypeVoid).GetPointerType()},
+ {"cw", ast_ctx->GetBasicType(lldb::eBasicTypeBool)},
+ {"payload", (m_element_type.GetCompleteType(), m_element_type)}});
+ std::string child_name;
+ uint32_t child_byte_size;
+ int32_t child_byte_offset = 0;
+ uint32_t child_bitfield_bit_size;
+ uint32_t child_bitfield_bit_offset;
+ bool child_is_base_class;
+ bool child_is_deref_of_parent;
+ uint64_t language_flags;
+ auto child_type =
+ llvm::expectedToStdOptional(tree_node_type.GetChildCompilerTypeAtIndex(
+ nullptr, 4, true, true, true, child_name, child_byte_size,
+ child_byte_offset, child_bitfield_bit_size, child_bitfield_bit_offset,
+ child_is_base_class, child_is_deref_of_parent, nullptr,
+ language_flags));
+ if (child_type && child_type->IsValid())
+ m_skip_size = (uint32_t)child_byte_offset;
}
lldb::ValueObjectSP
@@ -343,12 +337,8 @@ lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::GetChildAtIndex(
return lldb::ValueObjectSP();
}
GetValueOffset(iterated_sp);
- auto child_sp = iterated_sp->GetChildMemberWithName("__value_");
- if (child_sp)
- iterated_sp = child_sp;
- else
- iterated_sp = iterated_sp->GetSyntheticChildAtOffset(
- m_skip_size, m_element_type, true);
+ iterated_sp = iterated_sp->GetSyntheticChildAtOffset(m_skip_size,
+ m_element_type, true);
if (!iterated_sp) {
m_tree = nullptr;
return lldb::ValueObjectSP();
|
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Jul 3, 2024
…aluePair This patch cleans up the core of the `std::map` libc++ formatter. Depends on llvm#97544 and llvm#97549. Changes: * Renamed `m_skip_size` to `m_value_type_offset` to better describe what it's actually for. * Made updating `m_skip_size` (now `m_value_type_offset`) isolated to `GetKeyValuePair` (instead of doing so in the `GetValueOffset` helper). * We don't need special logic for the 0th index, so I merged the two `need_to_skip` branches.
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Jul 3, 2024
Depends on: * llvm#97544 * llvm#97549 * llvm#97551 This patch tries to simplify the way in which the `std::map` formatter goes from the root `__tree` pointer to a specific key/value pair. Previously we would synthesize a structure that mimicked what `__iter_pointer` looked like in memory, then called `GetChildCompilerTypeAtIndex` on it to find the byte offset into that structure at which the pair was located at, and finally use that offset through a call to `GetSyntheticChildAtOffset` to retrieve that pair. Not only was this logic hard to follow, and encoded the libc++ layout in non-obvious ways, it was also fragile to alignment miscalculations (llvm#97443); this would break after the new layout of std::map landed as part of inhttps://github.com/llvm/issues/93069. Instead, this patch simply casts the `__iter_pointer` to the `__node_pointer` and uses a straightforward `GetChildMemberWithName("__value_")` to get to the key/value we care about. This allows us to get rid of some support infrastructure/class state. Ideally we would fix the underlying alignment issue, but this unblocks the libc++ refactor in the interim, while also benefitting the formatter in terms of readability (in my opinion).
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Jul 3, 2024
…aluePair This patch cleans up the core of the `std::map` libc++ formatter. Depends on llvm#97544 and llvm#97549. Changes: * Renamed `m_skip_size` to `m_value_type_offset` to better describe what it's actually for. * Made updating `m_skip_size` (now `m_value_type_offset`) isolated to `GetKeyValuePair` (instead of doing so in the `GetValueOffset` helper). * We don't need special logic for the 0th index, so I merged the two `need_to_skip` branches.
Michael137
force-pushed
the
lldb/map-formatter-old-layout-removal
branch
from
3f760be to
2e36d3d
Compare
We currently supported the layout from pre-2016 (before the layout change in 14caaddd3f08e798dcd9ac0ddfc). We have another upcoming layout change in `__tree` and `map` (as part of require rewriting parts of this formatter. Removing the support will make those changes more straightforward to review/maintain. Being backward compatible would be great but we have no tests that actually verify that the old layout still works (and our oldest matrix bot tests clang-15). If anyone feels strongly about keeping this layout, we could possibly factor out that logic and keep it around.
Michael137
force-pushed
the
lldb/map-formatter-old-layout-removal
branch
from
2e36d3d to
fbcfcdf
Compare
Michael137
force-pushed
the
lldb/map-formatter-old-layout-removal
branch
from
fbcfcdf to
64af560
Compare
labath
approved these changes
Jul 8, 2024
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Jul 8, 2024
Depends on: * llvm#97544 * llvm#97549 * llvm#97551 This patch tries to simplify the way in which the `std::map` formatter goes from the root `__tree` pointer to a specific key/value pair. Previously we would synthesize a structure that mimicked what `__iter_pointer` looked like in memory, then called `GetChildCompilerTypeAtIndex` on it to find the byte offset into that structure at which the pair was located at, and finally use that offset through a call to `GetSyntheticChildAtOffset` to retrieve that pair. Not only was this logic hard to follow, and encoded the libc++ layout in non-obvious ways, it was also fragile to alignment miscalculations (llvm#97443); this would break after the new layout of std::map landed as part of inhttps://github.com/llvm/issues/93069. Instead, this patch simply casts the `__iter_pointer` to the `__node_pointer` and uses a straightforward `GetChildMemberWithName("__value_")` to get to the key/value we care about. This allows us to get rid of some support infrastructure/class state. Ideally we would fix the underlying alignment issue, but this unblocks the libc++ refactor in the interim, while also benefitting the formatter in terms of readability (in my opinion).
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Jul 8, 2024
Depends on: * llvm#97544 * llvm#97549 * llvm#97551 This patch tries to simplify the way in which the `std::map` formatter goes from the root `__tree` pointer to a specific key/value pair. Previously we would synthesize a structure that mimicked what `__iter_pointer` looked like in memory, then called `GetChildCompilerTypeAtIndex` on it to find the byte offset into that structure at which the pair was located at, and finally use that offset through a call to `GetSyntheticChildAtOffset` to retrieve that pair. Not only was this logic hard to follow, and encoded the libc++ layout in non-obvious ways, it was also fragile to alignment miscalculations (llvm#97443); this would break after the new layout of std::map landed as part of inhttps://github.com/llvm/issues/93069. Instead, this patch simply casts the `__iter_pointer` to the `__node_pointer` and uses a straightforward `GetChildMemberWithName("__value_")` to get to the key/value we care about. This allows us to get rid of some support infrastructure/class state. Ideally we would fix the underlying alignment issue, but this unblocks the libc++ refactor in the interim, while also benefitting the formatter in terms of readability (in my opinion).
Michael137
added a commit
that referenced
this pull request
Jul 8, 2024
Depends on: * #97544 * #97549 * #97551 This patch tries to simplify the way in which the `std::map` formatter goes from the root `__tree` pointer to a specific key/value pair. Previously we would: 1. synthesize a structure that mimicked what `__iter_pointer` looked like in memory 2. call `GetChildCompilerTypeAtIndex` on it to find the byte offset at which the pair was located in the synthesized structure 3. finally, use that offset through a call to `GetSyntheticChildAtOffset` to retrieve the key/value pair Not only was this logic hard to follow, and encoded the libc++ layout in non-obvious ways, it was also fragile to alignment miscalculations (#97443); this would break once the new layout of std::map landed as part of #93069. Instead, this patch simply casts the `__iter_pointer` to the `__node_pointer` and uses a straightforward `GetChildMemberWithName("__value_")` to get to the key/value we care about. This allows us to get rid of some support infrastructure/class state. Ideally we would fix the underlying alignment issue, but this unblocks the libc++ refactor in the interim, while also benefitting the formatter in terms of readability (in my opinion).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We currently supported the layout from pre-2016 (before the layout change in 14caaddd3f08e798dcd9ac0ddfc). We have another upcoming layout change in
__treeandmap(as part of #93069) which will likely require rewriting parts of this formatter. Removing the support for the pre-2016 layout will make those changes more straightforward to review/maintain.Being backward compatible would be great but we have no tests that actually verify that the old layout still works (and our oldest matrix bot tests clang-15). If anyone feels strongly about keeping this layout, we could possibly factor out that logic and keep it around.