From fcaf5f6c01a09f23b948afb8c91c4dd951d4525e Mon Sep 17 00:00:00 2001 From: shafik Date: Thu, 23 Jan 2020 14:42:12 -0800 Subject: [PATCH] [LLDB] Fix the handling of unnamed bit-fields when parsing DWARF We ran into an assert when debugging clang and performing an expression on a class derived from DeclContext. The assert was indicating we were getting the offsets wrong for RecordDeclBitfields. We were getting both the size and offset of unnamed bit-field members wrong. We could fix this case with a quick change but as I extended the test suite to include more combinations we kept finding more cases that were being handled incorrectly. A fix that handled all the new cases as well as the cases already covered required a refactor of the existing technique. Differential Revision: https://reviews.llvm.org/D72953 --- .../lldbsuite/test/lang/cpp/bitfields/Makefile | 3 + .../test/lang/cpp/bitfields/TestCppBitfields.py | 105 +++++++++++++ .../lldbsuite/test/lang/cpp/bitfields/main.cpp | 81 ++++++++++ .../SymbolFile/DWARF/DWARFASTParserClang.cpp | 166 +++++++-------------- .../Plugins/SymbolFile/DWARF/DWARFASTParserClang.h | 35 ++--- 5 files changed, 255 insertions(+), 135 deletions(-) create mode 100644 lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/Makefile create mode 100644 lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/TestCppBitfields.py create mode 100644 lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/main.cpp diff --git a/lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/Makefile b/lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/Makefile new file mode 100644 index 0000000..99998b2 --- /dev/null +++ b/lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/Makefile @@ -0,0 +1,3 @@ +CXX_SOURCES := main.cpp + +include Makefile.rules diff --git a/lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/TestCppBitfields.py b/lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/TestCppBitfields.py new file mode 100644 index 0000000..696e564 --- /dev/null +++ b/lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/TestCppBitfields.py @@ -0,0 +1,105 @@ +"""Show bitfields and check that they display correctly.""" + +import lldb +from lldbsuite.test.decorators import * +from lldbsuite.test.lldbtest import * +from lldbsuite.test import lldbutil + + +class CppBitfieldsTestCase(TestBase): + + mydir = TestBase.compute_mydir(__file__) + + def setUp(self): + # Call super's setUp(). + TestBase.setUp(self) + # Find the line number to break inside main(). + self.line = line_number('main.cpp', '// Set break point at this line.') + + # BitFields exhibit crashes in record layout on Windows + # (http://llvm.org/pr21800) + @skipIfWindows + def test_and_run_command(self): + """Test 'frame variable ...' on a variable with bitfields.""" + self.build() + + lldbutil.run_to_source_breakpoint(self, '// Set break point at this line.', + lldb.SBFileSpec("main.cpp", False)) + + # The stop reason of the thread should be breakpoint. + self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT, + substrs=['stopped', + 'stop reason = breakpoint']) + + # The breakpoint should have a hit count of 1. + self.expect("breakpoint list -f", BREAKPOINT_HIT_ONCE, + substrs=[' resolved, hit count = 1']) + + self.expect("expr (lba.a)", VARIABLES_DISPLAYED_CORRECTLY, + substrs=['unsigned int', '2']) + self.expect("expr (lbb.b)", VARIABLES_DISPLAYED_CORRECTLY, + substrs=['unsigned int', '3']) + self.expect("expr (lbc.c)", VARIABLES_DISPLAYED_CORRECTLY, + substrs=['unsigned int', '4']) + self.expect("expr (lbd.a)", VARIABLES_DISPLAYED_CORRECTLY, + substrs=['unsigned int', '5']) + self.expect("expr (clang_example.f.a)", VARIABLES_DISPLAYED_CORRECTLY, + substrs=['uint64_t', '1']) + + self.expect( + "frame variable --show-types lba", + VARIABLES_DISPLAYED_CORRECTLY, + substrs=[ + '(int:32) = ', + '(unsigned int:20) a = 2', + ]) + + self.expect( + "frame variable --show-types lbb", + VARIABLES_DISPLAYED_CORRECTLY, + substrs=[ + '(unsigned int:1) a = 1', + '(int:31) =', + '(unsigned int:20) b = 3', + ]) + + self.expect( + "frame variable --show-types lbc", + VARIABLES_DISPLAYED_CORRECTLY, + substrs=[ + '(int:22) =', + '(unsigned int:1) a = 1', + '(unsigned int:1) b = 0', + '(unsigned int:5) c = 4', + '(unsigned int:1) d = 1', + '(int:2) =', + '(unsigned int:20) e = 20', + ]) + + self.expect( + "frame variable --show-types lbd", + VARIABLES_DISPLAYED_CORRECTLY, + substrs=[ + '(char [3]) arr = "abc"', + '(int:32) =', + '(unsigned int:20) a = 5', + ]) + + self.expect( + "frame variable --show-types clang_example", + VARIABLES_DISPLAYED_CORRECTLY, + substrs=[ + '(int:22) =', + '(uint64_t:1) a = 1', + '(uint64_t:1) b = 0', + '(uint64_t:1) c = 1', + '(uint64_t:1) d = 0', + '(uint64_t:1) e = 1', + '(uint64_t:1) f = 0', + '(uint64_t:1) g = 1', + '(uint64_t:1) h = 0', + '(uint64_t:1) i = 1', + '(uint64_t:1) j = 0', + '(uint64_t:1) k = 1', + ]) + diff --git a/lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/main.cpp b/lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/main.cpp new file mode 100644 index 0000000..975c0f0 --- /dev/null +++ b/lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/main.cpp @@ -0,0 +1,81 @@ +#include + +int main(int argc, char const *argv[]) { + struct LargeBitsA { + unsigned int : 30, a : 20; + } lba; + + struct LargeBitsB { + unsigned int a : 1, : 11, : 12, b : 20; + } lbb; + + struct LargeBitsC { + unsigned int : 13, : 9, a : 1, b : 1, c : 5, d : 1, e : 20; + } lbc; + + struct LargeBitsD { + char arr[3]; + unsigned int : 30, a : 20; + } lbd; + + // This case came up when debugging clang and models RecordDeclBits + struct BitExampleFromClangDeclContext { + class fields { + uint64_t : 13; + uint64_t : 9; + + uint64_t a: 1; + uint64_t b: 1; + uint64_t c: 1; + uint64_t d: 1; + uint64_t e: 1; + uint64_t f: 1; + uint64_t g: 1; + uint64_t h: 1; + uint64_t i: 1; + uint64_t j: 1; + uint64_t k: 1; + + // In order to reproduce the crash for this case we need the + // members of fields to stay private :-( + friend struct BitExampleFromClangDeclContext; + }; + + union { + struct fields f; + }; + + BitExampleFromClangDeclContext() { + f.a = 1; + f.b = 0; + f.c = 1; + f.d = 0; + f.e = 1; + f.f = 0; + f.g = 1; + f.h = 0; + f.i = 1; + f.j = 0; + f.k = 1; + } + } clang_example; + + lba.a = 2; + + lbb.a = 1; + lbb.b = 3; + + lbc.a = 1; + lbc.b = 0; + lbc.c = 4; + lbc.d = 1; + lbc.e = 20; + + lbd.arr[0] = 'a'; + lbd.arr[1] = 'b'; + lbd.arr[2] = 'c'; + lbd.a = 5; + + + return 0; // Set break point at this line. +} diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp index 1aa82ab..7a610e0 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp @@ -85,35 +85,6 @@ static bool DeclKindIsCXXClass(clang::Decl::Kind decl_kind) { return false; } -struct BitfieldInfo { - uint64_t bit_size; - uint64_t bit_offset; - - BitfieldInfo() - : bit_size(LLDB_INVALID_ADDRESS), bit_offset(LLDB_INVALID_ADDRESS) {} - - void Clear() { - bit_size = LLDB_INVALID_ADDRESS; - bit_offset = LLDB_INVALID_ADDRESS; - } - - bool IsValid() const { - return (bit_size != LLDB_INVALID_ADDRESS) && - (bit_offset != LLDB_INVALID_ADDRESS); - } - - bool NextBitfieldOffsetIsValid(const uint64_t next_bit_offset) const { - if (IsValid()) { - // This bitfield info is valid, so any subsequent bitfields must not - // overlap and must be at a higher bit offset than any previous bitfield - // + size. - return (bit_size + bit_offset) <= next_bit_offset; - } else { - // If the this BitfieldInfo is not valid, then any offset isOK - return true; - } - } -}; ClangASTImporter &DWARFASTParserClang::GetClangASTImporter() { if (!m_clang_ast_importer_up) { @@ -2419,7 +2390,7 @@ void DWARFASTParserClang::ParseSingleMember( lldb::AccessType &default_accessibility, DelayedPropertyList &delayed_properties, lldb_private::ClangASTImporter::LayoutInfo &layout_info, - BitfieldInfo &last_field_info) { + FieldInfo &last_field_info) { ModuleSP module_sp = parent_die.GetDWARF()->GetObjectFile()->GetModule(); const dw_tag_t tag = die.Tag(); // Get the parent byte size so we can verify any members will fit @@ -2453,6 +2424,14 @@ void DWARFASTParserClang::ParseSingleMember( const dw_attr_t attr = attributes.AttributeAtIndex(i); DWARFFormValue form_value; if (attributes.ExtractFormValueAtIndex(i, form_value)) { + // DW_AT_data_member_location indicates the byte offset of the + // word from the base address of the structure. + // + // DW_AT_bit_offset indicates how many bits into the word + // (according to the host endianness) the low-order bit of the + // field starts. AT_bit_offset can be negative. + // + // DW_AT_bit_size indicates the size of the field in bits. switch (attr) { case DW_AT_name: name = form_value.AsCString(); @@ -2603,36 +2582,24 @@ void DWARFASTParserClang::ParseSingleMember( Type *member_type = die.ResolveTypeUID(encoding_form.Reference()); clang::FieldDecl *field_decl = nullptr; + const uint64_t character_width = 8; + const uint64_t word_width = 32; if (tag == DW_TAG_member) { if (member_type) { + CompilerType member_clang_type = member_type->GetLayoutCompilerType(); + if (accessibility == eAccessNone) accessibility = default_accessibility; member_accessibilities.push_back(accessibility); uint64_t field_bit_offset = (member_byte_offset == UINT32_MAX ? 0 : (member_byte_offset * 8)); - if (bit_size > 0) { - BitfieldInfo this_field_info; + if (bit_size > 0) { + FieldInfo this_field_info; this_field_info.bit_offset = field_bit_offset; this_field_info.bit_size = bit_size; - ///////////////////////////////////////////////////////////// - // How to locate a field given the DWARF debug information - // - // AT_byte_size indicates the size of the word in which the bit - // offset must be interpreted. - // - // AT_data_member_location indicates the byte offset of the - // word from the base address of the structure. - // - // AT_bit_offset indicates how many bits into the word - // (according to the host endianness) the low-order bit of the - // field starts. AT_bit_offset can be negative. - // - // AT_bit_size indicates the size of the field in bits. - ///////////////////////////////////////////////////////////// - if (data_bit_offset != UINT64_MAX) { this_field_info.bit_offset = data_bit_offset; } else { @@ -2649,8 +2616,9 @@ void DWARFASTParserClang::ParseSingleMember( } if ((this_field_info.bit_offset >= parent_bit_size) || - !last_field_info.NextBitfieldOffsetIsValid( - this_field_info.bit_offset)) { + (last_field_info.IsBitfield() && + !last_field_info.NextBitfieldOffsetIsValid( + this_field_info.bit_offset))) { ObjectFile *objfile = die.GetDWARF()->GetObjectFile(); objfile->GetModule()->ReportWarning( "0x%8.8" PRIx64 ": %s bitfield named \"%s\" has invalid " @@ -2659,40 +2627,12 @@ void DWARFASTParserClang::ParseSingleMember( "compiler and include the preprocessed output for %s\n", die.GetID(), DW_TAG_value_to_name(tag), name, this_field_info.bit_offset, GetUnitName(parent_die).c_str()); - this_field_info.Clear(); return; } // Update the field bit offset we will report for layout field_bit_offset = this_field_info.bit_offset; - // If the member to be emitted did not start on a character - // boundary and there is empty space between the last field and - // this one, then we need to emit an anonymous member filling - // up the space up to its start. There are three cases here: - // - // 1 If the previous member ended on a character boundary, then - // we can emit an - // anonymous member starting at the most recent character - // boundary. - // - // 2 If the previous member did not end on a character boundary - // and the distance - // from the end of the previous member to the current member - // is less than a - // word width, then we can emit an anonymous member starting - // right after the - // previous member and right before this member. - // - // 3 If the previous member did not end on a character boundary - // and the distance - // from the end of the previous member to the current member - // is greater than - // or equal a word width, then we act as in Case 1. - - const uint64_t character_width = 8; - const uint64_t word_width = 32; - // Objective-C has invalid DW_AT_bit_offset values in older // versions of clang, so we have to be careful and only insert // unnamed bitfields if we have a new enough clang. @@ -2704,53 +2644,57 @@ void DWARFASTParserClang::ParseSingleMember( die.GetCU()->Supports_unnamed_objc_bitfields(); if (detect_unnamed_bitfields) { - BitfieldInfo anon_field_info; - - if ((this_field_info.bit_offset % character_width) != - 0) // not char aligned - { - uint64_t last_field_end = 0; - - if (last_field_info.IsValid()) - last_field_end = - last_field_info.bit_offset + last_field_info.bit_size; - - if (this_field_info.bit_offset != last_field_end) { - if (((last_field_end % character_width) == 0) || // case 1 - (this_field_info.bit_offset - last_field_end >= - word_width)) // case 3 - { - anon_field_info.bit_size = - this_field_info.bit_offset % character_width; - anon_field_info.bit_offset = - this_field_info.bit_offset - anon_field_info.bit_size; - } else // case 2 - { - anon_field_info.bit_size = - this_field_info.bit_offset - last_field_end; - anon_field_info.bit_offset = last_field_end; - } - } + clang::Optional unnamed_field_info; + uint64_t last_field_end = 0; + + 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... + // but if it did take up the entire word then we need to extend + // last_field_end so the bit-field does not step into the last + // fields padding. + if (last_field_end != 0 && ((last_field_end % word_width) != 0)) + last_field_end += word_width - (last_field_end % word_width); } - if (anon_field_info.IsValid()) { + // 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 && + !(this_field_info.bit_offset < last_field_end)) { + unnamed_field_info = FieldInfo{}; + unnamed_field_info->bit_size = + this_field_info.bit_offset - last_field_end; + unnamed_field_info->bit_offset = last_field_end; + } + + if (unnamed_field_info) { clang::FieldDecl *unnamed_bitfield_decl = TypeSystemClang::AddFieldToRecordType( class_clang_type, llvm::StringRef(), m_ast.GetBuiltinTypeForEncodingAndBitSize(eEncodingSint, word_width), - accessibility, anon_field_info.bit_size); + accessibility, unnamed_field_info->bit_size); layout_info.field_offsets.insert(std::make_pair( - unnamed_bitfield_decl, anon_field_info.bit_offset)); + unnamed_bitfield_decl, unnamed_field_info->bit_offset)); } } + last_field_info = this_field_info; + last_field_info.SetIsBitfield(true); } else { - last_field_info.Clear(); + last_field_info.bit_offset = field_bit_offset; + + if (llvm::Optional clang_type_size = + member_clang_type.GetByteSize(nullptr)) { + last_field_info.bit_size = *clang_type_size * character_width; + } + + last_field_info.SetIsBitfield(false); } - CompilerType member_clang_type = member_type->GetLayoutCompilerType(); if (!member_clang_type.IsCompleteType()) member_clang_type.GetCompleteType(); @@ -2885,7 +2829,7 @@ bool DWARFASTParserClang::ParseChildMembers( if (!parent_die) return false; - BitfieldInfo last_field_info; + FieldInfo last_field_info; ModuleSP module_sp = parent_die.GetDWARF()->GetObjectFile()->GetModule(); TypeSystemClang *ast = diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h index 2992818..6cd2dc8 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h @@ -170,33 +170,20 @@ protected: lldb::ModuleSP GetModuleForType(const DWARFDIE &die); private: - struct BitfieldInfo { - uint64_t bit_size; - uint64_t bit_offset; + struct FieldInfo { + uint64_t bit_size = 0; + uint64_t bit_offset = 0; + bool is_bitfield = false; - BitfieldInfo() - : bit_size(LLDB_INVALID_ADDRESS), bit_offset(LLDB_INVALID_ADDRESS) {} + FieldInfo() = default; - void Clear() { - bit_size = LLDB_INVALID_ADDRESS; - bit_offset = LLDB_INVALID_ADDRESS; - } - - bool IsValid() const { - return (bit_size != LLDB_INVALID_ADDRESS) && - (bit_offset != LLDB_INVALID_ADDRESS); - } + void SetIsBitfield(bool flag) { is_bitfield = flag; } + bool IsBitfield() { return is_bitfield; } bool NextBitfieldOffsetIsValid(const uint64_t next_bit_offset) const { - if (IsValid()) { - // This bitfield info is valid, so any subsequent bitfields must not - // overlap and must be at a higher bit offset than any previous bitfield - // + size. - return (bit_size + bit_offset) <= next_bit_offset; - } else { - // If the this BitfieldInfo is not valid, then any offset isOK - return true; - } + // Any subsequent bitfields must not overlap and must be at a higher + // bit offset than any previous bitfield + size. + return (bit_size + bit_offset) <= next_bit_offset; } }; @@ -208,7 +195,7 @@ private: lldb::AccessType &default_accessibility, DelayedPropertyList &delayed_properties, lldb_private::ClangASTImporter::LayoutInfo &layout_info, - BitfieldInfo &last_field_info); + FieldInfo &last_field_info); bool CompleteRecordType(const DWARFDIE &die, lldb_private::Type *type, lldb_private::CompilerType &clang_type); -- 2.7.4