Hi!
This patch provides support for Microsoft ABI in CGRecordLayoutBuilder.
Also it includes fix for RecordLayoutBuilder.
- Dmitry.
ms-codegen.patch (9.54 KB)
Given that you've added an assert that the vfptr is at the beginning
of a class, I'm wondering we're doing primary base computations
correctly; please add a testcase for something like following:
struct A { int a; };
struct B { int b; };
struct C : virtual public A { int c; };
struct D : public B, public C { virtual void f(); };
D* x;
General comment for the CGRecordLayoutBuilder changes: please make
sure code is indented correctly.
I'm not following why all the changes to the way we generate padding
are necessary; they don't seem to have any useful effect. (LLVM
structs are not packed by default.)
-Eli
Given that you've added an assert that the vfptr is at the beginning
of a class, I'm wondering we're doing primary base computations
correctly; please add a testcase for something like following:struct A { int a; };
struct B { int b; };
struct C : virtual public A { int c; };
struct D : public B, public C { virtual void f(); };
D* x;
Good test, thnx. I found bug and fix it in patch.
I'm not following why all the changes to the way we generate padding
are necessary; they don't seem to have any useful effect. (LLVM
structs are not packed by default.)
I think this info is helpful at this stage of MS ABI implementing.
In future, when we will have MS ABI support(may be not full, but that works), this code can be easily removed.
But if you say that this is a waste code I'll delete it.
ms_codegen.patch (12 KB)
ping...
What about my patch?
- Dmitry.
+ if (HasNewVirtualFunction(RD)) {
+ CharUnits PtrWidth =
+ Context.toCharUnitsFromBits(Context.getTargetInfo().getPointerWidth(0));
+ } else {
+ if (Layout.getVFPtrOffset() != CharUnits::fromQuantity(-1)) {
+ llvm::Type *FunctionType =
+ llvm::FunctionType::get(llvm::Type::getInt32Ty(Types.getLLVMContext()),
+ /*isVarArg=*/true);
+ llvm::Type *VTableTy = FunctionType->getPointerTo();
+ if (Types.getContext().getTargetInfo().getCXXABI() == CXXABI_Microsoft) {
+ // MSVC can add padding before virtual bases.
+ // If we don`t have vbases then it will be tail padding.
+ CharUnits NumPadBytes = Layout.getNonVirtualSize() - NextFieldOffset;
+ AppendBytes(NumPadBytes);
+ }
I don't see why this needs to happen right here. The Itanium ABI
can also add padding before virtual bases, but that just naturally
happens as part of laying them out; and the tail padding case
should just work correctly if you weren't hacking
ComputeNonVirtualBaseType.
+ if (Types.getContext().getTargetInfo().getCXXABI() != CXXABI_Microsoft) {
+ // Append tail padding if necessary.
+ // In MS ABI it not necessary, because
+ // if we don`t have vbases then we have already add padding.
+ // And MSVC doesn`t add padding after vbases.
+ AppendTailPadding(Layout.getSize());
+ }
Not adding padding is not good enough, because LLVM's struct
layout for non-packed struct types uses the C layout algorithm, which
means that structs get automatically tail-padded to make their size
a multiple of their (IR) alignment. If we're laying out a class where the
required size is not a multiple of the IR alignment (as determined by
getAlignmentAsLLVMStruct), we need to implicitly go back and build
a packed struct layout, which you can do here just by returning false.
+ } else if (
+ Types.getContext().getTargetInfo().getCXXABI() == CXXABI_Microsoft) {
+ // In MS mode this code helps insert padding bytes before fields,
+ // like MSVC does.
+ CharUnits padding = fieldOffset - NextFieldOffset;
Okay, I've added comments.
+ if (Types.getContext().getTargetInfo().getCXXABI() == CXXABI_Microsoft) {
+ // MSVC can add padding before virtual bases.
+ // If we don`t have vbases then it will be tail padding.
+ CharUnits NumPadBytes = Layout.getNonVirtualSize() - NextFieldOffset;
+ AppendBytes(NumPadBytes);
+ }
Ok, without that we just won't have padding bytes at the end of struct/classes.
I mean that we simply won't see they in layout info.
I don't see why this needs to happen right here. The Itanium ABI
can also add padding before virtual bases, but that just naturally
happens as part of laying them out; and the tail padding case
should just work correctly if you weren't hacking
ComputeNonVirtualBaseType.+ if (Types.getContext().getTargetInfo().getCXXABI() != CXXABI_Microsoft) {
+ // Append tail padding if necessary.
+ // In MS ABI it not necessary, because
+ // if we don`t have vbases then we have already add padding.
+ // And MSVC doesn`t add padding after vbases.
+ AppendTailPadding(Layout.getSize());
+ }
I do some tests and you are right. I removed previous code and this "if".
All tests completed well.
Not adding padding is not good enough, because LLVM's struct
layout for non-packed struct types uses the C layout algorithm, which
means that structs get automatically tail-padded to make their size
a multiple of their (IR) alignment. If we're laying out a class where the
required size is not a multiple of the IR alignment (as determined by
getAlignmentAsLLVMStruct), we need to implicitly go back and build
a packed struct layout, which you can do here just by returning false.+ } else if (
+ Types.getContext().getTargetInfo().getCXXABI() == CXXABI_Microsoft) {
+ // In MS mode this code helps insert padding bytes before fields,
+ // like MSVC does.
+ CharUnits padding = fieldOffset - NextFieldOffset;
+
+ AppendBytes(padding);This still shouldn't be necessary. This routine just adds padding bytes
to get us up to fieldOffset, which is determined by the ASTRecordLayout.
If the alignment of the next IR type is high enough, we don't need explicit
padding bytes, which is what the 'if' condition above this is testing. This
isn't any different in the MS ABI.That said, it does look like there's a bug here, because we *always* need
to add padding bytes if we're building a packed layout, and that can
happen in MSVC; see above. So I think the condition should be:if (alignedNextFieldOffset< fieldOffset ||
(Packed&& fieldOffset != NextFieldOffset)) {
This condition doesn't work for class I from test.
We have such layout
struct I
0 | (I vftable pointer)
8 | (I vbtable pointer)
16 | double q
24 | class D (virtual base)
24 | (D vftable pointer)
32 | double a
and layout must be %struct.I = type { i32 (...)**, i32*, [4 x i8], double, %class.D }
But we will have %struct.I = type { i32 (...)**, i32*, double, %class.D } and Packed = false.
May be you mean (!Packed && (fieldOffset != NextFieldOffset))) ?
With this condition all works fine. Also, I compiled with Itanium ABI and everything seems ok.
- Dmitry.
ms-codegen.patch (8.42 KB)
Neither of these types is correct; both incorrectly place the
vbtbl at offset 4. It's just that the first type happens to create
some extra padding in the wrong place, and that makes the
total struct size right.
The type should be this:
type { i32 (...)**, [4 x i8], i32*, double, %class.D }
If you place the vbtbl at the right offset, you should find that
everything else falls out correctly.
John.
2011/10/28 John McCall <rjmccall@apple.com>
- } else if (
- Types.getContext().getTargetInfo().getCXXABI() == CXXABI_Microsoft) {
- // In MS mode this code helps insert padding bytes before fields,
- // like MSVC does.
- CharUnits padding = fieldOffset - NextFieldOffset;
- AppendBytes(padding);
This still shouldn’t be necessary. This routine just adds padding bytes
to get us up to fieldOffset, which is determined by the ASTRecordLayout.
If the alignment of the next IR type is high enough, we don’t need explicit
padding bytes, which is what the ‘if’ condition above this is testing. This
isn’t any different in the MS ABI.That said, it does look like there’s a bug here, because we always need
to add padding bytes if we’re building a packed layout, and that can
happen in MSVC; see above. So I think the condition should be:if (alignedNextFieldOffset< fieldOffset ||
(Packed&& fieldOffset != NextFieldOffset)) {
This condition doesn’t work for class I from test.
We have such layout
struct I
0 | (I vftable pointer)
8 | (I vbtable pointer)
16 | double q
24 | class D (virtual base)
24 | (D vftable pointer)
32 | double a
and layout must be %struct.I = type { i32 (…), i32*, [4 x i8], double, %class.D }
But we will have %struct.I = type { i32 (…), i32*, double, %class.D } and Packed = false.Neither of these types is correct; both incorrectly place the
vbtbl at offset 4. It’s just that the first type happens to create
some extra padding in the wrong place, and that makes the
total struct size right.The type should be this:
type { i32 (…)**, [4 x i8], i32*, double, %class.D }If you place the vbtbl at the right offset, you should find that
everything else falls out correctly.As you can see in my patch I place vbptr on offset from RecordLayout.
So I expect that LLVM add padding between them.
Well, somehow your patch gives us the wrong type, so I imagine that
some invariant is being broken. LLVM definitely will not add padding
between the first two elements in that type.
If we want show layout like MSVC then type must be
type { i32 (…)**, [4 x i8], i32*, [4 x i8], double, %class.D } .
But MSVC never shows padding between vfptr and vbptr.
I don’t really care about how we print out layouts for debugging; I just
want all the data to have the right offset in the IR struct type we make.
John.
In test we have class A
0 | class A
0 | class B (primary base)
0 | (B vftable pointer)
4 | int b_field
8 | int a_field
12 | char one
sizeof=16, dsize=16, align=4
nvsize=16, nvalign=4
%class.A = type { %class.B, i32, i8 }
Can I expect that LLVM add 3 bytes padding at the end?
Yes. The LLVM layout algorithm for non-packed structs is exactly the C algorithm, minus any support for bit-fields, flexible arrays members, or any of a thousand other non-essential extensions:
(1) The ABI size and alignment of a non-struct, non-array type is determined by the target layout string. Arrays have the alignment of their element and the size of N elements. The size and alignment of other structs are determined recursively by this algorithm.
(2) A field occupies a number of consecutive bytes equal to the ABI size of its type.
(3) The first field begins at offset 0.
(4) Every other field begins at the smallest offset that is both (a) a multiple of the ABI alignment of the field's type and (b) greater than the offset of the last byte in the previous field.
(5) The ABI alignment of the struct is the maximum of the ABI alignments of the field.
(6) The ABI size of the struct is equal to smallest offset that is both (a) a multiple of the ABI alignment of the struct and (b) greater than the offset of the last byte in the last field of the struct.
The layout algorithm for packed structs is the same, except that all alignments are considered to be 1.
John.
Please review new patch.
I added the generation of padding bytes between vfptr & vbptr.
- Dmitry.
ms-codegen.patch (8.31 KB)
Hi!
ping. Please review my patch as soon as possible.
Thanks in advance.
- Dmitry.
+ } else if (Context.getTargetInfo().getCXXABI() == CXXABI_Microsoft &&
+ !PrimaryBase && RD->isPolymorphic() &&
+ (RD->getNumVBases() == 0 || HasNewVirtualFunction(RD))) {
Please review new patch.
I added the generation of padding bytes between vfptr& vbptr.+ } else if (Context.getTargetInfo().getCXXABI() == CXXABI_Microsoft&&
+ !PrimaryBase&& RD->isPolymorphic()&&
+ (RD->getNumVBases() == 0 || HasNewVirtualFunction(RD))) {
+Please add the following comment:
// In the MS ABI, we need a vfptr if this class defines new virtual
// functions but has no primary base. The AST doesn't tell us
// directly that a class has new virtual functions, and checking for it
// is a bit expensive, but we can avoid the full check in some important
// cases: non-polymorphic classes have no virtual functions by
// definition, whereas polymorphic classes must either declare new
// virtual functions or inherit them from some base, and if that base
// were non-virtual then there would be a primary base.- if (HasNewVirtualFunction(RD)&&
- (!PrimaryBase || !BaseHasVFPtr(PrimaryBase))) {
- EnsureVTablePointerAlignment(PtrAlign);
- VFPtrOffset = getSize();
- setSize(getSize() + PtrWidth);
- setDataSize(getSize());
- }
- if (RD->getNumVBases()&&
+ if (HasDirectVirtualBases&&
(!PrimaryBase || !PrimaryBase->getNumVBases())) {I'm sorry if I've asked this before, but have you verified that this is the
correct condition? Specifically, does the ABI not re-use vbptrs from
non-primary non-virtual bases?For example, consider this hierarchy:
class A { int a; };
class B { int b; virtual void foo(); };
class C : public virtual A { int c; };
class D { int d; };
class E : public B, public virtual D { int e; };Does class E ultimately have two vbptrs or one?
In this test E will be have one vbptr. But if class E was declared like this "class E : public C, public virtual D" my code will generate 2 vbptr.
I fix it.
+ } else if (Layout.getVFPtrOffset() != CharUnits::fromQuantity(-1)) {
+ llvm::Type *FunctionType =
+ llvm::FunctionType::get(llvm::Type::getInt32Ty(Types.getLLVMContext()),
+ /*isVarArg=*/true);
+ llvm::Type *VTableTy = FunctionType->getPointerTo();
+
+ assert(NextFieldOffset.isZero()&&
+ "VTable pointer must come first!");
+ AppendField(Layout.getVFPtrOffset(), VTableTy->getPointerTo());Indentation.
+
+ if (Layout.getVBPtrOffset() != CharUnits::fromQuantity(-1)) {
+ CharUnits VBPtrOffset = Layout.getVBPtrOffset();
+ if (NextFieldOffset< VBPtrOffset) {
+ AppendBytes(VBPtrOffset - NextFieldOffset);
+ }
+ llvm::Type *Vbptr = llvm::Type::getInt32PtrTy(Types.getLLVMContext());
+ AppendField(VBPtrOffset, Vbptr);
+ }I'm still not sure why this is necessary. AppendField should already
be adding padding to bring NextFieldOffset up to VBPtrOffset. The
only time it doesn't add that padding explicitly is when the ABI
alignment of the field type being added would implicitly introduce it.
As you can see AppendField just does :
1. Save field type in types;
2. Renew NextField offset (NextField = fieldOffset + fieldSize).
+ if (alignedNextFieldOffset< fieldOffset ||
+ (!Packed&& (fieldOffset != NextFieldOffset))) {
class D {
public:
virtual void b(){}
double a;
};
struct I : public virtual D
{
virtual ~I(){}
double q;
};
For this example, we see that fieldOffset = 0x10 for q and alignedNextFieldOffset = 0x10, but NextFieldOffset = 0x0c.
Second condition wrong for MS ABI and useless for other ABI.
I think this condition may look like
if (alignedNextFieldOffset < fieldOffset)
else if ((ABI == MS ABI) && (alignedNextFieldOffset < fieldOffset
fieldOffset != NextFieldOffset))
- Dmitry.