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

[Lucas Prates via Phabricator via cfe-commits]

pratlucas created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77048

Files:
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/test/CodeGenCXX/bitfield-layout.cpp


Index: clang/test/CodeGenCXX/bitfield-layout.cpp
===================================================================
--- clang/test/CodeGenCXX/bitfield-layout.cpp
+++ clang/test/CodeGenCXX/bitfield-layout.cpp
@@ -1,11 +1,14 @@
-// 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 {
@@ -17,10 +20,16 @@
   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;
 
@@ -37,7 +46,42 @@
   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;
 }
+
Index: clang/lib/CodeGen/CGExprConstant.cpp
===================================================================
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -589,23 +589,37 @@
 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,
-                         AllowOverwrite);
+  // 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,

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits