From 3787478f2eef98981c7cbdc84b78580a2058304e Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 23 Jan 2025 02:48:18 +0100 Subject: [PATCH 1/3] Revert potential UB due to aliasing + more WB removals --- src/coreclr/jit/assertionprop.cpp | 24 ---------------------- src/coreclr/jit/importer.cpp | 15 ++++++++++++++ src/coreclr/jit/promotiondecomposition.cpp | 7 ++++--- 3 files changed, 19 insertions(+), 27 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 288891b0daab44..12f0ed85e03c66 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -5258,30 +5258,6 @@ static GCInfo::WriteBarrierForm GetWriteBarrierForm(Compiler* comp, ValueNum vn) return GetWriteBarrierForm(comp, funcApp.m_args[0]); } } - if (funcApp.m_func == VNF_InitVal) - { - unsigned lclNum = vnStore->CoercedConstantValue(funcApp.m_args[0]); - assert(lclNum != BAD_VAR_NUM); - CORINFO_CLASS_HANDLE srcCls = NO_CLASS_HANDLE; - - if (comp->compMethodHasRetVal() && (lclNum == comp->info.compRetBuffArg)) - { - // See if the address is in current method's return buffer - // while the return type is a byref-like type. - srcCls = comp->info.compMethodInfo->args.retTypeClass; - } - else if (lclNum == comp->info.compThisArg) - { - // Same for implicit "this" parameter - assert(!comp->info.compIsStatic); - srcCls = comp->info.compClassHnd; - } - - if ((srcCls != NO_CLASS_HANDLE) && comp->eeIsByrefLike(srcCls)) - { - return GCInfo::WriteBarrierForm::WBF_NoBarrier; - } - } } return GCInfo::WriteBarrierForm::WBF_BarrierUnknown; } diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 5bd6e842daa119..a92fdb5c0cb52e 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -9548,6 +9548,21 @@ void Compiler::impImportBlockCode(BasicBlock* block) { indirFlags |= GTF_IND_TGT_HEAP; } + else if ((lclTyp == TYP_STRUCT) && (fieldInfo.structType != NO_CLASS_HANDLE) && + eeIsByrefLike(fieldInfo.structType)) + { + // Field's type is a byref-like struct -> address is not on the heap. + indirFlags |= GTF_IND_TGT_NOT_HEAP; + } + else + { + // Field's owner is a byref-like struct -> address is not on the heap. + CORINFO_CLASS_HANDLE fldOwner = info.compCompHnd->getFieldClass(resolvedToken.hField); + if ((fldOwner != NO_CLASS_HANDLE) && eeIsByrefLike(fldOwner)) + { + indirFlags |= GTF_IND_TGT_NOT_HEAP; + } + } assert(varTypeIsI(op1)); op1 = (lclTyp == TYP_STRUCT) ? gtNewStoreBlkNode(layout, op1, op2, indirFlags)->AsIndir() diff --git a/src/coreclr/jit/promotiondecomposition.cpp b/src/coreclr/jit/promotiondecomposition.cpp index cde869becbe2b5..064e4d8fd2f0ab 100644 --- a/src/coreclr/jit/promotiondecomposition.cpp +++ b/src/coreclr/jit/promotiondecomposition.cpp @@ -523,16 +523,17 @@ class DecompositionPlan target_ssize_t addrBaseOffs = 0; FieldSeq* addrBaseOffsFldSeq = nullptr; GenTreeFlags indirFlags = GTF_EMPTY; - + GenTreeFlags flagsToPropagate = GTF_IND_COPYABLE_FLAGS; if (m_store->OperIs(GT_STORE_BLK)) { + flagsToPropagate |= GTF_IND_TGT_NOT_HEAP | GTF_IND_TGT_HEAP; addr = m_store->AsIndir()->Addr(); - indirFlags = m_store->gtFlags & GTF_IND_COPYABLE_FLAGS; + indirFlags = m_store->gtFlags & flagsToPropagate; } else if (m_src->OperIs(GT_BLK)) { addr = m_src->AsIndir()->Addr(); - indirFlags = m_src->gtFlags & GTF_IND_COPYABLE_FLAGS; + indirFlags = m_src->gtFlags & flagsToPropagate; } int numAddrUses = 0; From 43cead88dfe31bf95807be85f03255313afd80e2 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 23 Jan 2025 13:31:26 +0100 Subject: [PATCH 2/3] Address feedback --- src/coreclr/jit/morphblock.cpp | 4 ++++ src/coreclr/jit/promotiondecomposition.cpp | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/src/coreclr/jit/morphblock.cpp b/src/coreclr/jit/morphblock.cpp index 4b81ca7d79eaa3..c5ab836fdc2281 100644 --- a/src/coreclr/jit/morphblock.cpp +++ b/src/coreclr/jit/morphblock.cpp @@ -1363,6 +1363,10 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField() if (m_store->OperIs(GT_STORE_BLK, GT_STOREIND)) { indirFlags = m_store->gtFlags & (GTF_IND_TGT_NOT_HEAP | GTF_IND_TGT_HEAP); + if (m_store->OperIs(GT_STORE_BLK) && m_store->AsBlk()->GetLayout()->IsStackOnly(m_comp)) + { + indirFlags |= GTF_IND_TGT_NOT_HEAP; + } } dstFldStore = m_comp->gtNewStoreIndNode(srcType, fldAddr, srcFld, indirFlags); } diff --git a/src/coreclr/jit/promotiondecomposition.cpp b/src/coreclr/jit/promotiondecomposition.cpp index 064e4d8fd2f0ab..b7538acddbf2f3 100644 --- a/src/coreclr/jit/promotiondecomposition.cpp +++ b/src/coreclr/jit/promotiondecomposition.cpp @@ -529,6 +529,10 @@ class DecompositionPlan flagsToPropagate |= GTF_IND_TGT_NOT_HEAP | GTF_IND_TGT_HEAP; addr = m_store->AsIndir()->Addr(); indirFlags = m_store->gtFlags & flagsToPropagate; + if (m_store->AsBlk()->GetLayout()->IsStackOnly(m_compiler)) + { + indirFlags |= GTF_IND_TGT_NOT_HEAP; + } } else if (m_src->OperIs(GT_BLK)) { From 05059511a275e9760da8549c7aa1847e6b40fa70 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 23 Jan 2025 13:54:50 +0100 Subject: [PATCH 3/3] fix regressions --- src/coreclr/jit/morph.cpp | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index a7071e4ec4de9c..31785e2e052e49 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -7755,14 +7755,23 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA break; case GT_STOREIND: - if (op1->OperIs(GT_FIELD_ADDR) && varTypeIsGC(tree)) + if (varTypeIsGC(tree)) { - CORINFO_FIELD_HANDLE fieldHandle = op1->AsFieldAddr()->gtFldHnd; - if (eeIsByrefLike(info.compCompHnd->getFieldClass(fieldHandle))) + GenTree* addr = op1; + // If we're storing a reference to a field (GT_FIELD_ADDR), let's check if the field's owner is a + // byref-like struct. + while ((addr != nullptr) && addr->OperIs(GT_FIELD_ADDR)) { - JITDUMP("Marking [%06u] STOREIND as GTF_IND_TGT_NOT_HEAP: field's owner is a byref-like struct\n", + CORINFO_FIELD_HANDLE fieldHandle = addr->AsFieldAddr()->gtFldHnd; + if (eeIsByrefLike(info.compCompHnd->getFieldClass(fieldHandle))) + { + JITDUMP( + "Marking [%06u] STOREIND as GTF_IND_TGT_NOT_HEAP: field's owner is a byref-like struct\n", dspTreeID(tree)); - tree->gtFlags |= GTF_IND_TGT_NOT_HEAP; + tree->gtFlags |= GTF_IND_TGT_NOT_HEAP; + break; + } + addr = addr->AsFieldAddr()->GetFldObj(); } } break;