Fix bit field IR storage from weird integer to i8 array

Fixes ldc-developers#4646.
kinke · Jul 17, 2024 · fe32c13 · fe32c13
1 parent da4a6e4
commit fe32c13
Show file tree

Hide file tree

Showing 6 changed files with 79 additions and 17 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@
 #### Platform support
 
 #### Bug fixes
+- Fix potentially corrupt IR layouts for bit fields. (#4646, #4708)
 
 # LDC 1.39.0 (2024-07-04)
 

diff --git a/gen/dvalue.cpp b/gen/dvalue.cpp
@@ -183,6 +183,7 @@ DRValue *DBitFieldLValue::getRVal() {
   const auto sizeInBits = intType->getBitWidth();
   const auto ptr = val;
   LLValue *v = gIR->ir->CreateAlignedLoad(intType, ptr, llvm::MaybeAlign(1));
+  // TODO: byte-swap v for big-endian targets?
 
   if (bf->type->isunsigned()) {
     if (auto n = bf->bitOffset)
@@ -212,6 +213,7 @@ void DBitFieldLValue::store(LLValue *value) {
       llvm::APInt::getLowBitsSet(intType->getBitWidth(), bf->fieldWidth);
   const auto oldVal =
       gIR->ir->CreateAlignedLoad(intType, ptr, llvm::MaybeAlign(1));
+  // TODO: byte-swap oldVal for big-endian targets?
   const auto maskedOldVal =
       gIR->ir->CreateAnd(oldVal, ~(mask << bf->bitOffset));
 
@@ -221,6 +223,7 @@ void DBitFieldLValue::store(LLValue *value) {
     bfVal = gIR->ir->CreateShl(bfVal, n);
 
   const auto newVal = gIR->ir->CreateOr(maskedOldVal, bfVal);
+  // TODO: byte-swap newVal for big-endian targets?
   gIR->ir->CreateAlignedStore(newVal, ptr, llvm::MaybeAlign(1));
 }
 

diff --git a/gen/toir.cpp b/gen/toir.cpp
@@ -139,7 +139,7 @@ static void write_struct_literal(Loc loc, LLValue *mem, StructDeclaration *sd,
       // get a pointer to this group's IR field
       const auto ptr = DtoLVal(DtoIndexAggregate(mem, sd, vd));
 
-      // merge all initializers to a single value
+      // merge all initializers to a single integer value
       const auto intType =
           LLIntegerType::get(gIR->context(), group.sizeInBytes * 8);
       LLValue *val = LLConstant::getNullValue(intType);
@@ -165,6 +165,7 @@ static void write_struct_literal(Loc loc, LLValue *mem, StructDeclaration *sd,
       }
 
       IF_LOG Logger::cout() << "merged IR value: " << *val << '\n';
+      // TODO: byte-swap val for big-endian targets?
       gIR->ir->CreateAlignedStore(val, ptr, llvm::MaybeAlign(1));
       offset += group.sizeInBytes;
 

diff --git a/ir/iraggr.cpp b/ir/iraggr.cpp
@@ -27,6 +27,7 @@
 #include "ir/irtypeclass.h"
 #include "ir/irtypestruct.h"
 #include <algorithm>
+#include <unordered_map>
 
 using namespace dmd;
 
@@ -257,6 +258,8 @@ void IrAggr::addFieldInitializers(
     const VarInitMap &explicitInitializers, AggregateDeclaration *decl,
     unsigned &offset, unsigned &interfaceVtblIndex, bool &isPacked) {
 
+  using llvm::APInt;
+
   if (ClassDeclaration *cd = decl->isClassDeclaration()) {
     if (cd->baseClass) {
       addFieldInitializers(constants, explicitInitializers, cd->baseClass,
@@ -302,6 +305,9 @@ void IrAggr::addFieldInitializers(
                : getDefaultInitializer(field);
   };
 
+  // IR field index => APInt for bitfield group
+  std::unordered_map<unsigned, APInt> bitFieldGroupConstants;
+
   const auto addToBitFieldGroup = [&](BitFieldDeclaration *bf,
                                       unsigned fieldIndex, unsigned bitOffset) {
     LLConstant *init = getFieldInit(bf);
@@ -310,20 +316,24 @@ void IrAggr::addFieldInitializers(
       return;
     }
 
-    LLConstant *&constant = constants[baseLLFieldIndex + fieldIndex];
+    if (bitFieldGroupConstants.find(fieldIndex) ==
+        bitFieldGroupConstants.end()) {
+      const auto fieldType =
+          llvm::cast<LLArrayType>(b.defaultTypes()[fieldIndex]); // an i8 array
+      const auto bitFieldSize = fieldType->getNumElements();
+      bitFieldGroupConstants.emplace(fieldIndex, APInt(bitFieldSize * 8, 0));
+    }
+
+    APInt &constant = bitFieldGroupConstants[fieldIndex];
+    const auto intSizeInBits = constant.getBitWidth();
 
-    using llvm::APInt;
-    const auto fieldType = b.defaultTypes()[fieldIndex];
-    const auto intSizeInBits = fieldType->getIntegerBitWidth();
-    const APInt oldVal =
-        constant ? constant->getUniqueInteger() : APInt(intSizeInBits, 0);
+    const APInt oldVal = constant;
     const APInt bfVal = init->getUniqueInteger().zextOrTrunc(intSizeInBits);
     const APInt mask = APInt::getLowBitsSet(intSizeInBits, bf->fieldWidth)
                        << bitOffset;
     assert(!oldVal.intersects(mask) && "has masked bits set already");
-    const APInt newVal = oldVal | ((bfVal << bitOffset) & mask);
 
-    constant = LLConstant::getIntegerValue(fieldType, newVal);
+    constant = oldVal | ((bfVal << bitOffset) & mask);
   };
 
   // add explicit and non-overlapping implicit initializers
@@ -333,7 +343,7 @@ void IrAggr::addFieldInitializers(
     const auto fieldIndex = pair.second;
 
     if (auto bf = field->isBitFieldDeclaration()) {
-      // multiple bit fields can map to a single IR field (of integer type)
+      // multiple bit fields can map to a single IR field for the whole group
       addToBitFieldGroup(bf, fieldIndex, bf->bitOffset);
     } else {
       LLConstant *&constant = constants[baseLLFieldIndex + fieldIndex];
@@ -356,6 +366,25 @@ void IrAggr::addFieldInitializers(
                        (bf->offset - primary->offset) * 8 + bf->bitOffset);
   }
 
+  for (const auto &pair : bitFieldGroupConstants) {
+    const unsigned fieldIndex = pair.first;
+    const APInt intValue = pair.second;
+
+    LLConstant *&constant = constants[baseLLFieldIndex + fieldIndex];
+    assert(!constant && "already have a constant for a bitfield group?");
+
+    // convert APInt to i8 array
+    const auto i8Type = getI8Type();
+    const auto numBytes = intValue.getBitWidth() / 8;
+    llvm::SmallVector<LLConstant *, 8> bytes;
+    for (unsigned i = 0; i < numBytes; ++i) {
+      APInt byteVal = intValue.extractBits(8, i * 8);
+      bytes.push_back(LLConstant::getIntegerValue(i8Type, byteVal));
+    }
+
+    constant = llvm::ConstantArray::get(LLArrayType::get(i8Type, numBytes), bytes);
+  }
+
   // TODO: sanity check that all explicit initializers have been dealt with?
   //       (potential issue for bitfields in unions...)
 

diff --git a/ir/irtypeaggr.cpp b/ir/irtypeaggr.cpp
@@ -146,8 +146,9 @@ void AggrTypeBuilder::addAggregate(
       const uint64_t f_begin = field->offset;
       const uint64_t f_end = f_begin + fieldSize;
 
+      // use an i8 array for bit field groups
       const auto llType =
-          isBitField ? LLIntegerType::get(gIR->context(), fieldSize * 8)
+          isBitField ? llvm::ArrayType::get(getI8Type(), fieldSize)
                      : DtoMemType(field->type);
 
       // check for overlap with existing fields (on a byte level, not bits)
@@ -212,12 +213,8 @@ void AggrTypeBuilder::addAggregate(
       fieldSize = af.size;
     } else {
       fieldAlignment = getABITypeAlign(llType);
-      if (vd->isBitFieldDeclaration()) {
-        fieldSize = af.size; // an IR integer of possibly non-power-of-2 size
-      } else {
-        fieldSize = getTypeAllocSize(llType);
-        assert(fieldSize <= af.size);
-      }
+      fieldSize = getTypeAllocSize(llType);
+      assert(fieldSize <= af.size);
     }
 
     // advance offset to right past this field

diff --git a/tests/compilable/gh4646.d b/tests/compilable/gh4646.d
@@ -0,0 +1,31 @@
+// RUN: %ldc -c -preview=bitfields %s
+
+struct BitField {
+    uint _0 : 1;
+    uint _1 : 1;
+    uint _2 : 1;
+    uint _3 : 1;
+    uint _4 : 1;
+    uint _5 : 1;
+    uint _6 : 1;
+    uint _7 : 1;
+    uint _8 : 1;
+    uint _9 : 1;
+    uint _10 : 1;
+    uint _11 : 1;
+    uint _12 : 1;
+    uint _13 : 1;
+    uint _14 : 1;
+    uint _15 : 1;
+    uint _16 : 1;
+}
+
+static assert(BitField.sizeof == 4);
+static assert(BitField.alignof == 4);
+
+struct Foo {
+    BitField bf;
+}
+
+static assert(Foo.sizeof == 4);
+static assert(Foo.alignof == 4);