Fix the handling of unnamed bit-fields when parsing DWARF

[shafik]

Summary

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.

I removed a duplicate of BitfieldInfo and renamed BitfieldInfo -> FieldInfo since it is being used for both bit-fields and non-bit-fields. I extended TestBitfields.py to cover five more cases we uncovered while fixing this bug.

[aprantl]

Very cool. I think we can improve the code quite a bit while we're here.

lldb/packages/Python/lldbsuite/test/lang/c/bitfields/TestBitfields.py

155

Why don't we inspect the values of fields, too?

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

2427

DW_AT_data_member_location.
AT_foo makes git grep less useful

2674

Can you please add comments explaining what each of these cases handle?

2691

Looks like this could benefit from a refactoring with early exits. Can be a follow-up commit though.

2696

for example, it's not obvious what happened to end up in this else branch

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h

174

Not you fault, but: This data structure is a disaster waiting to happen. Instead of having magic sentinel values, should we use an Optional<FieldInfo> that is always valid if it exists? Or should the members be Optionals?

[teemperor]

(Just some quick comments, will review this properly during normal working hours)

Without this fix debugging Clang with LLDB is essentially impossible, so I'm in favour of landing this with as few pre-commit refactorings as possible to make backporting easier and getting this in ASAP. We probably also want to ping Hans to get this into the 10 release branch (which was created 2 days ago, so that's just a simple cherry-pick).

Also you might want to check out the recently added expect_expr command instead of calling expect (see the inline comment for an example). The API currently doesn't support the whole fuzzy substr matching (which is on purpose) so you might not be able to use it everywhere (at least the current version).

lldb/packages/Python/lldbsuite/test/lang/c/bitfields/TestBitfields.py

207

We can do this since last week: self.expect_expr("(lba.a)", result_type="unsigned int", result_value="2") which is a much more safer and convenient way of doing this.

lldb/packages/Python/lldbsuite/test/lang/c/bitfields/main.c

128

I would prefer generic names as otherwise Clang folks randomly see the test when they grep for usages of these variables by name in the LLVM repo.

[teemperor]

