[llvm-branch-commits] [lldb] b5cf892 - [LLDB] Fix the handling of unnamed bit-fields when parsing DWARF

Hans Wennborg via llvm-branch-commits llvm-branch-commits at lists.llvm.org
Mon Jan 27 06:10:28 PST 2020


Author: shafik
Date: 2020-01-27T15:10:11+01:00
New Revision: b5cf892651812003e64c4a8f0dbf81f74a499016

URL: https://github.com/llvm/llvm-project/commit/b5cf892651812003e64c4a8f0dbf81f74a499016
DIFF: https://github.com/llvm/llvm-project/commit/b5cf892651812003e64c4a8f0dbf81f74a499016.diff

LOG: [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

(cherry picked from commit fcaf5f6c01a09f23b948afb8c91c4dd951d4525e)

Added: 
    lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/Makefile
    lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/TestCppBitfields.py
    lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/main.cpp

Modified: 
    lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
    lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h

Removed: 
    


################################################################################
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 000000000000..99998b20bcb0
--- /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 000000000000..696e5647f13f
--- /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 000000000000..975c0f05683c
--- /dev/null
+++ b/lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/main.cpp
@@ -0,0 +1,81 @@
+#include <stdint.h>
+
+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 232063a6f339..6166aa77bda4 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<FieldInfo> 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 =
                     ClangASTContext::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<uint64_t> 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();
   ClangASTContext *ast =

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
index 4ad757247c3e..8a78299c8b10 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -170,33 +170,20 @@ class DWARFASTParserClang : public DWARFASTParser {
   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 @@ class DWARFASTParserClang : public DWARFASTParser {
                     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);


        


More information about the llvm-branch-commits mailing list