[cherry-pick][swift/releas/5.9] [lldb][DWARFASTParserClang] Don't create unnamed bitfields to account for vtable pointer #6865
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
Minor cleanup of redundant variable initialization and if-condition. These are leftovers/oversights from previous cleanup in this area: * https://reviews.llvm.org/D72953 * https://reviews.llvm.org/D76808 Differential Revision: https://reviews.llvm.org/D150589 (cherry picked from commit ca64f9a)
…ld creation into helper function This patch adds a new private helper `DWARFASTParserClang::ShouldCreateUnnamedBitfield` which `ParseSingleMember` whether we should fill the current gap in a structure layout with unnamed bitfields. Extracting this logic will allow us to add additional conditions in upcoming patches without jeoperdizing readability of `ParseSingleMember`. We also store some of the boolean conditions in local variables to make the intent more obvious. Differential Revision: https://reviews.llvm.org/D150590 (cherry picked from commit 56eff19)
… for vtable pointer **Summary** When filling out the LayoutInfo for a structure with the offsets from DWARF, LLDB fills gaps in the layout by creating unnamed bitfields and adding them to the AST. If we don't do this correctly and our layout has overlapping fields, we will hat an assertion in `clang::CGRecordLowering::lower()`. Specifically, if we have a derived class with a VTable and a bitfield immediately following the vtable pointer, we create a layout with overlapping fields. This is an oversight in some of the previous cleanups done around this area. In `D76808`, we prevented LLDB from creating unnamed bitfields if there was a gap between the last field of a base class and the start of a bitfield in the derived class. In `D112697`, we started accounting for the vtable pointer. The intention there was to make sure the offset bookkeeping accounted for the existence of a vtable pointer (but we didn't actually want to create any AST nodes for it). Now that `last_field_info.bit_size` was being set even for artifical fields, the previous fix `D76808` broke specifically for cases where the bitfield was the first member of a derived class with a vtable (this scenario wasn't tested so we didn't notice it). I.e., we started creating redundant unnamed bitfields for where the vtable pointer usually sits. This confused the lowering logic in clang. This patch adds a condition to `ShouldCreateUnnamedBitfield` which checks whether the first field in the derived class is a vtable ptr. **Testing** * Added API test case Differential Revision: https://reviews.llvm.org/D150591 (cherry picked from commit 3c30f22)
|
@swift-ci test |
adrian-prantl
approved these changes
May 16, 2023
adrian-prantl
approved these changes
May 16, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
When filling out the LayoutInfo for a structure with the offsets
from DWARF, LLDB fills gaps in the layout by creating unnamed
bitfields and adding them to the AST. If we don't do this correctly
and our layout has overlapping fields, we will hat an assertion
in
clang::CGRecordLowering::lower(). Specifically, if we havea derived class with a VTable and a bitfield immediately following
the vtable pointer, we create a layout with overlapping fields.
This is an oversight in some of the previous cleanups done around this
area.
In
D76808, we prevented LLDB from creating unnamed bitfields if therewas a gap between the last field of a base class and the start of a bitfield
in the derived class.
In
D112697, we started accounting for the vtable pointer. The intentionthere was to make sure the offset bookkeeping accounted for the
existence of a vtable pointer (but we didn't actually want to create
any AST nodes for it). Now that
last_field_info.bit_sizewas beingset even for artifical fields, the previous fix
D76808brokespecifically for cases where the bitfield was the first member of a
derived class with a vtable (this scenario wasn't tested so we didn't
notice it). I.e., we started creating redundant unnamed bitfields for
where the vtable pointer usually sits. This confused the lowering logic
in clang.
This patch adds a condition to
ShouldCreateUnnamedBitfieldwhichchecks whether the first field in the derived class is a vtable ptr.
Testing
Differential Revision: https://reviews.llvm.org/D150591
(cherry picked from commit 3c30f224005e3749c89e00592bd2a43aba2aaf75)