(Jim pointed out that we land this without expect_expr to make back porting easier but somehow Phabricator didn't add his comment to the review here. expect_expr is not yet in the downstream Github branch but it is in the 10 release branch, so that makes sense to me).

Anyway, some more points from my side:

1. I actually thought this would fix the crashes in CGRecordLowering::lower() that we kept seeing, but I've been dogfooding this patch today and I'm still getting the crashes when dumping arbitrary clang Decls from LLDB:

3  libsystem_platform.dylib 0x0000000102881000 _sigtramp + 18446603342990035968
4  liblldb.11.0.0git.dylib  0x0000000109d8eb3b (anonymous namespace)::CGRecordLowering::lower(bool) + 14555
5  liblldb.11.0.0git.dylib  0x0000000109d89aa9 clang::CodeGen::CodeGenTypes::ComputeRecordLayout(clang::RecordDecl const*, llvm::StructType*) + 249
6  liblldb.11.0.0git.dylib  0x0000000109e948b1 clang::CodeGen::CodeGenTypes::ConvertRecordDeclType(clang::RecordDecl const*) + 785
7  liblldb.11.0.0git.dylib  0x0000000109e91800 clang::CodeGen::CodeGenTypes::ConvertType(clang::QualType) + 2048
8  liblldb.11.0.0git.dylib  0x0000000109d8f269 (anonymous namespace)::CGRecordLowering::getStorageType(clang::FieldDecl const*) + 41
9  liblldb.11.0.0git.dylib  0x0000000109d8c9f3 (anonymous namespace)::CGRecordLowering::lower(bool) + 6035
10 liblldb.11.0.0git.dylib  0x0000000109d89aa9 clang::CodeGen::CodeGenTypes::ComputeRecordLayout(clang::RecordDecl const*, llvm::StructType*) + 249
11 liblldb.11.0.0git.dylib  0x0000000109e948b1 clang::CodeGen::CodeGenTypes::ConvertRecordDeclType(clang::RecordDecl const*) + 785
12 liblldb.11.0.0git.dylib  0x0000000109e91313 clang::CodeGen::CodeGenTypes::ConvertType(clang::QualType) + 787
13 liblldb.11.0.0git.dylib  0x0000000109d8f269 (anonymous namespace)::CGRecordLowering::getStorageType(clang::FieldDecl const*) + 41
14 liblldb.11.0.0git.dylib  0x0000000109d8c9f3 (anonymous namespace)::CGRecordLowering::lower(bool) + 6035
15 liblldb.11.0.0git.dylib  0x0000000109d89aa9 clang::CodeGen::CodeGenTypes::ComputeRecordLayout(clang::RecordDecl const*, llvm::StructType*) + 249
16 liblldb.11.0.0git.dylib  0x0000000109e948b1 clang::CodeGen::CodeGenTypes::ConvertRecordDeclType(clang::RecordDecl const*) + 785
17 liblldb.11.0.0git.dylib  0x0000000109e91800 clang::CodeGen::CodeGenTypes::ConvertType(clang::QualType) + 2048
18 liblldb.11.0.0git.dylib  0x0000000109f0a22a (anonymous namespace)::X86_64ABIInfo::classifyArgumentType(clang::QualType, unsigned int, unsigned int&, unsigned int&, bool) const + 634
19 liblldb.11.0.0git.dylib  0x0000000109f07515 (anonymous namespace)::X86_64ABIInfo::computeInfo(clang::CodeGen::CGFunctionInfo&) const + 2117
20 liblldb.11.0.0git.dylib  0x0000000109b513f6 clang::CodeGen::CodeGenTypes::arrangeLLVMFunctionInfo(clang::CanQual<clang::Type>, bool, bool, llvm::ArrayRef<clang::CanQual<clang::Type> >, clang::FunctionType::ExtInfo, llvm::ArrayRef<clang::FunctionType::ExtParameterInfo>, clang::CodeGen::RequiredArgs) + 2822
21 liblldb.11.0.0git.dylib  0x0000000109b51aec arrangeLLVMFunctionInfo(clang::CodeGen::CodeGenTypes&, bool, llvm::SmallVectorImpl<clang::CanQual<clang::Type> >&, clang::CanQual<clang::FunctionProtoType>) + 700
22 liblldb.11.0.0git.dylib  0x0000000109b51ba9 clang::CodeGen::CodeGenTypes::arrangeCXXMethodType(clang::CXXRecordDecl const*, clang::FunctionProtoType const*, clang::CXXMethodDecl const*) + 153
23 liblldb.11.0.0git.dylib  0x0000000109b51e0d clang::CodeGen::CodeGenTypes::arrangeCXXMethodDeclaration(clang::CXXMethodDecl const*) + 525
24 liblldb.11.0.0git.dylib  0x0000000109e15831 clang::CodeGen::CodeGenModule::EmitGlobalFunctionDefinition(clang::GlobalDecl, llvm::GlobalValue*) + 177
25 liblldb.11.0.0git.dylib  0x0000000109e0b48b clang::CodeGen::CodeGenModule::EmitGlobalDefinition(clang::GlobalDecl, llvm::GlobalValue*) + 2235
26 liblldb.11.0.0git.dylib  0x0000000109e19996 clang::CodeGen::CodeGenModule::EmitTopLevelDecl(clang::Decl*) + 150

So either there is another corner case not handled in this patch, we have another bug that screws up our layout or my local build is broken (I already dropped every patch beside this one and rebuild, so the last one is unlikely). Or I was mistaken what actually was causing this crash.

2. I'm still kinda confused by some parts of the code but the undocumented in/out last_field_info thing we did before was also confusing, so if this gets bitfields working then I'm fine with getting this in. However I wish we didn't add another in/out parameter by now having a normal field info and a field info for bitfields. If we could model this as one FieldInfo with a is_bitfield flag or so that would be preferred.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

2393

IIUC then those two bitfield info variables are mutually exclusive? I.e., either the last field was bitfield or a normal field? If yes, would it make sense to model this as one FieldInfo variable and have a bool value for differentiating between them? That way we don't have the possibility that we forget to clear one when we set the other.

2838

unrelated?

2842

This code looks like we forgot to initialise the last_bitfield_info fields and would IMHO also be more readable if we had a bool value like is_bitfield etc.

[teemperor]

I just went back to the radar we had about the CG crash and this *does* fix that issue (rdar://53932023). I could also not reproduce the CG crash again so I assume that was something else that fiddled around in that area. So LGTM minus the thing with the last_bitfield_info last_field_info duality which I would prefer we could get rid of before landing (but it's not a must and can be done as a follow-up commit).

[shafik]

Realized that the assert that I was hitting in some cases only reproduced using C++ for example one case we needed to use a class w/ private members. So created a new bit-field test which is C++ specific.

Addressed comments:

• Removed magic numbers from FieldInfo
• Removed the need for last_field_info and last_bitfield_info
• Refactored away more code

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

2393

It was not obvious during the initial refactoring wave but yes, I removed one of them.

2674

I removed a lot of in the next wave of refactoring.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h

174

This is a good point, it took a bit more refactoring of the code using this but I removed the magic numbers and used an Optional<FieldInfo> for the unnamed_field_info case.

[teemperor]

LGTM modulo some minor points regarding the test. The refactoring of the parsing code and using expect_expr can be done as NFC follow-ups. Thanks for the patch, great work!

lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/TestCppBitfields.py

32 ↗

(On Diff #239732)

All the setup code can just be:

self.build()

lldbutil.run_to_source_breakpoint(self, '// Set break point at this line.',
        lldb.SBFileSpec("main.cpp", False))

lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/main.cpp

3 ↗

(On Diff #239732)

Do we need stdio and string.h?

82 ↗

(On Diff #239732)

//// instead of //.

[shafik]

• Addressing minor comments
• Adding fix for TestObjCIvarOffsets.py we need to guard last_field_info.NextBitfieldOffsetIsValid(...) with a check to make sure we have a bit-field. Objective C treats non-bitfields as all having offset zero.

[aprantl]

lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/TestCppBitfields.py

15 ↗

(On Diff #240020)

Is this needed?

17 ↗

(On Diff #240020)

Is this needed?

lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/main.cpp

3 ↗

(On Diff #239732)

Since it's only used in bitfields we probably don't even need uint64_t? It speeds up compilation of the testcase quite a bit if the std module doesn't have to be built.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h

189

This is personal taste, but for a struct I probably wouldn't even bother writing a trivial accessor. Up to you.

[labath]

lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/main.cpp

76 ↗

(On Diff #240020)

BTW, I've changed this to '\0' because the memory following this array is uninitialized, and that would confuse the the "const char *" pretty printer, which would continue to print the garbage after the array. The actual value of arr does not seem to be relevant for this test (I hope), but the pretty printer behavior is suboptimal, so I've filed pr44649 to track that.

[shafik]

lldb/packages/Python/lldbsuite/test/lang/cpp/bitfields/main.cpp

76 ↗

(On Diff #240020)

Thank you for catching that and patching!

lldb/packages/Python/lldbsuite/test/lang/c/bitfields/TestBitfields.py

def test_and_run_command(self):
                 '(uint8_t:1) b15 = \'\\0\'',
                 '(uint8_t:1) b16 = \'\\0\'',
                 '(uint8_t:1) b17 = \'\\0\'',
                 ])
 
         self.expect("v/x large_packed", VARIABLES_DISPLAYED_CORRECTLY,
                     substrs=["a = 0x0000000cbbbbaaaa", "b = 0x0000000dffffeee"])
 
+        self.expect(
+            "frame variable --show-types lba",
+            VARIABLES_DISPLAYED_CORRECTLY,
+            substrs=[
+                '(int:32)  = ',
+                '(unsigned int:20) a =',
+                ])
+
+        self.expect(
+            "frame variable --show-types lbb",
+            VARIABLES_DISPLAYED_CORRECTLY,
+            substrs=[
+                '(unsigned int:1) a =',
+                '(int:31)  =',
+                '(unsigned int:20) b =',
+                ])
+
+        self.expect(
+            "frame variable --show-types lbc",
+            VARIABLES_DISPLAYED_CORRECTLY,
+            substrs=[
+                '(int:22)  =',
+                '(unsigned int:1) a =',
+                '(unsigned int:1) b =',
+                '(unsigned int:5) c =',
+                '(unsigned int:1) d =',
+                '(int:2)  =',
+                '(unsigned int:20) e =',
+                ])
+
+        self.expect(
+            "frame variable --show-types lbd",
+            VARIABLES_DISPLAYED_CORRECTLY,
+            substrs=[
+                '(char [3]) arr =',
+                '(int:32)  =',
+                '(unsigned int:20) a =',
+                ])
+
+        self.expect(
+            "frame variable --show-types clang_example",
+            VARIABLES_DISPLAYED_CORRECTLY,
+            substrs=[
+                   '(int:22)  =',
+                   '(uint64_t:1) HasFlexibleArrayMember =',
+                   '(uint64_t:1) AnonymousStructOrUnion =',
+                   '(uint64_t:1) HasObjectMember =',
+                   '(uint64_t:1) HasVolatileMember =',
+                   '(uint64_t:1) LoadedFieldsFromExternalStorage =',
+                   '(uint64_t:1) NonTrivialToPrimitiveDefaultInitialize =',
+                   '(uint64_t:1) NonTrivialToPrimitiveCopy =',
+                   '(uint64_t:1) NonTrivialToPrimitiveDestroy =',
+                   '(uint64_t:1) HasNonTrivialToPrimitiveDefaultInitializeCUnion =',
+                   '(uint64_t:1) HasNonTrivialToPrimitiveDestructCUnion =',
+                   '(uint64_t:1) HasNonTrivialToPrimitiveCopyCUnion =',
+                ])
+
+        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.HasFlexibleArrayMember)", VARIABLES_DISPLAYED_CORRECTLY,
+                    substrs=['uint64_t', '1'])
+
 
     @add_test_categories(['pyapi'])
     # BitFields exhibit crashes in record layout on Windows
     # (http://llvm.org/pr21800)
     @skipIfWindows
     def test_and_python_api(self):
         """Use Python APIs to inspect a bitfields variable."""
         self.build()

lldb/packages/Python/lldbsuite/test/lang/c/bitfields/main.c

 //===-- main.c --------------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #include <stdint.h>
 #include <stdio.h>
 #include <string.h>
 
-int main (int argc, char const *argv[])
-{
-    struct Bits
-    {
-        uint32_t    : 1, // Unnamed bitfield
+int main(int argc, char const *argv[]) {
+  struct Bits {
                     b1 : 1,
                     b2 : 2,
                     : 2, // Unnamed bitfield
                     b3 : 3,
                     : 2, // Unnamed bitfield (this will get removed)
                     b4 __attribute__ ((aligned(16))),
                     b5 : 5,
                     b6 : 6,
                     b7 : 7,
                     four : 4;
-    };
 
-    printf("%lu", sizeof(struct Bits));
 
-    struct Bits bits;
-    int i;
-    for (i=0; i<(1<<1); i++)
-        bits.b1 = i;        //// break $source:$line
-    for (i=0; i<(1<<2); i++)
-        bits.b2 = i;        //// break $source:$line
-    for (i=0; i<(1<<3); i++)
-        bits.b3 = i;        //// break $source:$line
-    for (i=0; i<(1<<4); i++)
-        bits.b4 = i;        //// break $source:$line
-    for (i=0; i<(1<<5); i++)
-        bits.b5 = i;        //// break $source:$line
-    for (i=0; i<(1<<6); i++)
-        bits.b6 = i;        //// break $source:$line
-    for (i=0; i<(1<<7); i++)
-        bits.b7 = i;        //// break $source:$line
-    for (i=0; i<(1<<4); i++)
-        bits.four = i;      //// break $source:$line
+  for (i = 0; i < (1 << 1); i++)
+    bits.b1 = i; //// break $source:$line
+  for (i = 0; i < (1 << 2); i++)
+    bits.b2 = i; //// break $source:$line
+  for (i = 0; i < (1 << 3); i++)
+    bits.b3 = i; //// break $source:$line
+  for (i = 0; i < (1 << 4); i++)
+    bits.b4 = i; //// break $source:$line
+  for (i = 0; i < (1 << 5); i++)
+    bits.b5 = i; //// break $source:$line
+  for (i = 0; i < (1 << 6); i++)
+    bits.b6 = i; //// break $source:$line
+  for (i = 0; i < (1 << 7); i++)
+    bits.b7 = i; //// break $source:$line
+  for (i = 0; i < (1 << 4); i++)
+    bits.four = i; //// break $source:$line
 
-    struct MoreBits
-    {
-        uint32_t    a : 3;
-        uint8_t       : 1;
-        uint8_t     b : 1;
-        uint8_t     c : 1;
-        uint8_t     d : 1;
-    };
+  struct MoreBits {
+    uint32_t a : 3;
+    uint8_t : 1;
+    uint8_t b : 1;
+    uint8_t c : 1;
+    uint8_t d : 1;
 
-    struct MoreBits more_bits;
 
-    more_bits.a = 3;
-    more_bits.b = 0;
-    more_bits.c = 1;
-    more_bits.d = 0;
 
-    struct EvenMoreBits
-    {
-        uint8_t b1  : 1, b2  : 1, b3  : 1, b4  : 1, b5  : 1, b6  : 1,
-                b7  : 1, b8  : 1, b9  : 1, b10 : 1, b11 : 1, b12 : 1,
-                b13 : 1, b14 : 1, b15 : 1, b16 : 1, b17 : 1;
-    };
+  struct EvenMoreBits {
+    uint8_t b1 : 1, b2 : 1, b3 : 1, b4 : 1, b5 : 1, b6 : 1, b7 : 1, b8 : 1,
+        b9 : 1, b10 : 1, b11 : 1, b12 : 1, b13 : 1, b14 : 1, b15 : 1, b16 : 1,
+        b17 : 1;
 
-    struct EvenMoreBits even_more_bits;
-    memset(&even_more_bits, 0, sizeof(even_more_bits));
-    even_more_bits.b1 = 1;
-    even_more_bits.b5 = 1;
-    even_more_bits.b7 = 1;
-    even_more_bits.b13 = 1;
 
 #pragma pack(1)
-    struct PackedBits
-    {
-        char a;
-      uint32_t b : 5,
-                 c : 27;
-    };
-#pragma pack()  
-    struct PackedBits packed;
-    packed.a = 'a';
-    packed.b = 10;
-    packed.c = 0x7112233;
+  struct PackedBits {
+    uint32_t b : 5, c : 27;
+#pragma pack()
 
-    struct LargePackedBits {
-        uint64_t a: 36;
-        uint64_t b: 36;
-    } __attribute__((packed));
+    uint64_t a : 36;
+    uint64_t b : 36;
 
-    struct LargePackedBits large_packed =
-      (struct LargePackedBits){ 0xcbbbbaaaa, 0xdffffeeee };
-    
-    return 0;               //// Set break point at this line.
+      (struct LargePackedBits){0xcbbbbaaaa, 0xdffffeeee};
+  struct LargerBitsA {
+    unsigned int : 30, a : 20;
+  } lba;
+
+  struct LargerBitsB {
+    unsigned int a : 1, : 11, : 12, b : 20;
+  } lbb;
+
+  struct LargerBitsC {
+    unsigned int : 13, : 9, a : 1, b : 1, c : 5, d : 1, e : 20;
+  } lbc;
+
+  struct LargerBitsD {
+    char arr[3];
+    unsigned int : 30, a : 20;
+  } lbd;
+
+  // This case came up when debugging clang
+  struct BitExampleFromClangDeclContext {
+    struct fields {
+      uint64_t : 13;
+      uint64_t : 9;
+
+      uint64_t HasFlexibleArrayMember : 1;
+      uint64_t AnonymousStructOrUnion : 1;
+      uint64_t HasObjectMember : 1;
+      uint64_t HasVolatileMember : 1;
+      uint64_t LoadedFieldsFromExternalStorage : 1;
+      uint64_t NonTrivialToPrimitiveDefaultInitialize : 1;
+      uint64_t NonTrivialToPrimitiveCopy : 1;
+      uint64_t NonTrivialToPrimitiveDestroy : 1;
+      uint64_t HasNonTrivialToPrimitiveDefaultInitializeCUnion : 1;
+      uint64_t HasNonTrivialToPrimitiveDestructCUnion : 1;
+      uint64_t HasNonTrivialToPrimitiveCopyCUnion : 1;
+    };
+
+    union {
+      struct fields f;
+    };
+  } clang_example;
+
+  lba.a = 2;
+  lbb.b = 3;
+  lbc.c = 4;
+  lbd.a = 5;
+  clang_example.f.HasFlexibleArrayMember = 1;
 
+  return 0; //// Set break point at this line.
 }

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h

lldb::TypeSP ParseTypeFromClangModule(const lldb_private::SymbolContext &sc,
                                         const DWARFDIE &die,
                                         lldb_private::Log *log);
 
   // Return true if this type is a declaration to a type in an external
   // module.
   lldb::ModuleSP GetModuleForType(const DWARFDIE &die);
 
 private:
-  struct BitfieldInfo {
+  struct FieldInfo {
     uint64_t bit_size;
     uint64_t bit_offset;
 
-    BitfieldInfo()
+    FieldInfo()
         : 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
+        // If the this FieldInfo is not valid, then any offset isOK
         return true;
       }
     }
   };
 
   void
   ParseSingleMember(const DWARFDIE &die, const DWARFDIE &parent_die,
                     lldb_private::CompilerType &class_clang_type,
                     const lldb::LanguageType class_language,
                     std::vector<int> &member_accessibilities,
                     lldb::AccessType &default_accessibility,
                     DelayedPropertyList &delayed_properties,
                     lldb_private::ClangASTImporter::LayoutInfo &layout_info,
-                    BitfieldInfo &last_field_info);
+                    FieldInfo &last_bitfield_info, FieldInfo &last_field_info);
 
   bool CompleteRecordType(const DWARFDIE &die, lldb_private::Type *type,
                           lldb_private::CompilerType &clang_type);
   bool CompleteEnumType(const DWARFDIE &die, lldb_private::Type *type,
                         lldb_private::CompilerType &clang_type);
 
   lldb::TypeSP ParseTypeModifier(const lldb_private::SymbolContext &sc,
                                  const DWARFDIE &die,

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

static bool DeclKindIsCXXClass(clang::Decl::Kind decl_kind) {
   case clang::Decl::ClassTemplateSpecialization:
     return true;
   default:
     break;
   }
   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) {
     m_clang_ast_importer_up.reset(new ClangASTImporter);
   }
   return *m_clang_ast_importer_up;
 }
 

 void DWARFASTParserClang::ParseSingleMember(
     const DWARFDIE &die, const DWARFDIE &parent_die,
     lldb_private::CompilerType &class_clang_type,
     const lldb::LanguageType class_language,
     std::vector<int> &member_accessibilities,
     lldb::AccessType &default_accessibility,
     DelayedPropertyList &delayed_properties,
     lldb_private::ClangASTImporter::LayoutInfo &layout_info,
-    BitfieldInfo &last_field_info) {
+    FieldInfo &last_bitfield_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
   const uint64_t parent_byte_size =
       parent_die.GetAttributeValueAsUnsigned(DW_AT_byte_size, UINT64_MAX);
   const uint64_t parent_bit_size =
       parent_byte_size == UINT64_MAX ? UINT64_MAX : parent_byte_size * 8;
 
if (num_attributes > 0) {
     size_t bit_size = 0;
     bool is_external =
         false; // On DW_TAG_members, this means the member is static
     uint32_t i;
     for (i = 0; i < num_attributes && !is_artificial; ++i) {
       const dw_attr_t attr = attributes.AttributeAtIndex(i);
       DWARFFormValue form_value;
       if (attributes.ExtractFormValueAtIndex(i, form_value)) {
+        // 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.
         switch (attr) {
         case DW_AT_name:
           name = form_value.AsCString();
           break;
         case DW_AT_type:
           encoding_form = form_value;
           break;
         case DW_AT_bit_offset:
if (is_external && member_byte_offset == UINT32_MAX) {
       }
       return;
     }
 
     if (!is_artificial) {
       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 {
               if (!byte_size)
                 byte_size = member_type->GetByteSize();
 
               ObjectFile *objfile = die.GetDWARF()->GetObjectFile();
               if (objfile->GetByteOrder() == eByteOrderLittle) {
                 this_field_info.bit_offset += byte_size.getValueOr(0) * 8;
                 this_field_info.bit_offset -= (bit_offset + bit_size);
               } else {
                 this_field_info.bit_offset += bit_offset;
               }
             }
 
             if ((this_field_info.bit_offset >= parent_bit_size) ||
-                !last_field_info.NextBitfieldOffsetIsValid(
+                !last_bitfield_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 "
                   "bit offset (0x%8.8" PRIx64
                   ") member will be ignored. Please file a bug against the "
                   "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.
             bool detect_unnamed_bitfields = true;
 
             if (class_language == eLanguageTypeObjC ||
                 class_language == eLanguageTypeObjC_plus_plus)
               detect_unnamed_bitfields =
                   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;
+              FieldInfo unnamed_field_info;
 
-                if (last_field_info.IsValid())
-                  last_field_end =
-                      last_field_info.bit_offset + last_field_info.bit_size;
+              // The last field was not a bit-field...
+              if (last_field_info.IsValid()) {
 
-                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;
-                  }
-                }
+                // 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);
+              } else if (last_bitfield_info.IsValid())
+                last_field_end =
+                    last_bitfield_info.bit_offset + last_bitfield_info.bit_size;
+
+              // 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.bit_size =
+                unnamed_field_info.bit_offset = last_field_end;
               }
 
-              if (anon_field_info.IsValid()) {
+              if (unnamed_field_info.IsValid()) {
+                if (data_bit_offset != UINT64_MAX)
+                  field_bit_offset = data_bit_offset;
+                else
+                  field_bit_offset = unnamed_field_info.bit_offset +
+                                     unnamed_field_info.bit_size;
+
                 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));
+              } else
+                field_bit_offset = this_field_info.bit_offset;
             }
-            last_field_info = this_field_info;
-          } else {
+
             last_field_info.Clear();
+            last_bitfield_info = this_field_info;
+          } else {
+            last_bitfield_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;
+            }
           }
 
-          CompilerType member_clang_type = member_type->GetLayoutCompilerType();
           if (!member_clang_type.IsCompleteType())
             member_clang_type.GetCompleteType();
 
           {
             // Older versions of clang emit array[0] and array[1] in the
             // same way (<rdar://problem/12566646>). If the current field
             // is at the end of the structure, then there is definitely no
             // room for extra elements and we override the type to
if (!is_artificial) {
             }
           }
 
           field_decl = ClangASTContext::AddFieldToRecordType(
               class_clang_type, name, member_clang_type, accessibility,
               bit_size);
 
           m_ast.SetMetadataAsUserID(field_decl, die.GetID());
-
           layout_info.field_offsets.insert(
               std::make_pair(field_decl, field_bit_offset));
         } else {
           if (name)
             module_sp->ReportError(
                 "0x%8.8" PRIx64 ": DW_TAG_member '%s' refers to type 0x%8.8x"
                 " which was unable to be parsed",
                 die.GetID(), name, encoding_form.Reference().GetOffset());
bool DWARFASTParserClang::ParseChildMembers(
     std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> &base_classes,
     std::vector<int> &member_accessibilities,
     std::vector<DWARFDIE> &member_function_dies,
     DelayedPropertyList &delayed_properties, AccessType &default_accessibility,
     bool &is_a_class, ClangASTImporter::LayoutInfo &layout_info) {
   if (!parent_die)
     return false;
 
-  BitfieldInfo last_field_info;
+  FieldInfo last_bitfield_info;
+  FieldInfo last_field_info;
+  last_field_info.bit_offset = 0;
+  last_field_info.bit_size = 0;
 
   ModuleSP module_sp = parent_die.GetDWARF()->GetObjectFile()->GetModule();
   ClangASTContext *ast =
       llvm::dyn_cast_or_null<ClangASTContext>(class_clang_type.GetTypeSystem());
   if (ast == nullptr)
     return false;
 
   for (DWARFDIE die = parent_die.GetFirstChild(); die.IsValid();
        die = die.GetSibling()) {
     dw_tag_t tag = die.Tag();
 
     switch (tag) {
     case DW_TAG_member:
     case DW_TAG_APPLE_property:
       ParseSingleMember(die, parent_die, class_clang_type, class_language,
                         member_accessibilities, default_accessibility,
-                        delayed_properties, layout_info, last_field_info);
+                        delayed_properties, layout_info, last_bitfield_info,
+                        last_field_info);
       break;
 
     case DW_TAG_subprogram:
       // Let the type parsing code handle this one for us.
       member_function_dies.push_back(die);
       break;
 
     case DW_TAG_inheritance: {