[libc++] Use [[no_unique_address]] in place of __compressed_pair to reduce compile time and debug info size
#93069
Labels
Comments
rnk
added
the
libc++
|
See #76756 |
|
@Michael137 My understanding was this work was waiting on lldb support for |
Yup it's on my list of things to do this week. Got sidetracked with other issues and was OOO for a bit. Will update this thread once I have the PR up |
Michael137
added a commit
to Michael137/llvm-project
that referenced
this issue
Jul 2, 2024
…in ExternalLayouts This patch is motivated by the LLDB support required for: llvm#93069 In the presence of `[[no_unique_address]]`, LLDB may ask Clang to lay out types with overlapping field offsets. Because we don't have attributes such as `packed` or `no_unique_address` in the LLDB AST, the `RecordLayoutBuilder` supports an `InferAlignment` mode, which, in the past, attempted to detect layouts which came from packed structures in order to provide a conservative estimate of alignment for it (since `DW_AT_alignment` isn't emitted unless explicitly changed with `alignas`, etc.). However, in the presence of overlapping fields due to `no_unique_address`, `InferAlignment` would set the alignment of structures to `1` for which that's incorrect. This poses issues in some LLDB formatters that synthesize new Clang types and rely on the layout builder to get the `FieldOffset` of structures right that we did have DWARF offsets for. The result of this is that if we get the alignment wrong, LLDB reads out garbage data from the wrong field offsets. There are a couple of solutions to this that we considered: 1. Make LLDB formatters not do the above, and make them more robust to inaccurate alignment. 2. Remove `InferAlignment` entirely and rely on Clang emitting `DW_AT_alignment` for packed structures. 3. Remove `InferAlignment` and detect packedness from within LLDB. 4. Make the `InferAlignment` logic account for overlapping fields. Option (1) turned out quite hairy and it's not clear we can achieve this with the tools available for certain STL formatters (particularly `std::map`). But I would still very much like to simplify this if we can. Option (2) wouldn't help with GCC-compiled binaries, and if we can get away with LLDB not needing the alignment, then we wouldn't need to increase debug-info size. Option (3), AFAICT, would require us to reimplement some of the layout logic in the layout builder. Would be great if we can avoid this added complexity. Option (4) seemed like the best option in the interim. As part of this change I also removed one of the `InferAlignment` blocks. The test-cases associated with this code-path pass regardless, and from the description of the change that introduced it it's not clear why specifically the base offsets would influence the `Alignment` field, and how it would imply packedness. But happy to be proven wrong. Ultimately it would be great if we can get rid of the `InferAlignment` infrastructure and support our use-cases in LLDB or DWARF instead.
Michael137
added a commit
to Michael137/llvm-project
that referenced
this issue
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).
This was referenced Jul 3, 2024
Michael137
added a commit
to Michael137/llvm-project
that referenced
this issue
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 issue
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 issue
Jul 8, 2024
We currently supported the layout from pre-2016 (before the layout change in [14caaddd3f08e798dcd9ac0ddfc](14caaddd3f08e798dcd9ac0ddfc)). We have another upcoming layout change in `__tree` and `map` (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.
Michael137
added a commit
that referenced
this issue
Jul 8, 2024
…#97754) Depends on #97752 This patch changes the way we retrieve the key/value pair in the `std::unordered_map::iterator` formatter (similar to how we are changing it for `std::map::iterator` in #97713, the motivations being the same). The old logic was not very easy to follow, and encoded the libc++ layout in non-obvious ways. But mainly it was also fragile to alignment miscalculations (#97443); this would break once the new layout of `std::unordered_map` landed as part of #93069. Instead, this patch simply casts the `__hash_iterator` to a `__node_pointer` (which is what libc++ does too) and uses a straightforward `GetChildMemberWithName("__value_")` to get to the key/value we care about. The `std::unordered_map` already does it this way, so we align the iterator counterpart to do the same. We can eventually re-use the core-part of the `std::unordered_map` and `std::unordered_map::iterator` formatters. But it will be an easier to change to review once both simplifications landed.
Michael137
added a commit
that referenced
this issue
Jul 8, 2024
…97713) Depends on #97687 Similar to #97579, this patch simplifies the way in which we retrieve the key/value pair of a `std::map` (in this case of the `std::map::iterator`). We do this for the same reason: not only was the old 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. We can eventually re-use the core-part of the `std::map` and `std::map::iterator` formatters. But it will be an easier to change to review once both simplifications landed.
Michael137
added a commit
that referenced
this issue
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).
|
Quick update: all the LLDB blockers have been merged/approved by now. The main remaining change is to the formatters themselves: #96538 (which is ready to land; I'm happy to coordinate merging this once the libc++ changes are ready). Outside of this there are 2 smaller LLDB issues that would be uncovered by this (which we know the root cause of and should be able to fix within in the coming weeks. But they're not a blocker since they only affect an experimental feature of LLDB). |
|
The last comment on #76756 says that we have to wait for the LLVM 20 release cycle because the last released version of XCode clang is too old (LLVM 15? unclear). Based on our usual release timeline, the LLVM 19 release will branch in September, so we'd have to wait until then to land this on mainline. Is there a path to expediting this, like making it opt-in, or only making it available under the unstable ABI? @zmodem gathered data from Chrome and internal Google binaries, and it looks really promising. I don't have the precise numbers handy, but it's in the realm of reducing overall debug info size by around 5%. |
|
The branch will be next week. After updating the Apple runners we can land the patch. The 19.1 release will be in September. |
|
Great, sorry about misunderstanding the branch timeline, that was my bad. |
Michael137
added a commit
that referenced
this issue
Jul 29, 2024
…ords" (#100903) This reverts commit 88e5206. The original change went in a while ago (last year) in https://reviews.llvm.org/D145057. The specific reason I'm proposing a revert is that this is now causing exactly the issue that @balazske predicted in https://reviews.llvm.org/D145057#4164717: > Problematic case is if the attribute has pointer to a Decl or Type that is imported here in a state when the field is already created but not initialized. Another problem is that attributes are added a second time in Import(Decl *) This now came up in the testing of LLDB support for #93069. There, `__compressed_pair`s are now replaced with fields that have an `alignof(...)` and `[[no_unique_address]]` attribute. In the specific failing case, we're importing following `std::list` method: ``` size_type& __sz() _NOEXCEPT { return __size_; } ``` During this process, we create a new `__size_` `FieldDecl` (but don't initialize it yet). Then we go down the `ImportAttrs` codepath added in D145057. This imports the `alignof` expression which then references the uninitialized `__size_` and we trigger an assertion. Important to note, this codepath was added specifically to support `[[no_unique_address]]` in LLDB, and was supposed to land with https://reviews.llvm.org/D143347. But the LLDB side of that never landed, and the way we plan to support `[[no_unique_address]]` doesn't require things like the `markEmpty` method added here. So really, this is a dead codepath, which as pointed out in the original review isn't fully sound.
Michael137
added a commit
that referenced
this issue
Jul 29, 2024
…rrayType (#100710) Depends on #100674 Currently, we treat VLAs declared as `int[]` and `int[0]` identically. I.e., we create them as `IncompleteArrayType`s. However, the `DW_AT_count` for `int[0]` *does* exist, and is retrievable without an execution context. This patch decouples the notion of "has 0 elements" from "has no known `DW_AT_count`". This aligns with how Clang represents `int[0]` in the AST (it treats it as a `ConstantArrayType` of 0 size). This issue was surfaced when adapting LLDB to #93069. There, the `__compressed_pair_padding` type has a `char[0]` member. If we previously got the `__compressed_pair_padding` out of a module (where clang represents `char[0]` as a `ConstantArrayType`), and try to merge the AST with one we got from DWARF (where LLDB used to represent `char[0]` as an `IncompleteArrayType`), the AST structural equivalence check fails, resulting in silent ASTImporter failures. This manifested in a failure in `TestNonModuleTypeSeparation.py`. **Implementation** 1. Adjust `ParseChildArrayInfo` to store the element counts of each VLA dimension as an `optional<uint64_t>`, instead of a regular `uint64_t`. So when we pass this down to `CreateArrayType`, we have a better heuristic for what is an `IncompleteArrayType`. 2. In `TypeSystemClang::GetBitSize`, if we encounter a `ConstantArrayType` simply return the size that it was created with. If we couldn't determine the authoritative bound from DWARF during parsing, we would've created an `IncompleteArrayType`. This ensures that `GetBitSize` on arrays with `DW_AT_count 0x0` returns `0` (whereas previously it would've returned a `std::nullopt`, causing that `FieldDecl` to just get dropped during printing)
Michael137
added a commit
to Michael137/llvm-project
that referenced
this issue
Oct 21, 2024
…in ExternalLayouts This patch is motivated by the LLDB support required for: llvm#93069 In the presence of `[[no_unique_address]]`, LLDB may ask Clang to lay out types with overlapping field offsets. Because we don't have attributes such as `packed` or `no_unique_address` in the LLDB AST, the `RecordLayoutBuilder` supports an `InferAlignment` mode, which, in the past, attempted to detect layouts which came from packed structures in order to provide a conservative estimate of alignment for it (since `DW_AT_alignment` isn't emitted unless explicitly changed with `alignas`, etc.). However, in the presence of overlapping fields due to `no_unique_address`, `InferAlignment` would set the alignment of structures to `1` for which that's incorrect. This poses issues in some LLDB formatters that synthesize new Clang types and rely on the layout builder to get the `FieldOffset` of structures right that we did have DWARF offsets for. The result of this is that if we get the alignment wrong, LLDB reads out garbage data from the wrong field offsets. There are a couple of solutions to this that we considered: 1. Make LLDB formatters not do the above, and make them more robust to inaccurate alignment. 2. Remove `InferAlignment` entirely and rely on Clang emitting `DW_AT_alignment` for packed structures. 3. Remove `InferAlignment` and detect packedness from within LLDB. 4. Make the `InferAlignment` logic account for overlapping fields. Option (1) turned out quite hairy and it's not clear we can achieve this with the tools available for certain STL formatters (particularly `std::map`). But I would still very much like to simplify this if we can. Option (2) wouldn't help with GCC-compiled binaries, and if we can get away with LLDB not needing the alignment, then we wouldn't need to increase debug-info size. Option (3), AFAICT, would require us to reimplement some of the layout logic in the layout builder. Would be great if we can avoid this added complexity. Option (4) seemed like the best option in the interim. As part of this change I also removed one of the `InferAlignment` blocks. The test-cases associated with this code-path pass regardless, and from the description of the change that introduced it it's not clear why specifically the base offsets would influence the `Alignment` field, and how it would imply packedness. But happy to be proven wrong. Ultimately it would be great if we can get rid of the `InferAlignment` infrastructure and support our use-cases in LLDB or DWARF instead.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
As covered in crbug.com/338094922 ,
__compressed_pairis a major contributor to debug info size. Many libc++ classes use them to store allocators, default deleters, and other things. I believe thatno_unique_addresswas intended to be layout-compatible with the__compressed_pairtechnique, but I'm not sure it actually is.Regardless, for unstable ABI users, it would be a major QoI improvement to avoid extra compressed pair template instantiations.
I thought this feature request had already been filed, but I couldn't find it.
Related work:
no_unique_addressis reserved word in pre-C++20 dialects (non-conforming) #61196Can we do this yet?
cc @zmodem @cjdb @amykhuang @zygoloid
The text was updated successfully, but these errors were encountered: