This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by zequanwu on Sep 26 2022, 10:21 AM.

Details

Summary

Missing it causes lldb to crash or give incorrect result.

Diff Detail

Event Timeline

zequanwu created this revision.Sep 26 2022, 10:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2022, 10:21 AM
zequanwu requested review of this revision.Sep 26 2022, 10:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2022, 10:21 AM
DavidSpickett added inline comments.
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 accepted this revision.Sep 27 2022, 7:47 AM

LGTM modulo formatting.

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

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.

This revision is now accepted and ready to land.Sep 27 2022, 7:47 AM

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 updated this revision to Diff 463290.Sep 27 2022, 11:23 AM
zequanwu marked 2 inline comments as done.

Address comments.

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
24

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

This revision was landed with ongoing or failed builds.Sep 27 2022, 11:42 AM
This revision was automatically updated to reflect the committed changes.

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.

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 added a comment.Oct 3 2022, 7:46 AM

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.