[lldb] Generalize empty record size computation to avoid giving empty C++ structs a size of 0

[teemperor]

Summary

C doesn't allow empty structs but Clang/GCC support them and give them a size of 0.

LLDB implements this by checking the tag kind and if it's DW_TAG_structure_type then
we give it a size of 0 via an empty external RecordLayout. This is done because our
internal TypeSystem is always in C++ mode (which means we would give them a size
of 1).

The current check for when we have this special case is currently too lax as types with
DW_TAG_structure_type can also occur in C++ with types defined using the struct
keyword. This means that in a C++ program with struct Empty{};, LLDB would return
0 for sizeof(Empty) even though the correct size is 1.

This patch removes this special case and replaces it with a generic approach that just
assigns empty structs the byte_size as specified in DWARF. The GCC/Clang special
case is handles as they both emit an explicit DW_AT_byte_size of 0. And if another
compiler decides to use a different byte size for this case then this should also be
handled by the same code as long as that information is provided via DW_AT_byte_size.

[teemperor]

lldb/test/API/lang/cpp/sizeof/TestCPPSizeof.py

21

I actually don't like these kinds of checks as they don't give very useful errors. I want to add do some kind of assertExprEqual check that would do these kind of checks + nice error handling, but for now I don't think we have a better way to do this.

[shafik]

LGTM

[shafik]

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

3

Just for completeness, we might want to add the scenario where the base class is Empty or EmptyClass and then both should be considered size zero.

[teemperor]

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

3

I think I need an example of what is supposed to be zero in that scenario to understand what you mean.

[stella.stamenova]

I think this change broke the Windows LLDB bot. More specifically the TestStructTypes test:

https://lab.llvm.org/buildbot/#/builders/83/builds/8528

[stella.stamenova]

In D105471#2896533, @stella.stamenova wrote:

I think this change broke the Windows LLDB bot. More specifically the TestStructTypes test:

https://lab.llvm.org/buildbot/#/builders/83/builds/8528

Looks like you fixed it already :)

[teemperor]

In D105471#2896544, @stella.stamenova wrote:

In D105471#2896533, @stella.stamenova wrote:

I think this change broke the Windows LLDB bot. More specifically the TestStructTypes test:

https://lab.llvm.org/buildbot/#/builders/83/builds/8528

Looks like you fixed it already :)

Lots of practice from breaking that bot far too often in the past :)

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

if (!die.HasChildren()) {
       } else {
         dwarf->GetObjectFile()->GetModule()->ReportError(
             "DWARF DIE at 0x%8.8x named \"%s\" was not able to start its "
             "definition.\nPlease file a bug and attach the file at the "
             "start of this error message",
             die.GetOffset(), attrs.name.GetCString());
       }
 
-      if (tag == DW_TAG_structure_type) // this only applies in C
-      {
+      // If the byte size of the record is specified then overwrite the size
+      // that would be computed by Clang. This is only needed as LLDB's
+      // TypeSystemClang is always in C++ mode, but some compilers such as
+      // GCC and Clang give empty structs a size of 0 in C mode (in contrast to
+      // the size of 1 for empty structs that would be computed in C++ mode).
+      if (attrs.byte_size) {
         clang::RecordDecl *record_decl =
             TypeSystemClang::GetAsRecordDecl(clang_type);
-
         if (record_decl) {
-          GetClangASTImporter().SetRecordLayout(
-              record_decl, ClangASTImporter::LayoutInfo());
+          ClangASTImporter::LayoutInfo layout;
+          layout.bit_size = *attrs.byte_size * 8;
+          GetClangASTImporter().SetRecordLayout(record_decl, layout);
         }
       }
     } else if (clang_type_was_created) {
       // Start the definition if the class is not objective C since the
       // underlying decls respond to isCompleteDefinition(). Objective
       // C decls don't respond to isCompleteDefinition() so we can't
       // start the declaration definition right away. For C++
       // class/union/structs we want to start the definition in case the

lldb/test/API/lang/c/sizeof/Makefile

+C_SOURCES := main.c
+
+include Makefile.rules

lldb/test/API/lang/c/sizeof/TestCSizeof.py

+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def test(self):
+        self.build()
+        self.createTestTarget()
+
+        # Empty structs are not allowed in C, but Clang/GCC allow them and
+        # give them a size of 0.
+        self.expect_expr("sizeof(Empty) == sizeof_empty", result_value="true")
+        self.expect_expr("sizeof(SingleMember) == sizeof_single", result_value="true")
+        self.expect_expr("sizeof(PaddingMember) == sizeof_padding", result_value="true")

lldb/test/API/lang/c/sizeof/main.c

+struct Empty {};
+struct SingleMember {
+  int i;
+};
+
+struct PaddingMember {
+  int i;
+  char c;
+};
+
+const unsigned sizeof_empty = sizeof(struct Empty);
+const unsigned sizeof_single = sizeof(struct SingleMember);
+const unsigned sizeof_padding = sizeof(struct PaddingMember);
+
+int main() {
+  struct Empty empty;
+  struct SingleMember single;
+  struct PaddingMember padding;
+  // Make sure globals are used.
+  return sizeof_empty + sizeof_single + sizeof_padding;
+}

lldb/test/API/lang/cpp/sizeof/Makefile

+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/test/API/lang/cpp/sizeof/TestCPPSizeof.py

+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def test(self):
+        self.build()
+        self.createTestTarget()
+
+        # Empty structs/classes have size 1 in C++.
+        self.expect_expr("sizeof(Empty) == sizeof_empty", result_value="true")
+        self.expect_expr("sizeof(EmptyClass) == sizeof_empty_class", result_value="true")
+        self.expect_expr("sizeof(SingleMember) == sizeof_single", result_value="true")
+        self.expect_expr("sizeof(SingleMemberClass) == sizeof_single_class", result_value="true")
+        self.expect_expr("sizeof(PaddingMember) == sizeof_padding", result_value="true")
+        self.expect_expr("sizeof(PaddingMemberClass) == sizeof_padding_class", result_value="true")

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

+struct Empty {};
+class EmptyClass {};
+
+struct SingleMember {
+  int i;
+};
+class SingleMemberClass {
+  int i;
+};
+
+struct PaddingMember {
+  int i;
+  char c;
+};
+class PaddingMemberClass {
+  int i;
+  char c;
+};
+
+const unsigned sizeof_empty = sizeof(Empty);
+const unsigned sizeof_empty_class = sizeof(EmptyClass);
+const unsigned sizeof_single = sizeof(SingleMember);
+const unsigned sizeof_single_class = sizeof(SingleMemberClass);
+const unsigned sizeof_padding = sizeof(PaddingMember);
+const unsigned sizeof_padding_class = sizeof(PaddingMemberClass);
+
+int main() {
+  Empty empty;
+  EmptyClass empty_class;
+  SingleMember single;
+  SingleMemberClass single_class;
+  PaddingMember padding;
+  PaddingMemberClass padding_class;
+  // Make sure globals are used.
+  return sizeof_empty + sizeof_empty_class + sizeof_single +
+    sizeof_single_class + sizeof_padding + sizeof_padding_class;
+}