From eb9d81dff99d4996f41c93ce71b08aaf753cbae8 Mon Sep 17 00:00:00 2001 From: Anders Carlsson Date: Sun, 17 Apr 2011 21:56:13 +0000 Subject: [PATCH] When laying out bases in, always try the "base subobject" LLVM type. If it turns out that a field or base needs to be laid out in the tail padding of the base, CGRecordLayoutBuilder::ResizeLastBaseFieldIfNecessary will convert it to an array of i8. I've audited the new test results to make sure that they are still valid. I've also verified that we pass a self-host with this change. This (finally) fixes PR5589! git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@129673 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/CodeGen/CGRecordLayoutBuilder.cpp | 80 ++++++++++++++++---- test/CodeGenCXX/class-layout.cpp | 28 +++++++ test/CodeGenCXX/global-init.cpp | 4 +- test/CodeGenCXX/pointers-to-data-members.cpp | 10 +-- test/CodeGenCXX/pragma-pack.cpp | 4 +- test/CodeGenCXX/x86_64-arguments.cpp | 6 +- 6 files changed, 107 insertions(+), 25 deletions(-) diff --git a/lib/CodeGen/CGRecordLayoutBuilder.cpp b/lib/CodeGen/CGRecordLayoutBuilder.cpp index 8f070f3e1c..5a8115639d 100644 --- a/lib/CodeGen/CGRecordLayoutBuilder.cpp +++ b/lib/CodeGen/CGRecordLayoutBuilder.cpp @@ -81,9 +81,19 @@ public: private: CodeGenTypes &Types; + /// LastLaidOutBaseInfo - Contains the offset and non-virtual size of the + /// last base laid out. Used so that we can replace the last laid out base + /// type with an i8 array if needed. + struct LastLaidOutBaseInfo { + CharUnits Offset; + CharUnits NonVirtualSize; + + bool isValid() const { return !NonVirtualSize.isZero(); } + void invalidate() { NonVirtualSize = CharUnits::Zero(); } + + } LastLaidOutBase; + /// Alignment - Contains the alignment of the RecordDecl. - // - // FIXME: This is not needed and should be removed. CharUnits Alignment; /// BitsAvailableInLastField - If a bit field spans only part of a LLVM field, @@ -143,6 +153,12 @@ private: /// struct size is a multiple of the field alignment. void AppendPadding(CharUnits fieldOffset, CharUnits fieldAlignment); + /// ResizeLastBaseFieldIfNecessary - Fields and bases can be laid out in the + /// tail padding of a previous base. If this happens, the type of the previous + /// base needs to be changed to an array of i8. Returns true if the last + /// laid out base was resized. + bool ResizeLastBaseFieldIfNecessary(CharUnits offset); + /// getByteArrayType - Returns a byte array type with the given number of /// elements. const llvm::Type *getByteArrayType(CharUnits NumBytes); @@ -190,6 +206,7 @@ void CGRecordLayoutBuilder::Layout(const RecordDecl *D) { // We weren't able to layout the struct. Try again with a packed struct Packed = true; + LastLaidOutBase.invalidate(); NextFieldOffset = CharUnits::Zero(); FieldTypes.clear(); Fields.clear(); @@ -334,6 +351,17 @@ void CGRecordLayoutBuilder::LayoutBitField(const FieldDecl *D, uint64_t nextFieldOffsetInBits = Types.getContext().toBits(NextFieldOffset); unsigned numBytesToAppend; + if (fieldOffset < nextFieldOffsetInBits && !BitsAvailableInLastField) { + assert(fieldOffset % 8 == 0 && "Field offset not aligned correctly"); + + CharUnits fieldOffsetInCharUnits = + Types.getContext().toCharUnitsFromBits(fieldOffset); + + // Try to resize the last base field. + if (ResizeLastBaseFieldIfNecessary(fieldOffsetInCharUnits)) + nextFieldOffsetInBits = Types.getContext().toBits(NextFieldOffset); + } + if (fieldOffset < nextFieldOffsetInBits) { assert(BitsAvailableInLastField && "Bitfield size mismatch!"); assert(!NextFieldOffset.isZero() && "Must have laid out at least one byte"); @@ -410,6 +438,14 @@ bool CGRecordLayoutBuilder::LayoutField(const FieldDecl *D, CharUnits alignedNextFieldOffsetInBytes = NextFieldOffset.RoundUpToAlignment(typeAlignment); + if (fieldOffsetInBytes < alignedNextFieldOffsetInBytes) { + // Try to resize the last base field. + if (ResizeLastBaseFieldIfNecessary(fieldOffsetInBytes)) { + alignedNextFieldOffsetInBytes = + NextFieldOffset.RoundUpToAlignment(typeAlignment); + } + } + if (fieldOffsetInBytes < alignedNextFieldOffsetInBytes) { assert(!Packed && "Could not place field even with packed struct!"); return false; @@ -421,6 +457,7 @@ bool CGRecordLayoutBuilder::LayoutField(const FieldDecl *D, Fields[D] = FieldTypes.size(); AppendField(fieldOffsetInBytes, Ty); + LastLaidOutBase.invalidate(); return true; } @@ -516,11 +553,16 @@ void CGRecordLayoutBuilder::LayoutUnion(const RecordDecl *D) { void CGRecordLayoutBuilder::LayoutBase(const CXXRecordDecl *base, const CGRecordLayout &baseLayout, CharUnits baseOffset) { + ResizeLastBaseFieldIfNecessary(baseOffset); + AppendPadding(baseOffset, CharUnits::One()); const ASTRecordLayout &baseASTLayout = Types.getContext().getASTRecordLayout(base); + LastLaidOutBase.Offset = NextFieldOffset; + LastLaidOutBase.NonVirtualSize = baseASTLayout.getNonVirtualSize(); + // Fields and bases can be laid out in the tail padding of previous // bases. If this happens, we need to allocate the base as an i8 // array; otherwise, we can use the subobject type. However, @@ -529,20 +571,10 @@ void CGRecordLayoutBuilder::LayoutBase(const CXXRecordDecl *base, // approximation, which is to use the base subobject type if it // has the same LLVM storage size as the nvsize. - // The nvsize, i.e. the unpadded size of the base class. - CharUnits nvsize = baseASTLayout.getNonVirtualSize(); - -#if 0 const llvm::StructType *subobjectType = baseLayout.getBaseSubobjectLLVMType(); - const llvm::StructLayout *baseLLVMLayout = - Types.getTargetData().getStructLayout(subobjectType); - CharUnits stsize = CharUnits::fromQuantity(baseLLVMLayout->getSizeInBytes()); + AppendField(baseOffset, subobjectType); - if (nvsize == stsize) - AppendField(baseOffset, subobjectType); - else -#endif - AppendBytes(nvsize); + Types.addBaseSubobjectTypeName(base, baseLayout); } void CGRecordLayoutBuilder::LayoutNonVirtualBase(const CXXRecordDecl *base, @@ -734,6 +766,8 @@ void CGRecordLayoutBuilder::AppendTailPadding(uint64_t RecordSize) { CharUnits RecordSizeInBytes = Types.getContext().toCharUnitsFromBits(RecordSize); + ResizeLastBaseFieldIfNecessary(RecordSizeInBytes); + assert(NextFieldOffset <= RecordSizeInBytes && "Size mismatch!"); CharUnits AlignedNextFieldOffset = @@ -777,6 +811,24 @@ void CGRecordLayoutBuilder::AppendPadding(CharUnits fieldOffset, } } +bool CGRecordLayoutBuilder::ResizeLastBaseFieldIfNecessary(CharUnits offset) { + // Check if we have a base to resize. + if (!LastLaidOutBase.isValid()) + return false; + + // This offset does not overlap with the tail padding. + if (offset >= NextFieldOffset) + return false; + + // Restore the field offset and append an i8 array instead. + FieldTypes.pop_back(); + NextFieldOffset = LastLaidOutBase.Offset; + AppendBytes(LastLaidOutBase.NonVirtualSize); + LastLaidOutBase.invalidate(); + + return true; +} + const llvm::Type *CGRecordLayoutBuilder::getByteArrayType(CharUnits numBytes) { assert(!numBytes.isZero() && "Empty byte arrays aren't allowed."); diff --git a/test/CodeGenCXX/class-layout.cpp b/test/CodeGenCXX/class-layout.cpp index 6675b4963b..672ce03f30 100644 --- a/test/CodeGenCXX/class-layout.cpp +++ b/test/CodeGenCXX/class-layout.cpp @@ -17,3 +17,31 @@ namespace Test3 { // CHECK: %"struct.Test3::A" = type { i32 (...)**, i32 } struct A { virtual void f(); int a; } *a; } + +namespace Test4 { + // Test from PR5589. + // CHECK: %"struct.Test4::A" = type { i32, i8, float } + // CHECK: %"struct.Test4::B" = type { %"struct.Test4::A", i16, double } + struct A { + int a; + char c; + float b; + }; + struct B : public A { + short d; + double e; + } *b; +} + +namespace Test5 { + struct A { + virtual void f(); + char a; + }; + + // CHECK: %"struct.Test4::B" = type { [9 x i8], i8, i8, [5 x i8] } + struct B : A { + char b : 1; + char c; + } *b; +} diff --git a/test/CodeGenCXX/global-init.cpp b/test/CodeGenCXX/global-init.cpp index f9eeb59145..9bd7390d07 100644 --- a/test/CodeGenCXX/global-init.cpp +++ b/test/CodeGenCXX/global-init.cpp @@ -20,8 +20,8 @@ struct D { ~D(); }; // PR6205: The casts should not require global initializers // CHECK: @_ZN6PR59741cE = external global %"struct.PR5974::C" -// CHECK: @_ZN6PR59741aE = global %"struct.PR5974::A"* bitcast (%"struct.PR5974::C"* @_ZN6PR59741cE to %"struct.PR5974::A"*), align 8 -// CHECK: @_ZN6PR59741bE = global %"struct.PR5974::A"* bitcast (i8* getelementptr (%"struct.PR5974::C"* @_ZN6PR59741cE, i32 0, i32 0, i64 4) to %"struct.PR5974::A"*), align 8 +// CHECK: @_ZN6PR59741aE = global %"struct.PR5974::A"* getelementptr inbounds (%"struct.PR5974::C"* @_ZN6PR59741cE, i32 0, i32 0) +// CHECK: @_ZN6PR59741bE = global %"struct.PR5974::A"* bitcast (i8* getelementptr (i8* bitcast (%"struct.PR5974::C"* @_ZN6PR59741cE to i8*), i64 4) to %"struct.PR5974::A"*), align 8 // CHECK: call void @_ZN1AC1Ev(%struct.A* @a) // CHECK: call i32 @__cxa_atexit(void (i8*)* bitcast (void (%struct.A*)* @_ZN1AD1Ev to void (i8*)*), i8* getelementptr inbounds (%struct.A* @a, i32 0, i32 0), i8* bitcast (i8** @__dso_handle to i8*)) diff --git a/test/CodeGenCXX/pointers-to-data-members.cpp b/test/CodeGenCXX/pointers-to-data-members.cpp index 40723a856c..2a22d23e50 100644 --- a/test/CodeGenCXX/pointers-to-data-members.cpp +++ b/test/CodeGenCXX/pointers-to-data-members.cpp @@ -55,7 +55,7 @@ namespace ZeroInit { }; struct C : A, B { int j; }; - // CHECK-GLOBAL: @_ZN8ZeroInit1cE = global {{%.*}} { [16 x i8] c"\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00", [176 x i8] c"\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF", i32 0, [4 x i8] zeroinitializer } + // CHECK-GLOBAL: @_ZN8ZeroInit1cE = global {{%.*}} { %"struct.PR7139::ptr_to_member_struct" { i64 -1, i32 0 }, %"struct.ZeroInit::B" { [10 x %"struct.PR7139::ptr_to_member_struct"] [%"struct.PR7139::ptr_to_member_struct" { i64 -1, i32 0 }, %"struct.PR7139::ptr_to_member_struct" { i64 -1, i32 0 }, %"struct.PR7139::ptr_to_member_struct" { i64 -1, i32 0 }, %"struct.PR7139::ptr_to_member_struct" { i64 -1, i32 0 }, %"struct.PR7139::ptr_to_member_struct" { i64 -1, i32 0 }, %"struct.PR7139::ptr_to_member_struct" { i64 -1, i32 0 }, %"struct.PR7139::ptr_to_member_struct" { i64 -1, i32 0 }, %"struct.PR7139::ptr_to_member_struct" { i64 -1, i32 0 }, %"struct.PR7139::ptr_to_member_struct" { i64 -1, i32 0 }, %"struct.PR7139::ptr_to_member_struct" { i64 -1, i32 0 }], i8 0, i64 -1 }, i32 0 }, align 8 C c; } @@ -172,15 +172,15 @@ struct A { int A::*i; }; -// CHECK-GLOBAL: @_ZN12VirtualBases1bE = global {{%.*}} { i32 (...)** null, [16 x i8] c"\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF" } +// CHECK-GLOBAL: @_ZN12VirtualBases1bE = global %"struct.VirtualBases::B" { i32 (...)** null, %"struct.VirtualBases::A" { i8 0, i64 -1 } }, align 8 struct B : virtual A { }; B b; -// CHECK-GLOBAL: @_ZN12VirtualBases1cE = global {{%.*}} { i32 (...)** null, i64 -1, [16 x i8] c"\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF" } +// CHECK-GLOBAL: @_ZN12VirtualBases1cE = global %"struct.VirtualBases::C" { i32 (...)** null, i64 -1, %"struct.VirtualBases::A" { i8 0, i64 -1 } }, align 8 struct C : virtual A { int A::*i; }; C c; - // CHECK-GLOBAL: @_ZN12VirtualBases1dE = global {{%.*}} { [16 x i8] c"\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF", i64 -1, [16 x i8] c"\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF" } +// CHECK-GLOBAL: @_ZN12VirtualBases1dE = global %"struct.VirtualBases::D" { %"struct.VirtualBases::C.base" { i32 (...)** null, i64 -1 }, i64 -1, %"struct.VirtualBases::A" { i8 0, i64 -1 } }, align 8 struct D : C { int A::*i; }; D d; @@ -227,6 +227,6 @@ namespace test4 { struct C : virtual B { int *C_p; }; struct D : C { int *D_p; }; - // CHECK-GLOBAL: @_ZN5test41dE = global {{%.*}} { [16 x i8] zeroinitializer, i32* null, [16 x i8] c"\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF", [4 x i8] zeroinitializer } + // CHECK-GLOBAL: @_ZN5test41dE = global %"struct.test4::D" { %"struct.test4::C.base" zeroinitializer, i32* null, %"struct.VirtualBases::C.base" { i32 (...)** null, i64 -1 }, %"struct.test4::A" zeroinitializer }, align 8 D d; } diff --git a/test/CodeGenCXX/pragma-pack.cpp b/test/CodeGenCXX/pragma-pack.cpp index c0ddb1d855..c0b0259784 100644 --- a/test/CodeGenCXX/pragma-pack.cpp +++ b/test/CodeGenCXX/pragma-pack.cpp @@ -10,5 +10,7 @@ struct Sub : virtual Base { char c; }; -// CHECK: %struct.Sub = type <{ i32 (...)**, i8, [8 x i8] }> +// CHECK: %struct.Sub = type <{ i32 (...)**, i8, %struct.Base }> void f(Sub*) { } + +static int i[sizeof(Sub) == 13 ? 1 : -1]; diff --git a/test/CodeGenCXX/x86_64-arguments.cpp b/test/CodeGenCXX/x86_64-arguments.cpp index e7316989fe..01f1a445b1 100644 --- a/test/CodeGenCXX/x86_64-arguments.cpp +++ b/test/CodeGenCXX/x86_64-arguments.cpp @@ -3,13 +3,13 @@ // Basic base class test. struct f0_s0 { unsigned a; }; struct f0_s1 : public f0_s0 { void *b; }; -// CHECK: define void @_Z2f05f0_s1(i64 %a0.coerce0, i8* %a0.coerce1) +// CHECK: define void @_Z2f05f0_s1(i32 %a0.coerce0, i8* %a0.coerce1) void f0(f0_s1 a0) { } // Check with two eight-bytes in base class. struct f1_s0 { unsigned a; unsigned b; float c; }; struct f1_s1 : public f1_s0 { float d;}; -// CHECK: define void @_Z2f15f1_s1(i64 %a0.coerce0, double %a0.coerce1) +// CHECK: define void @_Z2f15f1_s1(i64 %a0.coerce0, <2 x float> %a0.coerce1) void f1(f1_s1 a0) { } // Check with two eight-bytes in base class and merge. @@ -54,7 +54,7 @@ namespace PR7742 { // Also rdar://8250764 struct c2 : public s2 {}; - // CHECK: define double @_ZN6PR77423fooEPNS_2c2E(%"struct.PR7742::c2"* %P) + // CHECK: define <2 x float> @_ZN6PR77423fooEPNS_2c2E(%"struct.PR7742::c2"* %P) c2 foo(c2 *P) { } -- 2.40.0