r241916 - Respect alignment of nested bitfields
David Majnemer
david.majnemer at gmail.com
Fri Jul 10 22:05:14 PDT 2015
On Fri, Jul 10, 2015 at 10:30 AM, Ulrich Weigand <ulrich.weigand at de.ibm.com>
wrote:
> Author: uweigand
> Date: Fri Jul 10 12:30:00 2015
> New Revision: 241916
>
> URL: http://llvm.org/viewvc/llvm-project?rev=241916&view=rev
> Log:
> Respect alignment of nested bitfields
>
> tools/clang/test/CodeGen/packed-nest-unpacked.c contains this test:
>
> struct XBitfield {
> unsigned b1 : 10;
> unsigned b2 : 12;
> unsigned b3 : 10;
> };
> struct YBitfield {
> char x;
> struct XBitfield y;
> } __attribute((packed));
> struct YBitfield gbitfield;
>
> unsigned test7() {
> // CHECK: @test7
> // CHECK: load i32, i32* getelementptr inbounds (%struct.YBitfield,
> %struct.YBitfield* @gbitfield, i32 0, i32 1, i32 0), align 4
> return gbitfield.y.b2;
> }
>
> The "align 4" is actually wrong. Accessing all of "gbitfield.y" as a
> single
> i32 is of course possible, but that still doesn't make it 4-byte aligned as
> it remains packed at offset 1 in the surrounding gbitfield object.
>
> This alignment was changed by commit r169489, which also introduced changes
> to bitfield access code in CGExpr.cpp. Code before that change used to
> take
> into account *both* the alignment of the field to be accessed within the
> current struct, *and* the alignment of that outer struct itself; this logic
> was removed by the above commit.
>
> Neglecting to consider both values can cause incorrect code to be generated
> (I've seen an unaligned access crash on SystemZ due to this bug).
>
> In order to always use the best known alignment value, this patch removes
> the CGBitFieldInfo::StorageAlignment member and replaces it with a
> StorageOffset member specifying the offset from the start of the
> surrounding
> struct to the bitfield's underlying storage. This offset can then be
> combined
> with the best-known alignment for a bitfield access lvalue to determine the
> alignment to use when accessing the bitfield's storage.
>
> Differential Revision: http://reviews.llvm.org/D11034
>
> Modified:
> cfe/trunk/lib/CodeGen/CGAtomic.cpp
> cfe/trunk/lib/CodeGen/CGClass.cpp
> cfe/trunk/lib/CodeGen/CGExpr.cpp
> cfe/trunk/lib/CodeGen/CGObjCRuntime.cpp
> cfe/trunk/lib/CodeGen/CGRecordLayout.h
> cfe/trunk/lib/CodeGen/CGRecordLayoutBuilder.cpp
> cfe/trunk/test/CodeGen/bitfield-2.c
> cfe/trunk/test/CodeGen/packed-nest-unpacked.c
>
> Modified: cfe/trunk/lib/CodeGen/CGAtomic.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGAtomic.cpp?rev=241916&r1=241915&r2=241916&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/CodeGen/CGAtomic.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGAtomic.cpp Fri Jul 10 12:30:00 2015
> @@ -93,6 +93,7 @@ namespace {
> BFI = OrigBFI;
> BFI.Offset = Offset;
> BFI.StorageSize = AtomicSizeInBits;
> + BFI.StorageOffset += OffsetInChars;
> LVal = LValue::MakeBitfield(Addr, BFI, lvalue.getType(),
> lvalue.getAlignment());
> LVal.setTBAAInfo(lvalue.getTBAAInfo());
>
> Modified: cfe/trunk/lib/CodeGen/CGClass.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGClass.cpp?rev=241916&r1=241915&r2=241916&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/CodeGen/CGClass.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGClass.cpp Fri Jul 10 12:30:00 2015
> @@ -911,32 +911,22 @@ namespace {
> return;
> }
>
> - CharUnits Alignment;
> -
> uint64_t FirstByteOffset;
> if (FirstField->isBitField()) {
> const CGRecordLayout &RL =
> CGF.getTypes().getCGRecordLayout(FirstField->getParent());
> const CGBitFieldInfo &BFInfo = RL.getBitFieldInfo(FirstField);
> - Alignment = CharUnits::fromQuantity(BFInfo.StorageAlignment);
> // FirstFieldOffset is not appropriate for bitfields,
> // it won't tell us what the storage offset should be and thus
> might not
> // be properly aligned.
> //
> // Instead calculate the storage offset using the offset of the
> field in
> // the struct type.
>
This comment seems out of date now that we are not using the LLVM struct
type's layout to calculate FirstByteOffset.
> - const llvm::DataLayout &DL = CGF.CGM.getDataLayout();
> - FirstByteOffset =
> - DL.getStructLayout(RL.getLLVMType())
> - ->getElementOffsetInBits(RL.getLLVMFieldNo(FirstField));
> + FirstByteOffset = CGF.getContext().toBits(BFInfo.StorageOffset);
> } else {
> - Alignment = CGF.getContext().getDeclAlign(FirstField);
> FirstByteOffset = FirstFieldOffset;
> }
>
> - assert((CGF.getContext().toCharUnitsFromBits(FirstByteOffset) %
> - Alignment) == 0 && "Bad field alignment.");
> -
> CharUnits MemcpySize = getMemcpySize(FirstByteOffset);
> QualType RecordTy = CGF.getContext().getTypeDeclType(ClassDecl);
> llvm::Value *ThisPtr = CGF.LoadCXXThis();
> @@ -946,6 +936,9 @@ namespace {
> LValue SrcLV = CGF.MakeNaturalAlignAddrLValue(SrcPtr, RecordTy);
> LValue Src = CGF.EmitLValueForFieldInitialization(SrcLV,
> FirstField);
>
> + CharUnits Offset =
> CGF.getContext().toCharUnitsFromBits(FirstByteOffset);
> + CharUnits Alignment =
> DestLV.getAlignment().alignmentAtOffset(Offset);
> +
> emitMemcpyIR(Dest.isBitField() ? Dest.getBitFieldAddr() :
> Dest.getAddress(),
> Src.isBitField() ? Src.getBitFieldAddr() :
> Src.getAddress(),
> MemcpySize, Alignment);
>
> Modified: cfe/trunk/lib/CodeGen/CGExpr.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExpr.cpp?rev=241916&r1=241915&r2=241916&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/CodeGen/CGExpr.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGExpr.cpp Fri Jul 10 12:30:00 2015
> @@ -1356,14 +1356,15 @@ RValue CodeGenFunction::EmitLoadOfLValue
>
> RValue CodeGenFunction::EmitLoadOfBitfieldLValue(LValue LV) {
> const CGBitFieldInfo &Info = LV.getBitFieldInfo();
> + CharUnits Align =
> LV.getAlignment().alignmentAtOffset(Info.StorageOffset);
>
> // Get the output type.
> llvm::Type *ResLTy = ConvertType(LV.getType());
>
> llvm::Value *Ptr = LV.getBitFieldAddr();
> - llvm::Value *Val = Builder.CreateLoad(Ptr, LV.isVolatileQualified(),
> - "bf.load");
> - cast<llvm::LoadInst>(Val)->setAlignment(Info.StorageAlignment);
> + llvm::Value *Val = Builder.CreateAlignedLoad(Ptr, Align.getQuantity(),
> + LV.isVolatileQualified(),
> + "bf.load");
>
> if (Info.IsSigned) {
> assert(static_cast<unsigned>(Info.Offset + Info.Size) <=
> Info.StorageSize);
> @@ -1559,6 +1560,7 @@ void CodeGenFunction::EmitStoreThroughLV
> void CodeGenFunction::EmitStoreThroughBitfieldLValue(RValue Src, LValue
> Dst,
> llvm::Value
> **Result) {
> const CGBitFieldInfo &Info = Dst.getBitFieldInfo();
> + CharUnits Align =
> Dst.getAlignment().alignmentAtOffset(Info.StorageOffset);
> llvm::Type *ResLTy = ConvertTypeForMem(Dst.getType());
> llvm::Value *Ptr = Dst.getBitFieldAddr();
>
> @@ -1575,9 +1577,9 @@ void CodeGenFunction::EmitStoreThroughBi
> // and mask together with source before storing.
> if (Info.StorageSize != Info.Size) {
> assert(Info.StorageSize > Info.Size && "Invalid bitfield size.");
> - llvm::Value *Val = Builder.CreateLoad(Ptr, Dst.isVolatileQualified(),
> - "bf.load");
> - cast<llvm::LoadInst>(Val)->setAlignment(Info.StorageAlignment);
> + llvm::Value *Val = Builder.CreateAlignedLoad(Ptr, Align.getQuantity(),
> +
> Dst.isVolatileQualified(),
> + "bf.load");
>
> // Mask the source value as needed.
> if (!hasBooleanRepresentation(Dst.getType()))
> @@ -1603,9 +1605,8 @@ void CodeGenFunction::EmitStoreThroughBi
> }
>
> // Write the new value back out.
> - llvm::StoreInst *Store = Builder.CreateStore(SrcVal, Ptr,
> - Dst.isVolatileQualified());
> - Store->setAlignment(Info.StorageAlignment);
> + Builder.CreateAlignedStore(SrcVal, Ptr, Align.getQuantity(),
> + Dst.isVolatileQualified());
>
> // Return the new value of the bit-field, if requested.
> if (Result) {
>
> Modified: cfe/trunk/lib/CodeGen/CGObjCRuntime.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGObjCRuntime.cpp?rev=241916&r1=241915&r2=241916&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/CodeGen/CGObjCRuntime.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGObjCRuntime.cpp Fri Jul 10 12:30:00 2015
> @@ -134,7 +134,7 @@ LValue CGObjCRuntime::EmitValueForIvarAt
> CGBitFieldInfo *Info = new (CGF.CGM.getContext()) CGBitFieldInfo(
> CGBitFieldInfo::MakeInfo(CGF.CGM.getTypes(), Ivar, BitOffset,
> BitFieldSize,
> CGF.CGM.getContext().toBits(StorageSize),
> - Alignment.getQuantity()));
> + CharUnits::fromQuantity(0)));
>
> V = CGF.Builder.CreateBitCast(V,
>
> llvm::Type::getIntNPtrTy(CGF.getLLVMContext(),
>
> Modified: cfe/trunk/lib/CodeGen/CGRecordLayout.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGRecordLayout.h?rev=241916&r1=241915&r2=241916&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/CodeGen/CGRecordLayout.h (original)
> +++ cfe/trunk/lib/CodeGen/CGRecordLayout.h Fri Jul 10 12:30:00 2015
> @@ -78,16 +78,16 @@ struct CGBitFieldInfo {
> /// bitfield.
> unsigned StorageSize;
>
> - /// The alignment which should be used when accessing the bitfield.
> - unsigned StorageAlignment;
> + /// The offset of the bitfield storage from the start of the struct.
> + CharUnits StorageOffset;
>
> CGBitFieldInfo()
> - : Offset(), Size(), IsSigned(), StorageSize(), StorageAlignment() {}
> + : Offset(), Size(), IsSigned(), StorageSize(), StorageOffset() {}
>
> CGBitFieldInfo(unsigned Offset, unsigned Size, bool IsSigned,
> - unsigned StorageSize, unsigned StorageAlignment)
> + unsigned StorageSize, CharUnits StorageOffset)
> : Offset(Offset), Size(Size), IsSigned(IsSigned),
> - StorageSize(StorageSize), StorageAlignment(StorageAlignment) {}
> + StorageSize(StorageSize), StorageOffset(StorageOffset) {}
>
> void print(raw_ostream &OS) const;
> void dump() const;
> @@ -99,7 +99,7 @@ struct CGBitFieldInfo {
> const FieldDecl *FD,
> uint64_t Offset, uint64_t Size,
> uint64_t StorageSize,
> - uint64_t StorageAlignment);
> + CharUnits StorageOffset);
> };
>
> /// CGRecordLayout - This class handles struct and union layout info while
>
> Modified: cfe/trunk/lib/CodeGen/CGRecordLayoutBuilder.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGRecordLayoutBuilder.cpp?rev=241916&r1=241915&r2=241916&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/CodeGen/CGRecordLayoutBuilder.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGRecordLayoutBuilder.cpp Fri Jul 10 12:30:00
> 2015
> @@ -228,11 +228,7 @@ void CGRecordLowering::setBitFieldInfo(
> Info.Offset = (unsigned)(getFieldBitOffset(FD) -
> Context.toBits(StartOffset));
> Info.Size = FD->getBitWidthValue(Context);
> Info.StorageSize =
> (unsigned)DataLayout.getTypeAllocSizeInBits(StorageType);
> - // Here we calculate the actual storage alignment of the bits. E.g if
> we've
> - // got an alignment >= 2 and the bitfield starts at offset 6 we've got
> an
> - // alignment of 2.
> - Info.StorageAlignment =
> - Layout.getAlignment().alignmentAtOffset(StartOffset).getQuantity();
> + Info.StorageOffset = StartOffset;
> if (Info.Size > Info.StorageSize)
> Info.Size = Info.StorageSize;
> // Reverse the bit offsets for big endian machines. Because we represent
> @@ -651,7 +647,7 @@ CGBitFieldInfo CGBitFieldInfo::MakeInfo(
> const FieldDecl *FD,
> uint64_t Offset, uint64_t Size,
> uint64_t StorageSize,
> - uint64_t StorageAlignment) {
> + CharUnits StorageOffset) {
> // This function is vestigial from CGRecordLayoutBuilder days but is
> still
> // used in GCObjCRuntime.cpp. That usage has a "fixme" attached to it
> that
> // when addressed will allow for the removal of this function.
> @@ -683,7 +679,7 @@ CGBitFieldInfo CGBitFieldInfo::MakeInfo(
> Offset = StorageSize - (Offset + Size);
> }
>
> - return CGBitFieldInfo(Offset, Size, IsSigned, StorageSize,
> StorageAlignment);
> + return CGBitFieldInfo(Offset, Size, IsSigned, StorageSize,
> StorageOffset);
> }
>
> CGRecordLayout *CodeGenTypes::ComputeRecordLayout(const RecordDecl *D,
> @@ -856,7 +852,7 @@ void CGBitFieldInfo::print(raw_ostream &
> << " Size:" << Size
> << " IsSigned:" << IsSigned
> << " StorageSize:" << StorageSize
> - << " StorageAlignment:" << StorageAlignment << ">";
> + << " StorageOffset:" << StorageOffset.getQuantity() << ">";
> }
>
> void CGBitFieldInfo::dump() const {
>
> Modified: cfe/trunk/test/CodeGen/bitfield-2.c
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/bitfield-2.c?rev=241916&r1=241915&r2=241916&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/CodeGen/bitfield-2.c (original)
> +++ cfe/trunk/test/CodeGen/bitfield-2.c Fri Jul 10 12:30:00 2015
> @@ -14,7 +14,7 @@
> // CHECK-RECORD: LLVMType:%struct.s0 = type { [3 x i8] }
> // CHECK-RECORD: IsZeroInitializable:1
> // CHECK-RECORD: BitFields:[
> -// CHECK-RECORD: <CGBitFieldInfo Offset:0 Size:24 IsSigned:1
> StorageSize:24 StorageAlignment:1>
> +// CHECK-RECORD: <CGBitFieldInfo Offset:0 Size:24 IsSigned:1
> StorageSize:24 StorageOffset:0>
> struct __attribute((packed)) s0 {
> int f0 : 24;
> };
> @@ -54,8 +54,8 @@ unsigned long long test_0() {
> // CHECK-RECORD: LLVMType:%struct.s1 = type { [3 x i8] }
> // CHECK-RECORD: IsZeroInitializable:1
> // CHECK-RECORD: BitFields:[
> -// CHECK-RECORD: <CGBitFieldInfo Offset:0 Size:10 IsSigned:1
> StorageSize:24 StorageAlignment:1>
> -// CHECK-RECORD: <CGBitFieldInfo Offset:10 Size:10 IsSigned:1
> StorageSize:24 StorageAlignment:1>
> +// CHECK-RECORD: <CGBitFieldInfo Offset:0 Size:10 IsSigned:1
> StorageSize:24 StorageOffset:0>
> +// CHECK-RECORD: <CGBitFieldInfo Offset:10 Size:10 IsSigned:1
> StorageSize:24 StorageOffset:0>
>
> #pragma pack(push)
> #pragma pack(1)
> @@ -102,7 +102,7 @@ unsigned long long test_1() {
> // CHECK-RECORD: LLVMType:%union.u2 = type { i8 }
> // CHECK-RECORD: IsZeroInitializable:1
> // CHECK-RECORD: BitFields:[
> -// CHECK-RECORD: <CGBitFieldInfo Offset:0 Size:3 IsSigned:0
> StorageSize:8 StorageAlignment:1>
> +// CHECK-RECORD: <CGBitFieldInfo Offset:0 Size:3 IsSigned:0
> StorageSize:8 StorageOffset:0>
>
> union __attribute__((packed)) u2 {
> unsigned long long f0 : 3;
> @@ -274,8 +274,8 @@ _Bool test_6() {
> // CHECK-RECORD: LLVMType:%struct.s7 = type { i32, i32, i32, i8, i32,
> [12 x i8] }
> // CHECK-RECORD: IsZeroInitializable:1
> // CHECK-RECORD: BitFields:[
> -// CHECK-RECORD: <CGBitFieldInfo Offset:0 Size:5 IsSigned:1
> StorageSize:8 StorageAlignment:4>
> -// CHECK-RECORD: <CGBitFieldInfo Offset:0 Size:29 IsSigned:1
> StorageSize:32 StorageAlignment:16>
> +// CHECK-RECORD: <CGBitFieldInfo Offset:0 Size:5 IsSigned:1
> StorageSize:8 StorageOffset:12>
> +// CHECK-RECORD: <CGBitFieldInfo Offset:0 Size:29 IsSigned:1
> StorageSize:32 StorageOffset:16>
>
> struct __attribute__((aligned(16))) s7 {
> int a, b, c;
>
> Modified: cfe/trunk/test/CodeGen/packed-nest-unpacked.c
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/packed-nest-unpacked.c?rev=241916&r1=241915&r2=241916&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/CodeGen/packed-nest-unpacked.c (original)
> +++ cfe/trunk/test/CodeGen/packed-nest-unpacked.c Fri Jul 10 12:30:00 2015
> @@ -60,6 +60,35 @@ struct YBitfield gbitfield;
>
> unsigned test7() {
> // CHECK: @test7
> - // CHECK: load i32, i32* getelementptr inbounds (%struct.YBitfield,
> %struct.YBitfield* @gbitfield, i32 0, i32 1, i32 0), align 4
> + // CHECK: load i32, i32* getelementptr inbounds (%struct.YBitfield,
> %struct.YBitfield* @gbitfield, i32 0, i32 1, i32 0), align 1
> return gbitfield.y.b2;
> }
> +
> +void test8(unsigned x) {
> + // CHECK: @test8
> + // CHECK: load i32, i32* getelementptr inbounds (%struct.YBitfield,
> %struct.YBitfield* @gbitfield, i32 0, i32 1, i32 0), align 1
> + // CHECK: store i32 {{.*}}, i32* getelementptr inbounds
> (%struct.YBitfield, %struct.YBitfield* @gbitfield, i32 0, i32 1, i32 0),
> align 1
> + gbitfield.y.b2 = x;
> +}
> +
> +struct TBitfield
> +{
> + long a;
> + char b;
> + unsigned c:15;
> +};
> +struct TBitfield tbitfield;
> +
> +unsigned test9() {
> + // CHECK: @test9
> + // CHECK: load i16, i16* getelementptr inbounds (%struct.TBitfield,
> %struct.TBitfield* @tbitfield, i32 0, i32 2), align 1
> + return tbitfield.c;
> +}
> +
> +void test10(unsigned x) {
> + // CHECK: @test10
> + // CHECK: load i16, i16* getelementptr inbounds (%struct.TBitfield,
> %struct.TBitfield* @tbitfield, i32 0, i32 2), align 1
> + // CHECK: store i16 {{.*}}, i16* getelementptr inbounds
> (%struct.TBitfield, %struct.TBitfield* @tbitfield, i32 0, i32 2), align 1
> + tbitfield.c = x;
> +}
> +
>
>
> _______________________________________________
> 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/20150710/e4dce0ed/attachment.html>
More information about the cfe-commits
mailing list