From 79b9ed6e870459cf583aa41969deab7166dbbff1 Mon Sep 17 00:00:00 2001 From: Michael Buch Date: Mon, 15 May 2023 15:04:07 +0100 Subject: [PATCH 1/3] [lldb][DWARFASTParserClang][NFC] Simplify unnamed bitfield condition 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 ca64f9af04472a27656d84c06f4e332ebbf14b4d) --- .../SymbolFile/DWARF/DWARFASTParserClang.cpp | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp index 81d846ba2a25f..603a9c9a0f8b2 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp @@ -2839,10 +2839,9 @@ void DWARFASTParserClang::ParseSingleMember( die.GetCU()->Supports_unnamed_objc_bitfields(); if (detect_unnamed_bitfields) { - llvm::Optional unnamed_field_info; - uint64_t last_field_end = 0; - - last_field_end = last_field_info.bit_offset + last_field_info.bit_size; + std::optional unnamed_field_info; + uint64_t last_field_end = + last_field_info.bit_offset + last_field_info.bit_size; if (!last_field_info.IsBitfield()) { // The last field was not a bit-field... @@ -2862,10 +2861,8 @@ void DWARFASTParserClang::ParseSingleMember( // indeed an unnamed bit-field. We currently do not have the // machinary to track the offset of the last field of classes we // have seen before, so we are not handling this case. - if (this_field_info.bit_offset != last_field_end && - this_field_info.bit_offset > last_field_end && - !(last_field_info.bit_offset == 0 && - last_field_info.bit_size == 0 && + if (this_field_info.bit_offset > last_field_end && + !(last_field_info.bit_offset == 0 && last_field_info.bit_size == 0 && layout_info.base_offsets.size() != 0)) { unnamed_field_info = FieldInfo{}; unnamed_field_info->bit_size = From 4c5a02ca7d97ddf571fab9574874159a80f30913 Mon Sep 17 00:00:00 2001 From: Michael Buch Date: Mon, 15 May 2023 15:31:14 +0100 Subject: [PATCH 2/3] [lldb][DWARFASTParserClang][NFC] Extract condition for unnamed bitfield 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 56eff197d0d629c768e8e48fb995e8a1d2cd6441) --- .../SymbolFile/DWARF/DWARFASTParserClang.cpp | 40 +++++++++++++------ .../SymbolFile/DWARF/DWARFASTParserClang.h | 16 ++++++++ 2 files changed, 44 insertions(+), 12 deletions(-) diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp index 603a9c9a0f8b2..3c2aad55e7c7d 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp @@ -2852,18 +2852,8 @@ void DWARFASTParserClang::ParseSingleMember( last_field_end += word_width - (last_field_end % word_width); } - // If we have a gap between the last_field_end and the current - // field we have an unnamed bit-field. - // If we have a base class, we assume there is no unnamed - // bit-field if this is the first field since the gap can be - // attributed to the members from the base class. This assumption - // is not correct if the first field of the derived class is - // indeed an unnamed bit-field. We currently do not have the - // machinary to track the offset of the last field of classes we - // have seen before, so we are not handling this case. - if (this_field_info.bit_offset > last_field_end && - !(last_field_info.bit_offset == 0 && last_field_info.bit_size == 0 && - layout_info.base_offsets.size() != 0)) { + if (ShouldCreateUnnamedBitfield(last_field_info, last_field_end, + this_field_info, layout_info)) { unnamed_field_info = FieldInfo{}; unnamed_field_info->bit_size = this_field_info.bit_offset - last_field_end; @@ -3657,3 +3647,29 @@ bool DWARFASTParserClang::CopyUniqueClassMethodTypes( return !failures.empty(); } + +bool DWARFASTParserClang::ShouldCreateUnnamedBitfield( + FieldInfo const &last_field_info, uint64_t last_field_end, + FieldInfo const &this_field_info, + lldb_private::ClangASTImporter::LayoutInfo const &layout_info) const { + // If we have a gap between the last_field_end and the current + // field we have an unnamed bit-field. + if (this_field_info.bit_offset <= last_field_end) + return false; + + // If we have a base class, we assume there is no unnamed + // bit-field if this is the first field since the gap can be + // attributed to the members from the base class. This assumption + // is not correct if the first field of the derived class is + // indeed an unnamed bit-field. We currently do not have the + // machinary to track the offset of the last field of classes we + // have seen before, so we are not handling this case. + const bool have_base = layout_info.base_offsets.size() != 0; + const bool this_is_first_field = + last_field_info.bit_offset == 0 && last_field_info.bit_size == 0; + + if (have_base && this_is_first_field) + return false; + + return true; +} diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h index d479a73705af3..43ad2c668c3fb 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h @@ -219,6 +219,22 @@ class DWARFASTParserClang : public DWARFASTParser { } }; + /// Returns 'true' if we should create an unnamed bitfield + /// and add it to the parser's current AST. + /// + /// \param[in] last_field_info FieldInfo of the previous DW_TAG_member + /// we parsed. + /// \param[in] last_field_end Offset (in bits) where the last parsed field + /// ended. + /// \param[in] this_field_info FieldInfo of the current DW_TAG_member + /// being parsed. + /// \param[in] layout_info Layout information of all decls parsed by the + /// current parser. + bool ShouldCreateUnnamedBitfield( + FieldInfo const &last_field_info, uint64_t last_field_end, + FieldInfo const &this_field_info, + lldb_private::ClangASTImporter::LayoutInfo const &layout_info) const; + /// Parses a DW_TAG_APPLE_property DIE and appends the parsed data to the /// list of delayed Objective-C properties. /// From d3207b7dfc024e4efc4dc7234ade9b6815ed4ee1 Mon Sep 17 00:00:00 2001 From: Michael Buch Date: Mon, 15 May 2023 17:37:46 +0100 Subject: [PATCH 3/3] [lldb][DWARFASTParserClang] Don't create unnamed bitfields to account 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 3c30f224005e3749c89e00592bd2a43aba2aaf75) --- .../SymbolFile/DWARF/DWARFASTParserClang.cpp | 24 ++++++++++++------- .../SymbolFile/DWARF/DWARFASTParserClang.h | 4 ++++ .../lang/cpp/bitfields/TestCppBitfields.py | 11 +++++++++ lldb/test/API/lang/cpp/bitfields/main.cpp | 9 +++++++ 4 files changed, 40 insertions(+), 8 deletions(-) diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp index 3c2aad55e7c7d..1e5de6cc36971 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp @@ -2894,8 +2894,10 @@ void DWARFASTParserClang::ParseSingleMember( // artificial member with (unnamed bitfield) padding. // FIXME: This check should verify that this is indeed an artificial member // we are supposed to ignore. - if (attrs.is_artificial) + if (attrs.is_artificial) { + last_field_info.SetIsArtificial(true); return; + } if (!member_clang_type.IsCompleteType()) member_clang_type.GetCompleteType(); @@ -3658,17 +3660,23 @@ bool DWARFASTParserClang::ShouldCreateUnnamedBitfield( return false; // If we have a base class, we assume there is no unnamed - // bit-field if this is the first field since the gap can be - // attributed to the members from the base class. This assumption - // is not correct if the first field of the derived class is - // indeed an unnamed bit-field. We currently do not have the - // machinary to track the offset of the last field of classes we - // have seen before, so we are not handling this case. + // bit-field if either of the following is true: + // (a) this is the first field since the gap can be + // attributed to the members from the base class. + // FIXME: This assumption is not correct if the first field of + // the derived class is indeed an unnamed bit-field. We currently + // do not have the machinary to track the offset of the last field + // of classes we have seen before, so we are not handling this case. + // (b) Or, the first member of the derived class was a vtable pointer. + // In this case we don't want to create an unnamed bitfield either + // since those will be inserted by clang later. const bool have_base = layout_info.base_offsets.size() != 0; const bool this_is_first_field = last_field_info.bit_offset == 0 && last_field_info.bit_size == 0; + const bool first_field_is_vptr = + last_field_info.bit_offset == 0 && last_field_info.IsArtificial(); - if (have_base && this_is_first_field) + if (have_base && (this_is_first_field || first_field_is_vptr)) return false; return true; diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h index 43ad2c668c3fb..092dd56b96ed8 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h @@ -206,12 +206,16 @@ class DWARFASTParserClang : public DWARFASTParser { uint64_t bit_size = 0; uint64_t bit_offset = 0; bool is_bitfield = false; + bool is_artificial = false; FieldInfo() = default; void SetIsBitfield(bool flag) { is_bitfield = flag; } bool IsBitfield() { return is_bitfield; } + void SetIsArtificial(bool flag) { is_artificial = flag; } + bool IsArtificial() const { return is_artificial; } + bool NextBitfieldOffsetIsValid(const uint64_t next_bit_offset) const { // Any subsequent bitfields must not overlap and must be at a higher // bit offset than any previous bitfield + size. diff --git a/lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py b/lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py index f38d0f6c7edf4..b85f23eb4bd0a 100644 --- a/lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py +++ b/lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py @@ -168,3 +168,14 @@ def test_bitfield_behind_vtable_ptr(self): result_children=with_vtable_and_unnamed_children) self.expect_var_path("with_vtable_and_unnamed", children=with_vtable_and_unnamed_children) + + derived_with_vtable_children = [ + ValueCheck(name="Base", children=[ + ValueCheck(name="b_a", value="2", type="uint32_t") + ]), + ValueCheck(name="a", value="1", type="unsigned int:1") + ] + self.expect_expr("derived_with_vtable", + result_children=derived_with_vtable_children) + self.expect_var_path("derived_with_vtable", + children=derived_with_vtable_children) diff --git a/lldb/test/API/lang/cpp/bitfields/main.cpp b/lldb/test/API/lang/cpp/bitfields/main.cpp index eb9db7271aae6..6e1d4ba63bf6d 100644 --- a/lldb/test/API/lang/cpp/bitfields/main.cpp +++ b/lldb/test/API/lang/cpp/bitfields/main.cpp @@ -113,6 +113,12 @@ struct HasBaseWithVTable : BaseWithVTable { }; HasBaseWithVTable base_with_vtable; +struct DerivedWithVTable : public Base { + virtual ~DerivedWithVTable() {} + unsigned a : 1; +}; +DerivedWithVTable derived_with_vtable; + int main(int argc, char const *argv[]) { lba.a = 2; @@ -153,5 +159,8 @@ int main(int argc, char const *argv[]) { base_with_vtable.b = 0; base_with_vtable.c = 5; + derived_with_vtable.b_a = 2; + derived_with_vtable.a = 1; + return 0; // break here }