[lldb][DWARFASTParserClang] Prevent unnamed bitfield creation in the presence of overlapping fields

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

@@ -3024,7 +3024,6 @@ void DWARFASTParserClang::ParseSingleMember(
   }
 
   const uint64_t character_width = 8;
-  const uint64_t word_width = 32;
   CompilerType member_clang_type = member_type->GetLayoutCompilerType();
 
   const auto accessibility = attrs.accessibility == eAccessNone
@@ -3092,52 +3091,29 @@ void DWARFASTParserClang::ParseSingleMember(
       detect_unnamed_bitfields =
           die.GetCU()->Supports_unnamed_objc_bitfields();
 
-    if (detect_unnamed_bitfields) {
-      std::optional<FieldInfo> 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...
-        // 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 (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;
-        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, unnamed_field_info->bit_size);
-
-        layout_info.field_offsets.insert(std::make_pair(
-            unnamed_bitfield_decl, unnamed_field_info->bit_offset));
-      }
-    }
+    if (detect_unnamed_bitfields)
+      AddUnnamedBitfieldToRecordTypeIfNeeded(layout_info, class_clang_type,
+                                             last_field_info, this_field_info);
 
     last_field_info = this_field_info;
     last_field_info.SetIsBitfield(true);
   } else {
-    last_field_info.bit_offset = field_bit_offset;
+    FieldInfo this_field_info;
+    this_field_info.is_bitfield = false;
+    this_field_info.bit_offset = field_bit_offset;
 
+    // TODO: we shouldn't silently ignore the bit_size if we fail
+    //       to GetByteSize.
     if (std::optional<uint64_t> clang_type_size =
             llvm::expectedToOptional(member_type->GetByteSize(nullptr))) {
-      last_field_info.bit_size = *clang_type_size * character_width;
+      this_field_info.bit_size = *clang_type_size * character_width;
     }
 
-    last_field_info.SetIsBitfield(false);
+    if (this_field_info.GetFieldEnd() <= last_field_info.GetEffectiveFieldEnd())
+      this_field_info.SetEffectiveFieldEnd(
+          last_field_info.GetEffectiveFieldEnd());
+
+    last_field_info = this_field_info;
   }
 
   // Don't turn artificial members such as vtable pointers into real FieldDecls
@@ -3937,6 +3913,43 @@ bool DWARFASTParserClang::ShouldCreateUnnamedBitfield(
   return true;
 }
 
