[cfe-dev] CGRecordLayoutBuilder for MS ABI

[r4start]

On 02/11/2011 10:12, John McCall wrote:
> On Oct 30, 2011, at 11:56 PM, r4start wrote:
>> 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.