[Clang][CodeGen] Fixing mismatch between memory layout and const expressions for oversized bitfields

[pratlucas]

Summary

The construction of constants for structs/unions was conflicting the
expected memory layout for over-sized bit-fields. When building the
necessary bits for those fields, clang was ignoring the size information
computed for the struct/union memory layout and using the original data
from the AST's FieldDecl information. This caused an issue in big-endian
targets, where the field's contant was incorrectly misplaced due to
endian calculations.

This patch aims to separate the constant value from the necessary
padding bits, using the proper size information for each one of them.
With this, the layout of constants for over-sized bit-fields matches the
ABI requirements.

[pratlucas]

Formatting.

[efriedma]

clang/lib/CodeGen/CGExprConstant.cpp

613

The existing code in ConstantAggregateBuilder::add should skip over padding. I'm not sure what you're trying to accomplish by adding more code dealing with padding here.

[pratlucas]

Removing unecessary handling of padding bits.

[pratlucas]

clang/lib/CodeGen/CGExprConstant.cpp

613

You're right, I missed the fact that FieldOffsets take care of this.

[efriedma]

LGTM

clang/lib/CodeGen/CGExprConstant.cpp

bool ConstStructBuilder::AppendBytes(CharUnits FieldOffsetInChars,
                                      llvm::Constant *InitCst,
                                      bool AllowOverwrite) {
   return Builder.add(InitCst, StartOffset + FieldOffsetInChars, AllowOverwrite);
 }
 
 bool ConstStructBuilder::AppendBitField(
     const FieldDecl *Field, uint64_t FieldOffset, llvm::ConstantInt *CI,
     bool AllowOverwrite) {
-  uint64_t FieldSize = Field->getBitWidthValue(CGM.getContext());
+  const CGRecordLayout &RL =
+    CGM.getTypes().getCGRecordLayout(Field->getParent());
+  const CGBitFieldInfo &Info = RL.getBitFieldInfo(Field);
   llvm::APInt FieldValue = CI->getValue();
 
   // Promote the size of FieldValue if necessary
   // FIXME: This should never occur, but currently it can because initializer
   // constants are cast to bool, and because clang is not enforcing bitfield
   // width limits.
-  if (FieldSize > FieldValue.getBitWidth())
-    FieldValue = FieldValue.zext(FieldSize);
+  if (Info.Size > FieldValue.getBitWidth())
+    FieldValue = FieldValue.zext(Info.Size);
 
   // Truncate the size of FieldValue to the bit field size.
-  if (FieldSize < FieldValue.getBitWidth())
-    FieldValue = FieldValue.trunc(FieldSize);
+  if (Info.Size < FieldValue.getBitWidth())
+    FieldValue = FieldValue.trunc(Info.Size);
 
-  return Builder.addBits(FieldValue,
-                         CGM.getContext().toBits(StartOffset) + FieldOffset,
+  // Add field value bits
+  uint64_t AdjustedOffset = CGM.getContext().toBits(StartOffset) + FieldOffset;
+  if (!Builder.addBits(FieldValue, AdjustedOffset, AllowOverwrite))
+    return false;
+  
+  // Add padding bits in case of over-sized bit-field.
+  //   "The first sizeof(T)*8 bits are used to hold the value of the bit-field,
+  //   followed by n - sizeof(T)*8 bits of padding."
+  uint64_t FieldWidth = Field->getBitWidthValue(CGM.getContext());
+  if (FieldWidth > Info.Size) {
+    llvm::APInt PaddingValue((FieldWidth - Info.Size), /*val=*/0);
+    return Builder.addBits(PaddingValue, (AdjustedOffset + Info.Size),
                           AllowOverwrite);
   }
+  return true;
+}
 
 static bool EmitDesignatedInitUpdater(ConstantEmitter &Emitter,
                                       ConstantAggregateBuilder &Const,
                                       CharUnits Offset, QualType Type,
                                       InitListExpr *Updater) {
   if (Type->isRecordType())
     return ConstStructBuilder::UpdateStruct(Emitter, Const, Offset, Updater);
 

clang/test/CodeGenCXX/bitfield-layout.cpp

-// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin10 -emit-llvm -o - -O3 | FileCheck -check-prefix CHECK-LP64 %s
-// RUN: %clang_cc1 %s -triple=i386-apple-darwin10 -emit-llvm -o - -O3 | FileCheck -check-prefix CHECK-LP32 %s
+// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin10 -emit-llvm -o - -O3 | FileCheck -check-prefix=CHECK-LP64 -check-prefix=CHECK %s
+// RUN: %clang_cc1 %s -triple=i386-apple-darwin10 -emit-llvm -o - -O3 | FileCheck -check-prefix CHECK-LP32 -check-prefix=CHECK %s
+// RUN: %clang_cc1 %s -triple=aarch64_be-none-eabi -emit-llvm -o - -O3 | FileCheck -check-prefix CHECK-A64BE -check-prefix=CHECK %s
+// RUN: %clang_cc1 %s -triple=thumbv7_be-none-eabi -emit-llvm -o - -O3 | FileCheck -check-prefix CHECK-A32BE -check-prefix=CHECK %s
 
 // CHECK-LP64: %union.Test1 = type { i32, [4 x i8] }
 union Test1 {
   int a;
   int b: 39;
-} t1;
+};
+Test1 t1;
 
 // CHECK-LP64: %union.Test2 = type { i8 }
 union Test2 {
   int : 6;
 } t2;
 
 // CHECK-LP64: %union.Test3 = type { i16 }
 union Test3 {
   int : 9;
 } t3;
 
+// CHECK: %union.Test4 = type { i8, i8 }
+union Test4 {
+  char val : 16;
+};
+Test4 t4;
 
 #define CHECK(x) if (!(x)) return __LINE__
 
-int f() {
+// CHECK: define i32 @_Z11test_assignv()
+int test_assign() {
   struct {
     int a;
 
     unsigned long long b : 65;
 
     int c;
   } c;
   
   c.a = 0;
   c.b = (unsigned long long)-1;
   c.c = 0;
 
   CHECK(c.a == 0);
   CHECK(c.b == (unsigned long long)-1);
   CHECK(c.c == 0);
 
-// CHECK-LP64: ret i32 0
-// CHECK-LP32: ret i32 0
+  Test1 u1;
+  Test4 u2;
+
+  u1.b = 1;
+  u2.val = 42;
+
+  CHECK(u1.b == 1);
+  CHECK(u2.val == 42);
+
+// CHECK: ret i32 0
+  return 0;
+}
+
+// CHECK: define i32 @_Z9test_initv()
+int test_init() {
+  struct S {
+    int a;
+
+    unsigned long long b : 65;
+
+    int c;
+  };
+  S s1 = {1, 42, 0};
+
+  CHECK(s1.a == 1);
+  CHECK(s1.b == (unsigned long long)42);
+  CHECK(s1.c == 0);
+
+  Test1 u1 = {1};
+  Test4 u2 = {42};
+
+  CHECK(u1.a == 1);
+  CHECK(u1.b == 1);
+  CHECK(u2.val == 42);
+
+// CHECK: ret i32 0
   return 0;
 }
+