[LLDB][NativePDB] Add class/union layout bit size.

[zequanwu]

Summary

Missing it causes lldb to crash or give incorrect result.

[DavidSpickett]

lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp

53

Drive by comment: you have an extra space at the start of the added lines. (unless phabricator is just indenting it for effect)

[labath]

LGTM modulo formatting.

lldb/test/Shell/SymbolFile/NativePDB/packed_class_layout.cpp

25

I suspect these attributes do not materially change the layout of the union when it stands on its own. It might be more interesting to take this under-aligned union, place it into a (regular) struct, and then check whether we can print it correctly.

[labath]

That said, I am kinda surprised that this is the whole thing. I would have expected to see more here. In dwarf we specify the offsets of individual class members. Does PDB encode that information? If not, how does it handle the case when the definition of some class is missing? If that class is a member of some other class, the offsets of all subsequent members in the bigger class will be wrong? That will probably be true even if we are always able to obtain the size of the smaller class, because of things like vtable pointers.

[zequanwu]

Address comments.

[zequanwu]

In D134656#3818234, @labath wrote:

That said, I am kinda surprised that this is the whole thing. I would have expected to see more here. In dwarf we specify the offsets of individual class members. Does PDB encode that information?

Yes. It has offsets to non-inherited individual class members. For members from parent classes, pdb only has information saying that its direct parents class are at some offsets for this class. For class without vtable, it's easy to calculate inherited member offsets by adding parent class offsets with their member offsets. For class with vtable, it's more complicated to calculate the offsets.

If not, how does it handle the case when the definition of some class is missing? If that class is a member of some other class, the offsets of all subsequent members in the bigger class will be wrong? That will probably be true even if we are always able to obtain the size of the smaller class, because of things like vtable pointers.

I don't quite understand this. vtable pointers are ignored in this visitor https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp#L136-L139.

lldb/test/Shell/SymbolFile/NativePDB/packed_class_layout.cpp

25

Yeah, it doesn't change layout of the union. Moved to a struct.

[labath]

In D134656#3818759, @zequanwu wrote:

In D134656#3818234, @labath wrote:

That said, I am kinda surprised that this is the whole thing. I would have expected to see more here. In dwarf we specify the offsets of individual class members. Does PDB encode that information?

Yes. It has offsets to non-inherited individual class members.

Yes, I only meant direct members. And when I speak of members here, I am also thinking about base classes as they behave very similarly here.

For members from parent classes, pdb only has information saying that its direct parents class are at some offsets for this class. For class without vtable, it's easy to calculate inherited member offsets by adding parent class offsets with their member offsets. For class with vtable, it's more complicated to calculate the offsets.

Yes, so should we take the offsets from the debug info and provide them to clang (so that it does not have to recompute the offsets) ?

If not, how does it handle the case when the definition of some class is missing? If that class is a member of some other class, the offsets of all subsequent members in the bigger class will be wrong? That will probably be true even if we are always able to obtain the size of the smaller class, because of things like vtable pointers.

I don't quite understand this. vtable pointers are ignored in this visitor https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp#L136-L139.

Imagine a class like struct B : A { virtual void foo(); int b; }; and two classes like struct A1 { virtual void bar(); }; struct A2 { void *a; };. Both A1 and A2 have the same size (sizeof(void *)), but that doesn't mean the layout of B will be the same if we substitute it for the base. If A = A1, then B will reuse the vtable pointer of the base, and offsetof(B, b) will be 8. That can't happen with A2, so the compiler will create a new vtable pointer and b offset will be 16 (2*sizeof(void)) (at least that's how it works in the itanium ABI, but I'm sure you could create something similar with the MSVC ABI as well). If you don't have the definition of A, then you just can't know which of these two cases you are in. That's why I think that getting the member locations from the debug info is important.

[zequanwu]

For members from parent classes, pdb only has information saying that its direct parents class are at some offsets for this class. For class without vtable, it's easy to calculate inherited member offsets by adding parent class offsets with their member offsets. For class with vtable, it's more complicated to calculate the offsets.

Yes, so should we take the offsets from the debug info and provide them to clang (so that it does not have to recompute the offsets) ?

Oh, it's already there. This patch just add the missing bit_size.

