On Fri, Jul 10, 2015 at 10:30 AM, Ulrich Weigand <ulrich.weig...@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@cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits