From fe32c13589e89fd2a176dc32d269a2a7fc6d3372 Mon Sep 17 00:00:00 2001 From: Martin Kinkelin Date: Wed, 17 Jul 2024 14:30:44 +0200 Subject: [PATCH] Fix bit field IR storage from weird integer to i8 array Fixes #4646. --- CHANGELOG.md | 1 + gen/dvalue.cpp | 3 +++ gen/toir.cpp | 3 ++- ir/iraggr.cpp | 47 +++++++++++++++++++++++++++++++-------- ir/irtypeaggr.cpp | 11 ++++----- tests/compilable/gh4646.d | 31 ++++++++++++++++++++++++++ 6 files changed, 79 insertions(+), 17 deletions(-) create mode 100644 tests/compilable/gh4646.d diff --git a/CHANGELOG.md b/CHANGELOG.md index 15308ff1d05..63e107b8f2f 100644 --- 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 index 2673f891d86..2debc5c4054 100644 --- 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 index 003143f142e..b53c097fca0 100644 --- 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 index e99e3944a3f..d8b0bb1762f 100644 --- a/ir/iraggr.cpp +++ b/ir/iraggr.cpp @@ -27,6 +27,7 @@ #include "ir/irtypeclass.h" #include "ir/irtypestruct.h" #include +#include 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 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(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 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 index 21c1e9a10b3..3f49cc7c65e 100644 --- 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 new file mode 100644 index 00000000000..686f285061b --- /dev/null +++ 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);