r218577 - CodeGen: Don't crash when initializing pointer-to-member fields in bases
Reid Kleckner
rnk at google.com
Mon Sep 29 09:52:37 PDT 2014
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/ac12f3b5/attachment.html>
More information about the cfe-commits
mailing list