Respect alignment of nested bitfields

[uweigand]

Summary

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 by chandlerc, 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.

However, this is incorrect; we do need to consider both these alignment values:

LV.getAlignment() is the alignment of the surrounding struct
                  (see e.g. EmitLValueForField code computing this value)
Info.StorageAlignment is the alignment of the storage backing
                      the bitfield relative to the struct

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).

This patch adds back logic to also consider alignment of the outer struct
in EmitLoadOfBitfieldLValue and EmitStoreThroughBitfieldLValue. It also
extends the test case to cover the case of storing into the nested bitfield.

(Patch originally posted to cfe-commits; now moved to Phabricator
to simplify review.)

[rjmccall]

Good catch!

The better fix would be to ignore the bitfield's abstract storage alignment and instead use the base l-value's alignment, adjusted with alignmentAtOffset to the offset of the bitfield's storage. This ensures we get the right alignment when the base l-value is *over*-aligned: in a packed struct, bitfield storage at offset 0 will have alignment 1, but if the packed struct itself is known to have alignment 4 (e.g., if it is embedded in an unpacked struct), then we still know that the storage has alignment 4.alignmentAtOffset(0) == 4.

Also, please use CreateAlignedStore and CreateAlignedLoad.

[uweigand]

Addressed review comments:

• Use CreateAlignedLoad / CreateAlignedStore

[uweigand]

In D11034#201452, @rjmccall wrote:

Good catch!

The better fix would be to ignore the bitfield's abstract storage alignment and instead use the base l-value's alignment, adjusted with alignmentAtOffset to the offset of the bitfield's storage. This ensures we get the right alignment when the base l-value is *over*-aligned: in a packed struct, bitfield storage at offset 0 will have alignment 1, but if the packed struct itself is known to have alignment 4 (e.g., if it is embedded in an unpacked struct), then we still know that the storage has alignment 4.alignmentAtOffset(0) == 4.

Good point. Unfortunately, at the point the access is generated, there doesn't appear to be an easy way to retrieve the offset of the bitfield's storage relative to the surrounding struct. This value is present as "StartOffset" in CGRecordLowering::setBitFieldInfo, but is not actually stored in the CGBitFieldInfo struct. Maybe instead of the StorageAlignment field, the CGBitFieldInfo struct should instead have a StorageOffset field?

Also, please use CreateAlignedStore and CreateAlignedLoad.

OK, done.

[rjmccall]

In D11034#201981, @uweigand wrote:

In D11034#201452, @rjmccall wrote:

Good catch!

The better fix would be to ignore the bitfield's abstract storage alignment and instead use the base l-value's alignment, adjusted with alignmentAtOffset to the offset of the bitfield's storage. This ensures we get the right alignment when the base l-value is *over*-aligned: in a packed struct, bitfield storage at offset 0 will have alignment 1, but if the packed struct itself is known to have alignment 4 (e.g., if it is embedded in an unpacked struct), then we still know that the storage has alignment 4.alignmentAtOffset(0) == 4.

Good point. Unfortunately, at the point the access is generated, there doesn't appear to be an easy way to retrieve the offset of the bitfield's storage relative to the surrounding struct. This value is present as "StartOffset" in CGRecordLowering::setBitFieldInfo, but is not actually stored in the CGBitFieldInfo struct. Maybe instead of the StorageAlignment field, the CGBitFieldInfo struct should instead have a StorageOffset field?

Storing the offset makes sense. If this means that we don't otherwise need StorageAlignment — probably true — then we can drop that at the same time.

[uweigand]

Replaced CGBitFieldInfo::StorageAlignment with a StorageOffset field.

[rjmccall]

LGTM, thanks!

lib/CodeGen/CGExpr.cpp

if (LV.isGlobalReg())
     return EmitLoadOfGlobalRegLValue(LV);
 
   assert(LV.isBitField() && "Unknown LValue type!");
   return EmitLoadOfBitfieldLValue(LV);
 }
 
 RValue CodeGenFunction::EmitLoadOfBitfieldLValue(LValue LV) {
   const CGBitFieldInfo &Info = LV.getBitFieldInfo();
+  int64_t Align = Info.StorageAlignment;
+  if (!LV.getAlignment().isZero())
+    Align = std::min(Align, LV.getAlignment().getQuantity());
 
   // 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);
+  cast<llvm::LoadInst>(Val)->setAlignment(Align);
 
   if (Info.IsSigned) {
     assert(static_cast<unsigned>(Info.Offset + Info.Size) <= Info.StorageSize);
     unsigned HighBits = Info.StorageSize - Info.Offset - Info.Size;
     if (HighBits)
       Val = Builder.CreateShl(Val, HighBits, "bf.shl");
     if (Info.Offset + HighBits)
       Val = Builder.CreateAShr(Val, Info.Offset + HighBits, "bf.ashr");
void CodeGenFunction::EmitStoreThroughLValue(RValue Src, LValue Dst,
 
   assert(Src.isScalar() && "Can't emit an agg store with this method");
   EmitStoreOfScalar(Src.getScalarVal(), Dst, isInit);
 }
 
 void CodeGenFunction::EmitStoreThroughBitfieldLValue(RValue Src, LValue Dst,
                                                      llvm::Value **Result) {
   const CGBitFieldInfo &Info = Dst.getBitFieldInfo();
+  int64_t Align = Info.StorageAlignment;
+  if (!Dst.getAlignment().isZero())
+    Align = std::min(Align, Dst.getAlignment().getQuantity());
   llvm::Type *ResLTy = ConvertTypeForMem(Dst.getType());
   llvm::Value *Ptr = Dst.getBitFieldAddr();
 
   // Get the source value, truncated to the width of the bit-field.
   llvm::Value *SrcVal = Src.getScalarVal();
 
   // Cast the source to the storage type and shift it into place.
   SrcVal = Builder.CreateIntCast(SrcVal,
                                  Ptr->getType()->getPointerElementType(),
                                  /*IsSigned=*/false);
   llvm::Value *MaskedVal = SrcVal;
 
   // See if there are other bits in the bitfield's storage we'll need to load
   // 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);
+    cast<llvm::LoadInst>(Val)->setAlignment(Align);
 
     // Mask the source value as needed.
     if (!hasBooleanRepresentation(Dst.getType()))
       SrcVal = Builder.CreateAnd(SrcVal,
                                  llvm::APInt::getLowBitsSet(Info.StorageSize,
                                                             Info.Size),
                                  "bf.value");
     MaskedVal = SrcVal;
if (Info.StorageSize != Info.Size) {
     SrcVal = Builder.CreateOr(Val, SrcVal, "bf.set");
   } else {
     assert(Info.Offset == 0);
   }
 
   // Write the new value back out.
   llvm::StoreInst *Store = Builder.CreateStore(SrcVal, Ptr,
                                                Dst.isVolatileQualified());
-  Store->setAlignment(Info.StorageAlignment);
+  Store->setAlignment(Align);
 
   // Return the new value of the bit-field, if requested.
   if (Result) {
     llvm::Value *ResultVal = MaskedVal;
 
     // Sign extend the value if needed.
     if (Info.IsSigned) {
       assert(Info.Size <= Info.StorageSize);

test/CodeGen/packed-nest-unpacked.c

 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
+  // 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;
+}
+