You'll find no disagreement from me, that change is quite a bit more invasive than this one though.
On Mon, Sep 29, 2014 at 9:52 AM, Reid Kleckner <[email protected]> wrote: > Long term, we should solve this a different way: make NVBaseType the > primary way that we describe structs in LLVM IR, instead of trying to use > the complete type or memory type. Both of the complete and memory type can > use the nvbase type as a subobject with extra bits at the end. > > > > On Sat, Sep 27, 2014 at 11:39 PM, David Majnemer <[email protected] > > wrote: > >> Author: majnemer >> Date: Sun Sep 28 01:39:30 2014 >> New Revision: 218577 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=218577&view=rev >> Log: >> CodeGen: Don't crash when initializing pointer-to-member fields in bases >> >> Clang uses two types to talk about a C++ class, the >> NonVirtualBaseLLVMType and the LLVMType. Previously, we would allow one >> of these to be packed and the other not. >> >> This is problematic. If both don't agree on a common subset of fields, >> then routines like getLLVMFieldNo will point to the wrong field. Solve >> this by copying the 'packed'-ness of the complete type to the >> non-virtual subobject. For this to work, we need to take into account >> the non-virtual subobject's size and alignment when we are computing the >> layout of the complete object. >> >> This fixes PR21089. >> >> Modified: >> cfe/trunk/lib/CodeGen/CGRecordLayoutBuilder.cpp >> cfe/trunk/test/CodeGenCXX/class-layout.cpp >> cfe/trunk/test/CodeGenCXX/pointers-to-data-members.cpp >> cfe/trunk/test/CodeGenCXX/pr18962.cpp >> >> Modified: cfe/trunk/lib/CodeGen/CGRecordLayoutBuilder.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGRecordLayoutBuilder.cpp?rev=218577&r1=218576&r2=218577&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/CodeGen/CGRecordLayoutBuilder.cpp (original) >> +++ cfe/trunk/lib/CodeGen/CGRecordLayoutBuilder.cpp Sun Sep 28 01:39:30 >> 2014 >> @@ -93,7 +93,7 @@ struct CGRecordLowering { >> bool operator <(const MemberInfo& a) const { return Offset < >> a.Offset; } >> }; >> // The constructor. >> - CGRecordLowering(CodeGenTypes &Types, const RecordDecl *D); >> + CGRecordLowering(CodeGenTypes &Types, const RecordDecl *D, bool >> Packed); >> // Short helper routines. >> /// \brief Constructs a MemberInfo instance from an offset and >> llvm::Type *. >> MemberInfo StorageInfo(CharUnits Offset, llvm::Type *Data) { >> @@ -174,7 +174,7 @@ struct CGRecordLowering { >> /// padding that is or can potentially be used. >> void clipTailPadding(); >> /// \brief Determines if we need a packed llvm struct. >> - void determinePacked(); >> + void determinePacked(bool NVBaseType); >> /// \brief Inserts padding everwhere it's needed. >> void insertPadding(); >> /// \brief Fills out the structures that are ultimately consumed. >> @@ -203,12 +203,12 @@ private: >> }; >> } // namespace { >> >> -CGRecordLowering::CGRecordLowering(CodeGenTypes &Types, const RecordDecl >> *D) >> +CGRecordLowering::CGRecordLowering(CodeGenTypes &Types, const RecordDecl >> *D, bool Packed) >> : Types(Types), Context(Types.getContext()), D(D), >> RD(dyn_cast<CXXRecordDecl>(D)), >> Layout(Types.getContext().getASTRecordLayout(D)), >> DataLayout(Types.getDataLayout()), IsZeroInitializable(true), >> - IsZeroInitializableAsBase(true), Packed(false) {} >> + IsZeroInitializableAsBase(true), Packed(Packed) {} >> >> void CGRecordLowering::setBitFieldInfo( >> const FieldDecl *FD, CharUnits StartOffset, llvm::Type *StorageType) >> { >> @@ -269,7 +269,7 @@ void CGRecordLowering::lower(bool NVBase >> std::stable_sort(Members.begin(), Members.end()); >> Members.push_back(StorageInfo(Size, getIntNType(8))); >> clipTailPadding(); >> - determinePacked(); >> + determinePacked(NVBaseType); >> insertPadding(); >> Members.pop_back(); >> calculateZeroInit(); >> @@ -533,8 +533,13 @@ void CGRecordLowering::clipTailPadding() >> } >> } >> >> -void CGRecordLowering::determinePacked() { >> +void CGRecordLowering::determinePacked(bool NVBaseType) { >> + if (Packed) >> + return; >> CharUnits Alignment = CharUnits::One(); >> + CharUnits NVAlignment = CharUnits::One(); >> + CharUnits NVSize = >> + !NVBaseType && RD ? Layout.getNonVirtualSize() : CharUnits::Zero(); >> for (std::vector<MemberInfo>::const_iterator Member = Members.begin(), >> MemberEnd = Members.end(); >> Member != MemberEnd; ++Member) { >> @@ -544,12 +549,19 @@ void CGRecordLowering::determinePacked() >> // then the entire record must be packed. >> if (Member->Offset % getAlignment(Member->Data)) >> Packed = true; >> + if (Member->Offset < NVSize) >> + NVAlignment = std::max(NVAlignment, getAlignment(Member->Data)); >> Alignment = std::max(Alignment, getAlignment(Member->Data)); >> } >> // If the size of the record (the capstone's offset) is not a multiple >> of the >> // record's alignment, it must be packed. >> if (Members.back().Offset % Alignment) >> Packed = true; >> + // If the non-virtual sub-object is not a multiple of the non-virtual >> + // sub-object's alignment, it must be packed. We cannot have a packed >> + // non-virtual sub-object and an unpacked complete object or vise >> versa. >> + if (NVSize % NVAlignment) >> + Packed = true; >> // Update the alignment of the sentinal. >> if (!Packed) >> Members.back().Data = getIntNType(Context.toBits(Alignment)); >> @@ -641,20 +653,24 @@ CGBitFieldInfo CGBitFieldInfo::MakeInfo( >> >> CGRecordLayout *CodeGenTypes::ComputeRecordLayout(const RecordDecl *D, >> llvm::StructType *Ty) { >> - CGRecordLowering Builder(*this, D); >> + CGRecordLowering Builder(*this, D, /*Packed=*/false); >> >> - Builder.lower(false); >> + Builder.lower(/*NonVirtualBaseType=*/false); >> >> // If we're in C++, compute the base subobject type. >> llvm::StructType *BaseTy = nullptr; >> if (isa<CXXRecordDecl>(D) && !D->isUnion() && >> !D->hasAttr<FinalAttr>()) { >> BaseTy = Ty; >> if (Builder.Layout.getNonVirtualSize() != Builder.Layout.getSize()) { >> - CGRecordLowering BaseBuilder(*this, D); >> - BaseBuilder.lower(true); >> + CGRecordLowering BaseBuilder(*this, D, /*Packed=*/Builder.Packed); >> + BaseBuilder.lower(/*NonVirtualBaseType=*/true); >> BaseTy = llvm::StructType::create( >> getLLVMContext(), BaseBuilder.FieldTypes, "", >> BaseBuilder.Packed); >> addRecordTypeName(D, BaseTy, ".base"); >> + // BaseTy and Ty must agree on their packedness for getLLVMFieldNo >> to work >> + // on both of them with the same index. >> + assert(Builder.Packed == BaseBuilder.Packed && >> + "Non-virtual and complete types must agree on packedness"); >> } >> } >> >> >> Modified: cfe/trunk/test/CodeGenCXX/class-layout.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/class-layout.cpp?rev=218577&r1=218576&r2=218577&view=diff >> >> ============================================================================== >> --- cfe/trunk/test/CodeGenCXX/class-layout.cpp (original) >> +++ cfe/trunk/test/CodeGenCXX/class-layout.cpp Sun Sep 28 01:39:30 2014 >> @@ -14,7 +14,7 @@ namespace Test2 { >> >> namespace Test3 { >> // C should have a vtable pointer. >> - // CHECK: %"struct.Test3::A" = type { i32 (...)**, i32 } >> + // CHECK: %"struct.Test3::A" = type <{ i32 (...)**, i32, [4 x i8] }> >> struct A { virtual void f(); int a; } *a; >> } >> >> >> Modified: cfe/trunk/test/CodeGenCXX/pointers-to-data-members.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/pointers-to-data-members.cpp?rev=218577&r1=218576&r2=218577&view=diff >> >> ============================================================================== >> --- cfe/trunk/test/CodeGenCXX/pointers-to-data-members.cpp (original) >> +++ cfe/trunk/test/CodeGenCXX/pointers-to-data-members.cpp Sun Sep 28 >> 01:39:30 2014 >> @@ -55,7 +55,7 @@ namespace ZeroInit { >> }; >> >> struct C : A, B { int j; }; >> - // CHECK-GLOBAL: @_ZN8ZeroInit1cE = global {{%.*}} { >> %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::B" { [10 x >> %"struct.ZeroInit::A"] [%"struct.ZeroInit::A" { i64 -1, i32 0 }, >> %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, >> i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { >> i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, >> %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, >> i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { >> i64 -1, i32 0 }], i8 0, i64 -1 }, i32 0 }, align 8 >> + // CHECK-GLOBAL: @_ZN8ZeroInit1cE = global {{%.*}} <{ >> %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::B" { [10 x >> %"struct.ZeroInit::A"] [%"struct.ZeroInit::A" { i64 -1, i32 0 }, >> %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, >> i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { >> i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, >> %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, >> i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { >> i64 -1, i32 0 }], i8 0, i64 -1 }, i32 0, [4 x i8] zeroinitializer }>, align >> 8 >> C c; >> } >> >> @@ -255,4 +255,17 @@ namespace PR13097 { >> // CHECK: call void @_ZN7PR130971XC1ERKS0_ >> } >> >> +namespace PR21089 { >> +struct A { >> + bool : 1; >> + int A::*x; >> + bool y; >> + A(); >> +}; >> +struct B : A { >> +}; >> +B b; >> +// CHECK-GLOBAL: @_ZN7PR210891bE = global %"struct.PR21089::B" { >> %"struct.PR21089::A.base" <{ i8 0, [7 x i8] zeroinitializer, i64 -1, i8 0 >> }>, [7 x i8] zeroinitializer }, align 8 >> +} >> + >> // CHECK-O3: attributes [[NUW]] = { nounwind readnone{{.*}} } >> >> Modified: cfe/trunk/test/CodeGenCXX/pr18962.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/pr18962.cpp?rev=218577&r1=218576&r2=218577&view=diff >> >> ============================================================================== >> --- cfe/trunk/test/CodeGenCXX/pr18962.cpp (original) >> +++ cfe/trunk/test/CodeGenCXX/pr18962.cpp Sun Sep 28 01:39:30 2014 >> @@ -26,7 +26,7 @@ fn2(C *) { >> >> // We end up using an opaque type for 'append' to avoid circular >> references. >> // CHECK: %class.A = type { {}* } >> -// CHECK: %class.C = type { %class.D*, %class.B } >> +// CHECK: %class.C = type <{ %class.D*, %class.B, [3 x i8] }> >> // CHECK: %class.D = type { %class.C.base, [3 x i8] } >> // CHECK: %class.C.base = type <{ %class.D*, %class.B }> >> // CHECK: %class.B = type { i8 } >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
