[lldb][DWARFASTParserClang] Don't create unnamed bitfields to account for vtable pointer

[Michael137]

Summary

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 hit 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

[Michael137]

• Rephrase comment

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

protected:
   // module.
   lldb::ModuleSP GetModuleForType(const DWARFDIE &die);
 
 private:
   struct FieldInfo {
     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.
       return (bit_size + bit_offset) <= next_bit_offset;
     }
   };
 
   /// Returns 'true' if we should create an unnamed bitfield

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

void DWARFASTParserClang::ParseSingleMember(
   // Don't turn artificial members such as vtable pointers into real FieldDecls
   // in our AST. Clang will re-create those articial members and they would
   // otherwise just overlap in the layout with the FieldDecls we add here.
   // This needs to be done after updating FieldInfo which keeps track of where
   // field start/end so we don't later try to fill the the space of this
   // 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();
 
   {
     // 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
bool DWARFASTParserClang::ShouldCreateUnnamedBitfield(
     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.
+  // 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;
 }

lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py

def test_bitfield_behind_vtable_ptr(self):
             ValueCheck(name="", type="int:4", value="0"),
             ValueCheck(name="b", type="unsigned int:4", value="0"),
             ValueCheck(name="c", type="unsigned int:4", value="5")
         ]
         self.expect_expr("with_vtable_and_unnamed",
                          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)

lldb/test/API/lang/cpp/bitfields/main.cpp

 };
 struct HasBaseWithVTable : BaseWithVTable {
   unsigned a : 4;
   unsigned b : 4;
   unsigned c : 4;
 };
 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;
 
   lbb.a = 1;
   lbb.b = 3;
 
   lbc.a = 1;
   lbc.b = 0;
int main(int argc, char const *argv[]) {
 
   with_vtable_and_unnamed.b = 0;
   with_vtable_and_unnamed.c = 5;
 
   base_with_vtable.a = 5;
   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
 }