[Lldb-commits] [lldb] e2ede17 - [lldb] Update field offset/sizes when encountering artificial members such as vtable pointers

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2021-10-30T13:22:21+02:00
New Revision: e2ede1715d4141279bb76d8850e0494b85d1eee8

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

LOG: [lldb] Update field offset/sizes when encountering artificial members such as vtable pointers

`DWARFASTParserClang::ParseSingleMember` turns DWARF DIEs that describe
struct/class members into their respective Clang representation (e.g.,
clang::FieldDecl). It also updates a record of where the last field
started/ended so that we can speculatively fill any holes between a field and a
bitfield with unnamed bitfield padding.

Right now we are completely ignoring 'artificial' members when parsing the DWARF
of a struct/class. The only artificial member that seems to be emitted in
practice for C/C++ seems to be the vtable pointer.

By completely skipping both the Clang AST node creation and the updating of the
last-field record, we essentially leave a hole in our layout with the size of
our artificial member. If the next member is a bitfield we then speculatively
fill the hole with an unnamed bitfield. During CodeGen Clang inserts an
artificial vtable pointer into the layout again which now occupies the same
offset as the unnamed bitfield. This later brings down Clang's
`CGRecordLowering::insertPadding` when it checks that none of the fields of the
generated record layout overlap.

Note that this is not a Clang bug. We explicitly set the offset of our fields in
LLDB and overwrite whatever Clang makes up.

Reviewed By: labath

Differential Revision: https://reviews.llvm.org/D112697

Added: 
    

Modified: 
    lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
    lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
    lldb/test/API/lang/cpp/bitfields/main.cpp

Removed: 
    


################################################################################
diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 8affb05d56a26..b6b99d2e9bcf6 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2675,11 +2675,6 @@ void DWARFASTParserClang::ParseSingleMember(
 
   // FIXME: Remove the workarounds below and make this const.
   MemberAttributes attrs(die, parent_die, module_sp);
-  // Skip artificial members such as vtable pointers.
-  // FIXME: This check should verify that this is indeed an artificial member
-  // we are supposed to ignore.
-  if (attrs.is_artificial)
-    return;
 
   const bool class_is_objc_object_or_interface =
       TypeSystemClang::IsObjCObjectOrInterfaceType(class_clang_type);
@@ -2845,6 +2840,17 @@ void DWARFASTParserClang::ParseSingleMember(
     last_field_info.SetIsBitfield(false);
   }
 
+  // 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)
+    return;
+
   if (!member_clang_type.IsCompleteType())
     member_clang_type.GetCompleteType();
 

diff  --git a/lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py b/lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
index 4cdaf32ca0659..956ae275b337e 100644
--- a/lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
+++ b/lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
@@ -145,8 +145,6 @@ def test_bitfields(self):
         self.expect_expr("base_with_vtable", result_children=base_with_vtable_children)
         self.expect_var_path("base_with_vtable", children=base_with_vtable_children)
 
-    # FIXME: These all crash due the vtable ptr.
-    @skipIf
     @no_debug_info_test
     def test_bitfield_behind_vtable_ptr(self):
         self.build()
@@ -164,7 +162,7 @@ def test_bitfield_behind_vtable_ptr(self):
 
         # Test a class with a vtable ptr and unnamed bitfield directly after.
         with_vtable_and_unnamed_children = [
-            ValueCheck(name="", type="unsigned int:4", value="0"),
+            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")
         ]

diff  --git a/lldb/test/API/lang/cpp/bitfields/main.cpp b/lldb/test/API/lang/cpp/bitfields/main.cpp
index e40f1648ac834..eb9db7271aae6 100644
--- a/lldb/test/API/lang/cpp/bitfields/main.cpp
+++ b/lldb/test/API/lang/cpp/bitfields/main.cpp
@@ -97,7 +97,7 @@ WithVTable with_vtable;
 
 struct WithVTableAndUnnamed {
   virtual ~WithVTableAndUnnamed() {}
-  unsigned a : 4;
+  unsigned : 4;
   unsigned b : 4;
   unsigned c : 4;
 };
@@ -146,7 +146,6 @@ int main(int argc, char const *argv[]) {
   with_vtable.b = 0;
   with_vtable.c = 5;
 
-  with_vtable_and_unnamed.a = 5;
   with_vtable_and_unnamed.b = 0;
   with_vtable_and_unnamed.c = 5;