r218577 - CodeGen: Don't crash when initializing pointer-to-member fields in bases
David Majnemer
david.majnemer at gmail.com
Mon Sep 29 10:16:27 PDT 2014
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 <rnk at google.com> 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 <david.majnemer at gmail.com
> > 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
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140929/de25b9e2/attachment.html>
More information about the cfe-commits
mailing list