From ba9af8d3d17113f2e5334fa2cee748e96fb7421e Mon Sep 17 00:00:00 2001 From: Martin Kinkelin Date: Sun, 18 Aug 2024 15:20:16 +0200 Subject: [PATCH 1/4] Tighten checks for IR aggregate sizes --- ir/iraggr.cpp | 7 ++++++- ir/irtypeclass.cpp | 7 +++++++ ir/irtypestruct.cpp | 6 ++++++ tests/compilable/gh4734.d | 20 ++++++++++++++++++++ 4 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 tests/compilable/gh4734.d diff --git a/ir/iraggr.cpp b/ir/iraggr.cpp index faca04f25c4..a19867af544 100644 --- a/ir/iraggr.cpp +++ b/ir/iraggr.cpp @@ -11,6 +11,7 @@ #include "dmd/aggregate.h" #include "dmd/declaration.h" +#include "dmd/errors.h" #include "dmd/expression.h" #include "dmd/identifier.h" #include "dmd/init.h" @@ -217,8 +218,12 @@ IrAggr::createInitializerConstant(const VarInitMap &explicitInitializers) { // tail padding? const size_t structsize = aggrdecl->size(Loc()); - if (offset < structsize) + if (offset < structsize) { add_zeros(constants, offset, structsize); + } else if (offset > structsize) { + error(Loc(), "ICE: IR aggregate constant size exceeds the frontend size"); + fatal(); + } // get LL field types llvm::SmallVector types; diff --git a/ir/irtypeclass.cpp b/ir/irtypeclass.cpp index e29b7460706..ad35dacdbc5 100644 --- a/ir/irtypeclass.cpp +++ b/ir/irtypeclass.cpp @@ -12,6 +12,7 @@ #include "dmd/aggregate.h" #include "dmd/declaration.h" #include "dmd/dsymbol.h" +#include "dmd/errors.h" #include "dmd/mtype.h" #include "dmd/target.h" #include "dmd/template.h" @@ -98,6 +99,12 @@ IrTypeClass *IrTypeClass::get(ClassDeclaration *cd) { isaStruct(t->type)->setBody(builder.defaultTypes(), builder.isPacked()); t->varGEPIndices = builder.varGEPIndices(); + if (!cd->isInterfaceDeclaration() && instanceSize && + getTypeAllocSize(t->type) != instanceSize) { + error(cd->loc, "ICE: class IR size does not match the frontend size"); + fatal(); + } + IF_LOG Logger::cout() << "class type: " << *t->type << std::endl; return t; diff --git a/ir/irtypestruct.cpp b/ir/irtypestruct.cpp index ccbb5b9514c..fba984498a1 100644 --- a/ir/irtypestruct.cpp +++ b/ir/irtypestruct.cpp @@ -11,6 +11,7 @@ #include "dmd/aggregate.h" #include "dmd/declaration.h" +#include "dmd/errors.h" #include "dmd/init.h" #include "dmd/mtype.h" #include "dmd/template.h" @@ -91,6 +92,11 @@ IrTypeStruct *IrTypeStruct::get(StructDeclaration *sd) { builder.addTailPadding(sd->structsize); isaStruct(t->type)->setBody(builder.defaultTypes(), builder.isPacked()); t->varGEPIndices = builder.varGEPIndices(); + + if (getTypeAllocSize(t->type) != sd->structsize) { + error(sd->loc, "ICE: struct IR size does not match the frontend size"); + fatal(); + } } IF_LOG Logger::cout() << "final struct type: " << *t->type << std::endl; diff --git a/tests/compilable/gh4734.d b/tests/compilable/gh4734.d new file mode 100644 index 00000000000..49656e86077 --- /dev/null +++ b/tests/compilable/gh4734.d @@ -0,0 +1,20 @@ +// RUN: %ldc -c %s + +align(1) struct Item { + KV v; + uint i; +} + +struct KV { + align(1) S* s; + uint k; +} + +struct S { + Table table; +} + +struct Table { + char a; + Item v; +} From 3305549796d2086cc2cf12528fb96617927c3b36 Mon Sep 17 00:00:00 2001 From: Martin Kinkelin Date: Sun, 18 Aug 2024 16:10:08 +0200 Subject: [PATCH 2/4] Get rid of cycles in DtoType() Fixes #4734. --- CHANGELOG.md | 1 + ir/irtype.cpp | 35 ++++++++--------------------------- ir/irtypeaggr.cpp | 17 +++++++++-------- 3 files changed, 18 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c0a3e8111ce..b05c664ce11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ #### Bug fixes - Fix potentially corrupt IR layouts for bit fields. (#4646, #4708) +- Fix potentially corrupt IR layouts for explicitly under-aligned aggregates, a regression introduced in LDC v1.31. (#4734, #4736) # LDC 1.39.0 (2024-07-04) diff --git a/ir/irtype.cpp b/ir/irtype.cpp index d00f35c5386..edf8aa65990 100644 --- a/ir/irtype.cpp +++ b/ir/irtype.cpp @@ -117,25 +117,12 @@ IrTypePointer *IrTypePointer::get(Type *dt) { auto &ctype = getIrType(dt); assert(!ctype); - LLType *elemType; - unsigned addressSpace = 0; - if (dt->ty == TY::Tnull) { - elemType = llvm::Type::getInt8Ty(getGlobalContext()); - } else { - elemType = DtoMemType(dt->nextOf()); - if (dt->nextOf()->ty == TY::Tfunction) { - addressSpace = gDataLayout->getProgramAddressSpace(); - } - - // DtoType could have already created the same type, e.g. for - // dt == Node* in struct Node { Node* n; }. - if (ctype) { - return ctype->isPointer(); - } - } + unsigned addressSpace = + dt->ty == TY::Tpointer && dt->nextOf()->ty == TY::Tfunction + ? gDataLayout->getProgramAddressSpace() + : 0; - auto t = - new IrTypePointer(dt, llvm::PointerType::get(elemType, addressSpace)); + auto t = new IrTypePointer(dt, getOpaquePtrType(addressSpace)); ctype = t; return t; } @@ -173,15 +160,9 @@ IrTypeArray *IrTypeArray::get(Type *dt) { auto &ctype = getIrType(dt); assert(!ctype); - LLType *elemType = DtoMemType(dt->nextOf()); - - // Could have already built the type as part of a struct forward reference, - // just as for pointers. - if (!ctype) { - llvm::Type *types[] = {DtoSize_t(), llvm::PointerType::get(elemType, 0)}; - LLType *at = llvm::StructType::get(getGlobalContext(), types, false); - ctype = new IrTypeArray(dt, at); - } + llvm::Type *types[] = {DtoSize_t(), getOpaquePtrType()}; + LLType *at = llvm::StructType::get(getGlobalContext(), types, false); + ctype = new IrTypeArray(dt, at); return ctype->isArray(); } diff --git a/ir/irtypeaggr.cpp b/ir/irtypeaggr.cpp index 500896b61a4..c4f4c690096 100644 --- a/ir/irtypeaggr.cpp +++ b/ir/irtypeaggr.cpp @@ -212,17 +212,18 @@ void AggrTypeBuilder::addAggregate( // add default type m_defaultTypes.push_back(llType); - unsigned fieldAlignment, fieldSize; if (!llType->isSized()) { - // forward reference in a cycle or similar, we need to trust the D type - fieldAlignment = DtoAlignment(vd->type); - fieldSize = af.size; - } else { - fieldAlignment = getABITypeAlign(llType); - fieldSize = getTypeAllocSize(llType); - assert(fieldSize <= af.size); + error(vd->loc, + "unexpected IR type forward declaration for aggregate member of " + "type `%s`. This is an ICE, please file an LDC issue.", + vd->type->toPrettyChars()); + fatal(); } + const unsigned fieldAlignment = getABITypeAlign(llType); + const unsigned fieldSize = getTypeAllocSize(llType); + assert(fieldSize <= af.size); + // advance offset to right past this field if (!m_packed) { assert(fieldAlignment); From b27a1e007672b5b089e30ca962fa496f36e188b1 Mon Sep 17 00:00:00 2001 From: Martin Kinkelin Date: Sun, 18 Aug 2024 17:53:56 +0200 Subject: [PATCH 3/4] Lazily define class *memory* IR type To break remaining cycles in DtoType() - a class *ref* is an opaque IR pointer nowadays. --- ir/irtypeaggr.cpp | 3 ++- ir/irtypeaggr.h | 4 +++- ir/irtypeclass.cpp | 48 +++++++++++++++++++++++++++++++--------------- ir/irtypeclass.h | 4 +++- 4 files changed, 41 insertions(+), 18 deletions(-) diff --git a/ir/irtypeaggr.cpp b/ir/irtypeaggr.cpp index c4f4c690096..a8ab7387697 100644 --- a/ir/irtypeaggr.cpp +++ b/ir/irtypeaggr.cpp @@ -279,10 +279,11 @@ IrTypeAggr::IrTypeAggr(AggregateDeclaration *ad) LLStructType::create(gIR->context(), ad->toPrettyChars())), aggr(ad) {} -unsigned IrTypeAggr::getMemberLocation(VarDeclaration *var, bool& isFieldIdx) const { +unsigned IrTypeAggr::getMemberLocation(VarDeclaration *var, bool& isFieldIdx) { // Note: The interface is a bit more general than what we actually return. // Specifically, the frontend offset information we use for overlapping // fields is always based at the object start. + const auto &varGEPIndices = getVarGEPIndices(); auto it = varGEPIndices.find(var); if (it != varGEPIndices.end()) { isFieldIdx = true; diff --git a/ir/irtypeaggr.h b/ir/irtypeaggr.h index 780aab80000..1907cb37214 100644 --- a/ir/irtypeaggr.h +++ b/ir/irtypeaggr.h @@ -72,7 +72,7 @@ class IrTypeAggr : public IrType { /// Returns the index of the field in the LLVM struct type that corresponds /// to the given member variable, plus the offset to the actual field start /// due to overlapping (union) fields, if any. - unsigned getMemberLocation(VarDeclaration *var, bool& isFieldIdx) const; + unsigned getMemberLocation(VarDeclaration *var, bool& isFieldIdx); protected: /// @@ -90,6 +90,8 @@ class IrTypeAggr : public IrType { /// the field index of a variable in the frontend, it only stores the byte /// offset. VarGEPIndices varGEPIndices; + + virtual const VarGEPIndices &getVarGEPIndices() { return varGEPIndices; } }; // A helper for aggregating consecutive bit fields to a group. diff --git a/ir/irtypeclass.cpp b/ir/irtypeclass.cpp index ad35dacdbc5..8e4a77d6fb8 100644 --- a/ir/irtypeclass.cpp +++ b/ir/irtypeclass.cpp @@ -61,24 +61,34 @@ void IrTypeClass::addClassData(AggrTypeBuilder &builder, } IrTypeClass *IrTypeClass::get(ClassDeclaration *cd) { + const auto t = new IrTypeClass(cd); + getIrType(cd->type) = t; + return t; +} + +llvm::Type *IrTypeClass::getLLType() { return getOpaquePtrType(); } + +// Lazily build the actual IR struct type when needed. +// Note that it is this function that initializes most fields! +llvm::Type *IrTypeClass::getMemoryLLType() { + if (!isaStruct(type)->isOpaque()) + return type; + IF_LOG Logger::println("Building class type %s @ %s", cd->toPrettyChars(), cd->loc.toChars()); LOG_SCOPE; - const auto t = new IrTypeClass(cd); - getIrType(cd->type) = t; - const unsigned instanceSize = cd->structsize; IF_LOG Logger::println("Instance size: %u", instanceSize); AggrTypeBuilder builder; // add vtbl - builder.addType(llvm::PointerType::get(t->vtbl_type, 0), target.ptrsize); + builder.addType(llvm::PointerType::get(vtbl_type, 0), target.ptrsize); if (cd->isInterfaceDeclaration()) { // interfaces are just a vtable - t->num_interface_vtbls = + num_interface_vtbls = cd->vtblInterfaces ? cd->vtblInterfaces->length : 0; } else { // classes have monitor and fields @@ -88,7 +98,7 @@ IrTypeClass *IrTypeClass::get(ClassDeclaration *cd) { } // add data members recursively - t->addClassData(builder, cd); + addClassData(builder, cd); // add tail padding if (instanceSize) // can be 0 for opaque classes @@ -96,25 +106,23 @@ IrTypeClass *IrTypeClass::get(ClassDeclaration *cd) { } // set struct body and copy GEP indices - isaStruct(t->type)->setBody(builder.defaultTypes(), builder.isPacked()); - t->varGEPIndices = builder.varGEPIndices(); + isaStruct(type)->setBody(builder.defaultTypes(), builder.isPacked()); + varGEPIndices = builder.varGEPIndices(); if (!cd->isInterfaceDeclaration() && instanceSize && - getTypeAllocSize(t->type) != instanceSize) { + getTypeAllocSize(type) != instanceSize) { error(cd->loc, "ICE: class IR size does not match the frontend size"); fatal(); } - IF_LOG Logger::cout() << "class type: " << *t->type << std::endl; + IF_LOG Logger::cout() << "class type: " << *type << std::endl; - return t; + return type; } -llvm::Type *IrTypeClass::getLLType() { return llvm::PointerType::get(type, 0); } - -llvm::Type *IrTypeClass::getMemoryLLType() { return type; } - size_t IrTypeClass::getInterfaceIndex(ClassDeclaration *inter) { + getMemoryLLType(); // lazily resolve + auto it = interfaceMap.find(inter); if (it == interfaceMap.end()) { return ~0UL; @@ -122,6 +130,16 @@ size_t IrTypeClass::getInterfaceIndex(ClassDeclaration *inter) { return it->second; } +unsigned IrTypeClass::getNumInterfaceVtbls() { + getMemoryLLType(); // lazily resolve + return num_interface_vtbls; +} + +const VarGEPIndices &IrTypeClass::getVarGEPIndices() { + getMemoryLLType(); // lazily resolve + return varGEPIndices; +} + void IrTypeClass::addInterfaceToMap(ClassDeclaration *inter, size_t index) { // don't duplicate work or overwrite indices if (interfaceMap.find(inter) != interfaceMap.end()) { diff --git a/ir/irtypeclass.h b/ir/irtypeclass.h index 95c9e22217c..7ad19859d92 100644 --- a/ir/irtypeclass.h +++ b/ir/irtypeclass.h @@ -47,7 +47,7 @@ class IrTypeClass : public IrTypeAggr { /// Returns the number of interface implementations (vtables) in this /// class. - unsigned getNumInterfaceVtbls() { return num_interface_vtbls; } + unsigned getNumInterfaceVtbls(); protected: /// @@ -73,6 +73,8 @@ class IrTypeClass : public IrTypeAggr { ////////////////////////////////////////////////////////////////////////// + const VarGEPIndices &getVarGEPIndices() override; + /// Adds the data members for the given class to the type builder, including /// those inherited from base classes/interfaces. void addClassData(AggrTypeBuilder &builder, ClassDeclaration *currCd); From 2c04d8648f04e43383d0bcd1609bd5591eaa4ea5 Mon Sep 17 00:00:00 2001 From: Martin Kinkelin Date: Sun, 18 Aug 2024 18:08:33 +0200 Subject: [PATCH 4/4] GHA: Adopt new Xcode 16 beta --- .github/workflows/main.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index f11f2ba1c22..9739b9c58be 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -94,9 +94,9 @@ jobs: with: submodules: true fetch-depth: 50 - - name: 'macOS 14: Switch to Xcode 16 Beta 4' + - name: 'macOS 14: Switch to Xcode 16 Beta 5' if: matrix.os == 'macos-14' - run: sudo xcode-select -switch /Applications/Xcode_16_beta_4.app + run: sudo xcode-select -switch /Applications/Xcode_16_beta_5.app - name: Install prerequisites uses: ./.github/actions/1-setup with: