[PATCH] D11034: Respect alignment of nested bitfields

[Ulrich Weigand]

uweigand created this revision.
uweigand added reviewers: rjmccall, rsmith, chandlerc.
uweigand added a subscriber: cfe-commits.

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

http://reviews.llvm.org/D11034

Files:
  lib/CodeGen/CGExpr.cpp
  test/CodeGen/packed-nest-unpacked.c

Index: test/CodeGen/packed-nest-unpacked.c
===================================================================
--- test/CodeGen/packed-nest-unpacked.c
+++ test/CodeGen/packed-nest-unpacked.c
@@ -60,6 +60,35 @@
 
 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;
+}
+
Index: lib/CodeGen/CGExpr.cpp
===================================================================
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -1356,14 +1356,17 @@
 
 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);
@@ -1559,6 +1562,9 @@
 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();
 
@@ -1577,7 +1583,7 @@
     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()))
@@ -1605,7 +1611,7 @@
   // 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) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D11034.29261.patch
Type: text/x-patch
Size: 3696 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150708/c11769ce/attachment.bin>