+void DWARFASTParserClang::AddUnnamedBitfieldToRecordTypeIfNeeded(
+    ClangASTImporter::LayoutInfo &class_layout_info,
+    const CompilerType &class_clang_type, const FieldInfo &previous_field,
+    const FieldInfo &current_field) {
+  // TODO: get this value from target
+  const uint64_t word_width = 32;
+  uint64_t last_field_end = previous_field.GetEffectiveFieldEnd();
+
+  if (!previous_field.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);
+  }
+
+  // Nothing to be done.
+  if (!ShouldCreateUnnamedBitfield(previous_field, last_field_end,
+                                   current_field, class_layout_info))
+    return;
+
+  // Place the unnamed bitfield into the gap between the previous field's end
+  // and the current field's start.
+  const uint64_t unnamed_bit_size = current_field.bit_offset - last_field_end;
+  const uint64_t unnamed_bit_offset = last_field_end;
+
+  clang::FieldDecl *unnamed_bitfield_decl =
+      TypeSystemClang::AddFieldToRecordType(
+          class_clang_type, llvm::StringRef(),
+          m_ast.GetBuiltinTypeForEncodingAndBitSize(eEncodingSint, word_width),
+          lldb::AccessType::eAccessPublic, unnamed_bit_size);
+
+  class_layout_info.field_offsets.insert(
+      std::make_pair(unnamed_bitfield_decl, unnamed_bit_offset));
+}
+
 void DWARFASTParserClang::ParseRustVariantPart(
     DWARFDIE &die, const DWARFDIE &parent_die, CompilerType &class_clang_type,
     const lldb::AccessType default_accesibility,

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

@@ -270,15 +270,33 @@ class DWARFASTParserClang : public lldb_private::plugin::dwarf::DWARFASTParser {
 
 private:
   struct FieldInfo {
+    /// Size in bits that this field occupies. Can but
+    /// need not be the DW_AT_bit_size of the field.
     uint64_t bit_size = 0;
+
+    /// Offset of this field in bits from the beginning
+    /// of the containing struct. Can but need not
+    /// be the DW_AT_data_bit_offset of the field.
     uint64_t bit_offset = 0;
+
+    /// In case this field is folded into the storage
+    /// of a previous member's storage (for example
+    /// with [[no_unique_address]]), the effective field
+    /// end is the offset in bits from the beginning of
+    /// the containing struct where the field we were
+    /// folded into ended.
+    std::optional<uint64_t> effective_field_end;
+
+    /// Set to 'true' if this field is a bit-field.
     bool is_bitfield = false;
+
+    /// Set to 'true' if this field is DW_AT_artificial.
     bool is_artificial = false;
 
     FieldInfo() = default;
 
     void SetIsBitfield(bool flag) { is_bitfield = flag; }
-    bool IsBitfield() { return is_bitfield; }
+    bool IsBitfield() const { return is_bitfield; }
 
     void SetIsArtificial(bool flag) { is_artificial = flag; }
     bool IsArtificial() const { return is_artificial; }
@@ -288,6 +306,19 @@ class DWARFASTParserClang : public lldb_private::plugin::dwarf::DWARFASTParser {
       // bit offset than any previous bitfield + size.
       return (bit_size + bit_offset) <= next_bit_offset;
     }
+
+    /// Returns the offset in bits of where the storage this field
+    /// occupies ends.
+    uint64_t GetFieldEnd() const { return bit_size + bit_offset; }
+
+    void SetEffectiveFieldEnd(uint64_t val) { effective_field_end = val; }
+
+    /// If this field was folded into storage of a previous field,
+    /// returns the offset in bits of where that storage ends. Otherwise,
+    /// returns the regular field end (see \ref GetFieldEnd).
+    uint64_t GetEffectiveFieldEnd() const {
+      return effective_field_end.value_or(GetFieldEnd());
+    }
   };
 
   /// Parsed form of all attributes that are relevant for parsing type members.
@@ -330,6 +361,35 @@ class DWARFASTParserClang : public lldb_private::plugin::dwarf::DWARFASTParser {
       FieldInfo const &this_field_info,
       lldb_private::ClangASTImporter::LayoutInfo const &layout_info) const;
 
+  /// Tries to detect whether \ref class_clang_type contained an unnamed
+  /// bit-field between \ref previous_field and \ref current_field, and if
+  /// so, adds a clang::FieldDecl representing that bit-field to
+  /// \ref class_clang_type.
+  ///
+  /// This is necessary because Clang (and GCC) doesn't emit a DW_TAG_member
+  /// entry for unnamed bit-fields. So we derive it (with some exceptions),
+  /// by checking whether there is a gap between where the storage of a
+  /// DW_TAG_member ended and the subsequent DW_TAG_member began.
+  ///
+  /// \param[in,out] layout_info Layout information of all decls parsed by the
+  ///                            current parser. Will contain an entry for
+  ///                            the unnamed bit-field if this function created
+  ///                            one.
+  ///
+  /// \param[in] class_clang_type The RecordType to which the unnamed bit-field
+  ///                             will be added (if any).
+  ///
+  /// \param[in] previous_field FieldInfo of the previous DW_TAG_member
+  ///                           we parsed.
+  ///
+  /// \param[in] current_field FieldInfo of the current DW_TAG_member
+  ///                          being parsed.
+  ///
+  void AddUnnamedBitfieldToRecordTypeIfNeeded(
+      lldb_private::ClangASTImporter::LayoutInfo &class_layout_info,
+      const lldb_private::CompilerType &class_clang_type,
+      const FieldInfo &previous_field, const FieldInfo &current_field);
+
   /// Parses a DW_TAG_APPLE_property DIE and appends the parsed data to the
   /// list of delayed Objective-C properties.
   ///

lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/string/TestDataFormatterLibcxxStringSimulator.py

@@ -13,9 +13,6 @@
 class LibcxxStringDataFormatterSimulatorTestCase(TestBase):
     NO_DEBUG_INFO_TESTCASE = True
 
-    @skipIfDarwin
-    @skipIfWindows
-    @skipIfLinux
     def _run_test(self, defines):
         cxxflags_extras = " ".join(["-D%s" % d for d in defines])
         self.build(dictionary=dict(CXXFLAGS_EXTRAS=cxxflags_extras))

lldb/test/Shell/SymbolFile/DWARF/no_unique_address-with-bitfields.cpp

@@ -0,0 +1,110 @@
+// RUN: %clang --target=x86_64-apple-macosx -c -gdwarf -o %t %s
+// RUN: %lldb %t \
+// RUN:   -o "target var global" \
+// RUN:   -o "target var global2" \
+// RUN:   -o "target var global3" \
+// RUN:   -o "target var global4" \
+// RUN:   -o "target var global5" \
+// RUN:   -o "image dump ast" \
+// RUN:   -o exit | FileCheck %s
+
+// CHECK:      (lldb) image dump ast
+// CHECK:      CXXRecordDecl {{.*}} struct Foo definition
+// CHECK:      |-FieldDecl {{.*}} data 'char[5]'
+// CHECK-NEXT: |-FieldDecl {{.*}} padding 'Empty'
+// CHECK-NEXT: `-FieldDecl {{.*}} flag 'unsigned long'
+// CHECK-NEXT:   `-IntegerLiteral {{.*}} 'int' 1
+
+struct Empty {};
+struct Empty2 {};
+struct Empty3 {};
+
+struct Foo {
+  char data[5];
+  [[no_unique_address]] Empty padding;
+  unsigned long flag : 1;
+};
+
+Foo global;
+
+// CHECK:      CXXRecordDecl {{.*}} struct ConsecutiveOverlap definition
+// CHECK:      |-FieldDecl {{.*}} data 'char[5]'
+// CHECK-NEXT: |-FieldDecl {{.*}} p1 'Empty'
+// CHECK-NEXT: |-FieldDecl {{.*}} p2 'Empty2'
+// CHECK-NEXT: |-FieldDecl {{.*}} p3 'Empty3'
+// CHECK-NEXT: `-FieldDecl {{.*}} flag 'unsigned long'
+// CHECK-NEXT:   `-IntegerLiteral {{.*}} 'int' 1
+
+struct ConsecutiveOverlap {
+  char data[5];
+  [[no_unique_address]] Empty p1;
+  [[no_unique_address]] Empty2 p2;
+  [[no_unique_address]] Empty3 p3;
+  unsigned long flag : 1;
+};
+
+ConsecutiveOverlap global2;
+
+// FIXME: we fail to deduce the unnamed bitfields here.
+//
+// CHECK:      CXXRecordDecl {{.*}} struct MultipleAtOffsetZero definition
+// CHECK:      |-FieldDecl {{.*}} data 'char[5]'
+// CHECK-NEXT: |-FieldDecl {{.*}} p1 'Empty'
+// CHECK-NEXT: |-FieldDecl {{.*}} f1 'unsigned long'
+// CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 1
+// CHECK-NEXT: |-FieldDecl {{.*}} p2 'Empty2'
+// CHECK-NEXT: `-FieldDecl {{.*}} f2 'unsigned long'
+// CHECK-NEXT:   `-IntegerLiteral {{.*}} 'int' 1
+
+struct MultipleAtOffsetZero {
+  char data[5];
+  [[no_unique_address]] Empty p1;
+  int : 4;
+  unsigned long f1 : 1;
+  [[no_unique_address]] Empty2 p2;
+  int : 4;
+  unsigned long f2 : 1;
+};
+
+MultipleAtOffsetZero global3;
+
+// FIXME: we fail to deduce the unnamed bitfields here.
+//
+// CHECK:      CXXRecordDecl {{.*}} struct MultipleEmpty definition
+// CHECK:      |-FieldDecl {{.*}} data 'char[5]'
+// CHECK-NEXT: |-FieldDecl {{.*}} p1 'Empty'
+// CHECK-NEXT: |-FieldDecl {{.*}} f1 'unsigned long'
+// CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 1
+// CHECK-NEXT: |-FieldDecl {{.*}} p2 'Empty'
+// CHECK-NEXT: `-FieldDecl {{.*}} f2 'unsigned long'
+// CHECK-NEXT:   `-IntegerLiteral {{.*}} 'int' 1
+
+struct MultipleEmpty {
+  char data[5];
+  [[no_unique_address]] Empty p1;
+  int : 4;
+  unsigned long f1 : 1;
+  [[no_unique_address]] Empty p2;
+  int : 4;
+  unsigned long f2 : 1;
+};
+
+MultipleEmpty global4;
+
+// CHECK:      CXXRecordDecl {{.*}} struct FieldBitfieldOverlap definition
+// CHECK:      |-FieldDecl {{.*}} a 'int'
+// CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 3
+// CHECK-NEXT: |-FieldDecl {{.*}} p1 'Empty'
+// CHECK-NEXT: |-FieldDecl {{.*}} b 'int'
+// CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 6
+// CHECK-NEXT: `-FieldDecl {{.*}} c 'int'
+// CHECK-NEXT:   `-IntegerLiteral {{.*}} 'int' 1
+
+struct FieldBitfieldOverlap {
+  int a : 3;
+  [[no_unique_address]] Empty p1;
+  int b : 6;
+  int c : 1;
+};
+
+FieldBitfieldOverlap global5;