If not, how does it handle the case when the definition of some class is missing? If that class is a member of some other class, the offsets of all subsequent members in the bigger class will be wrong? That will probably be true even if we are always able to obtain the size of the smaller class, because of things like vtable pointers.

I don't quite understand this. vtable pointers are ignored in this visitor https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp#L136-L139.

Imagine a class like struct B : A { virtual void foo(); int b; }; and two classes like struct A1 { virtual void bar(); }; struct A2 { void *a; };. Both A1 and A2 have the same size (sizeof(void *)), but that doesn't mean the layout of B will be the same if we substitute it for the base. If A = A1, then B will reuse the vtable pointer of the base, and offsetof(B, b) will be 8. That can't happen with A2, so the compiler will create a new vtable pointer and b offset will be 16 (2*sizeof(void)) (at least that's how it works in the itanium ABI, but I'm sure you could create something similar with the MSVC ABI as well). If you don't have the definition of A, then you just can't know which of these two cases you are in. That's why I think that getting the member locations from the debug info is important.

Yeah, in this case, pdb has the info: offsetof(B<A1>, b) == 8 and offsetof(B<A2>, b) == 16. And they are added into LayoutInfo::field_offsets.

[labath]

In D134656#3819025, @zequanwu wrote:

For members from parent classes, pdb only has information saying that its direct parents class are at some offsets for this class. For class without vtable, it's easy to calculate inherited member offsets by adding parent class offsets with their member offsets. For class with vtable, it's more complicated to calculate the offsets.

Yes, so should we take the offsets from the debug info and provide them to clang (so that it does not have to recompute the offsets) ?

Oh, it's already there. This patch just add the missing bit_size.

Ah, cool. Ok then.

lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp

: m_id(id), m_derived_ct(derived_ct), m_tag_decl(tag_decl),
       m_cxx_record_map(cxx_record_map) {
   CVType cvt = m_index.tpi().getType(m_id.index);
   switch (cvt.kind()) {
   case LF_ENUM:
     llvm::cantFail(TypeDeserializer::deserializeAs<EnumRecord>(cvt, m_cvr.er));
     break;
   case LF_UNION:
     llvm::cantFail(TypeDeserializer::deserializeAs<UnionRecord>(cvt, m_cvr.ur));
+    m_layout.bit_size = m_cvr.ur.getSize() * 8;
     break;
   case LF_CLASS:
   case LF_STRUCTURE:
     llvm::cantFail(TypeDeserializer::deserializeAs<ClassRecord>(cvt, m_cvr.cr));
+    m_layout.bit_size = m_cvr.cr.getSize() * 8;
     break;
   default:
     llvm_unreachable("unreachable!");
   }
 }
 
 clang::QualType UdtRecordCompleter::AddBaseClassForTypeIndex(
     llvm::codeview::TypeIndex ti, llvm::codeview::MemberAccess access,

lldb/test/Shell/SymbolFile/NativePDB/packed_class_layout.cpp

+// clang-format off
+// REQUIRES: lld, x86
+
+// Make sure class layout is correct.
+// RUN: %clang_cl --target=x86_64-windows-msvc -Od -Z7 -c /Fo%t.obj -- %s
+// RUN: lld-link -debug:full -nodefaultlib -entry:main %t.obj -out:%t.exe -pdb:%t.pdb
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe \
+// RUN:   -o "expr a" -o "expr b.c" -o "expr b.u.c" -o "expr b.u.i" -o "exit" | FileCheck %s
+
+// CHECK:      (lldb) expr a
+// CHECK-NEXT: (A) $0 = (d1 = 'a', d2 = 1, d3 = 2, d4 = 'b')
+// CHECK-NEXT: (lldb) expr b.c
+// CHECK-NEXT: (char) $1 = 'a'
+// CHECK-NEXT: (lldb) expr b.u.c
+// CHECK-NEXT: (char[2]) $2 = "b"
+// CHECK-NEXT: (lldb) expr b.u.i
+// CHECK-NEXT: (int) $3 = 98
+
+struct __attribute__((packed, aligned(1))) A {
+  char d1;
+  int d2;
+  int d3;
+  char d4;
+};
+
+struct __attribute__((packed, aligned(1))) B {
+  char c;
+  union {
+    char c[2];
+    int i;
+  } u;
+};
+
+A a = {'a', 1, 2, 'b'};
+B b = {'a', {"b"}};
+
+int main() {
+  return 0;
+}