From 25df37a1755ae0b900814ff1e03290538b53490b Mon Sep 17 00:00:00 2001 From: egorbot Date: Wed, 25 Feb 2026 00:36:14 +0100 Subject: [PATCH 1/4] don't hoist potentially invalid byrefs --- src/coreclr/jit/compiler.h | 2 ++ src/coreclr/jit/gentree.cpp | 61 ++++++++++++++++++++++++++++++++ src/coreclr/jit/ifconversion.cpp | 7 ++++ 3 files changed, 70 insertions(+) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 89e5f28d1bd447..c379281b8967de 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -3668,6 +3668,8 @@ class Compiler // Returns "true" iff "tree" or its (transitive) children have any of the side effects in "flags". bool gtTreeHasSideEffects(GenTree* tree, GenTreeFlags flags, bool ignoreCctors = false); + static bool gtTreeMayHaveInvalidByrefs(Compiler* compiler, GenTree* tree); + void gtExtractSideEffList(GenTree* expr, GenTree** pList, GenTreeFlags GenTreeFlags = GTF_SIDE_EFFECT, diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index d44e0bf76a1a91..dfc66a402c02e8 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -17160,6 +17160,67 @@ bool Compiler::gtNodeHasSideEffects(GenTree* tree, GenTreeFlags flags, bool igno return false; } +//----------------------------------------------------------- +// gtTreeMayHaveInvalidByrefs: Returns true if the given tree may contain potentially invalid byrefs. +// Technically, we should place a special side-effect flag on all GC-typed nodes until +// they are proven to be unconditionally valid or not pointing to the heap. We don't +// do that today, as it would likely cause a significant size regression. +// Consequently, any code that speculatively hoists nodes with byrefs must call this +// method to verify the tree and avoid hoisting if potentially invalid byrefs are present. +// +// Arguments: +// compiler - The compiler instance to use for the walk. +// tree - The tree to check. +// +// Return Value: +// True if the tree may contain potentially invalid byrefs; false otherwise. +// +bool Compiler::gtTreeMayHaveInvalidByrefs(Compiler* compiler, GenTree* tree) +{ + class ByrefSideEffectVisitor final : public GenTreeVisitor + { + public: + ByrefSideEffectVisitor(Compiler* compiler) + : GenTreeVisitor(compiler) + { + } + + enum + { + DoPreOrder = true, + }; + + fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) + { + GenTree* tree = *use; + // It is possible to be more precise, but it is not required by the contract. + // Conservatively assume that some TYP_REF could be invalid too, e.g. IND nodes. + if (varTypeIsGC(tree) && !tree->OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR, GT_RETURN)) + { + if (tree->OperIs(GT_CNS_INT)) + { + if (tree->IsIntegralConst(0)) + { + // Null is fine. + return WALK_CONTINUE; + } + + if (tree->TypeIs(TYP_REF) && tree->IsIconHandle(GTF_ICON_OBJ_HDL)) + { + // We assume frozen object handles are always valid in any context. + return WALK_CONTINUE; + } + } + return WALK_ABORT; + } + return WALK_CONTINUE; + } + }; + + ByrefSideEffectVisitor visitor(compiler); + return visitor.WalkTree(&tree, nullptr) == WALK_ABORT; +} + /***************************************************************************** * Returns true if the expr tree has any side effects. */ diff --git a/src/coreclr/jit/ifconversion.cpp b/src/coreclr/jit/ifconversion.cpp index 0acda646e0d9e6..4f3bd575eb2194 100644 --- a/src/coreclr/jit/ifconversion.cpp +++ b/src/coreclr/jit/ifconversion.cpp @@ -690,6 +690,13 @@ bool OptIfConversionDsc::optIfConvert(int* pReachabilityBudget) } } + if (m_compiler->gtTreeMayHaveInvalidByrefs(m_compiler, m_thenOperation.node) || + (m_doElseConversion && m_compiler->gtTreeMayHaveInvalidByrefs(m_compiler, m_elseOperation.node))) + { + JITDUMP("Skipping if-conversion that may have invalid byrefs in the then or else block\n"); + return false; + } + // Get the select node inputs. var_types selectType; GenTree* selectTrueInput; From dd7799a24a655661c6381fcd154b93143c01a1a4 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Wed, 25 Feb 2026 03:58:21 +0100 Subject: [PATCH 2/4] Update gentree.cpp --- src/coreclr/jit/gentree.cpp | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index dfc66a402c02e8..f7e081d6b93fcb 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -17162,11 +17162,13 @@ bool Compiler::gtNodeHasSideEffects(GenTree* tree, GenTreeFlags flags, bool igno //----------------------------------------------------------- // gtTreeMayHaveInvalidByrefs: Returns true if the given tree may contain potentially invalid byrefs. -// Technically, we should place a special side-effect flag on all GC-typed nodes until +// Technically, we should place a special side-effect flag on all TYP_BYREF-typed nodes until // they are proven to be unconditionally valid or not pointing to the heap. We don't // do that today, as it would likely cause a significant size regression. // Consequently, any code that speculatively hoists nodes with byrefs must call this // method to verify the tree and avoid hoisting if potentially invalid byrefs are present. +// We also assume that all TYP_REF nodes are always valid. All attempts to have some kinds +// of tagged unions of unmanaged + managed fields are UB. // // Arguments: // compiler - The compiler instance to use for the walk. @@ -17194,23 +17196,8 @@ bool Compiler::gtTreeMayHaveInvalidByrefs(Compiler* compiler, GenTree* tree) { GenTree* tree = *use; // It is possible to be more precise, but it is not required by the contract. - // Conservatively assume that some TYP_REF could be invalid too, e.g. IND nodes. - if (varTypeIsGC(tree) && !tree->OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR, GT_RETURN)) + if (tree->TypeIs(TYP_BYREF) && !tree->OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR, GT_RETURN)) { - if (tree->OperIs(GT_CNS_INT)) - { - if (tree->IsIntegralConst(0)) - { - // Null is fine. - return WALK_CONTINUE; - } - - if (tree->TypeIs(TYP_REF) && tree->IsIconHandle(GTF_ICON_OBJ_HDL)) - { - // We assume frozen object handles are always valid in any context. - return WALK_CONTINUE; - } - } return WALK_ABORT; } return WALK_CONTINUE; From 81b10ba418481065291b0fd158549f47319432fd Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Wed, 25 Feb 2026 03:58:38 +0100 Subject: [PATCH 3/4] Update src/coreclr/jit/ifconversion.cpp Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/coreclr/jit/ifconversion.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/ifconversion.cpp b/src/coreclr/jit/ifconversion.cpp index 4f3bd575eb2194..3e271a18c630da 100644 --- a/src/coreclr/jit/ifconversion.cpp +++ b/src/coreclr/jit/ifconversion.cpp @@ -690,8 +690,8 @@ bool OptIfConversionDsc::optIfConvert(int* pReachabilityBudget) } } - if (m_compiler->gtTreeMayHaveInvalidByrefs(m_compiler, m_thenOperation.node) || - (m_doElseConversion && m_compiler->gtTreeMayHaveInvalidByrefs(m_compiler, m_elseOperation.node))) + if (Compiler::gtTreeMayHaveInvalidByrefs(m_compiler, m_thenOperation.node) || + (m_doElseConversion && Compiler::gtTreeMayHaveInvalidByrefs(m_compiler, m_elseOperation.node))) { JITDUMP("Skipping if-conversion that may have invalid byrefs in the then or else block\n"); return false; From 4686782d784068c17e6ac665a2fa546c0c2fd479 Mon Sep 17 00:00:00 2001 From: egorbot Date: Wed, 25 Feb 2026 14:25:53 +0100 Subject: [PATCH 4/4] ignore nulls --- src/coreclr/jit/gentree.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index f7e081d6b93fcb..b026681f38cb95 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -17196,7 +17196,8 @@ bool Compiler::gtTreeMayHaveInvalidByrefs(Compiler* compiler, GenTree* tree) { GenTree* tree = *use; // It is possible to be more precise, but it is not required by the contract. - if (tree->TypeIs(TYP_BYREF) && !tree->OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR, GT_RETURN)) + if (tree->TypeIs(TYP_BYREF) && !tree->OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR, GT_RETURN) && + !tree->IsIntegralConst(0)) { return WALK_ABORT; }