Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT: Keep delegate object alive during invoke #105099

Closed
wants to merge 34 commits into from
Closed

JIT: Keep delegate object alive during invoke #105099

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
4a90d51
Disable Comparer_get_Default test on win-arm64-crossgen
EgorBo Jul 18, 2024
76c6f4d
Keep 'this' alive for delegate invoke
EgorBo Jul 18, 2024
49b10a6
Update issues.targets
EgorBo Jul 18, 2024
8bf8365
Update lower.cpp
EgorBo Jul 18, 2024
96543b3
less conservative version
EgorBo Jul 18, 2024
5db07f8
fix define
EgorBo Jul 18, 2024
118c8ad
Apply Jakob's suggestion
EgorBo Jul 18, 2024
ffe5317
GT_STOP_NOGC
EgorBo Jul 19, 2024
5558203
Merge branch 'main' into keep-this-alive-fptr
EgorBo Jul 19, 2024
bfc872f
Update lower.cpp
EgorBo Jul 19, 2024
300ab49
Address feedback
EgorBo Jul 19, 2024
26da6ab
Address feedback
EgorBo Jul 19, 2024
f95c267
Merge branch 'main' of https://github.com/dotnet/runtime into keep-th…
EgorBo Aug 1, 2024
b35c892
handle tail calls
EgorBo Aug 1, 2024
255c5bd
Merge branch 'main' into keep-this-alive-fptr
EgorBo Aug 1, 2024
ffdc8e5
remove bogus assert
EgorBo Aug 1, 2024
9e34a9a
Merge branch 'keep-this-alive-fptr' of github.com:EgorBo/runtime-1 in…
EgorBo Aug 1, 2024
1dac215
Update lower.cpp
EgorBo Aug 1, 2024
7cb1d65
Update lower.cpp
EgorBo Aug 1, 2024
f0964fd
Update lower.cpp
EgorBo Aug 1, 2024
126fca9
Update lower.cpp
EgorBo Aug 2, 2024
c28b235
Merge branch 'main' of github.com:dotnet/runtime into keep-this-alive…
EgorBo Oct 10, 2024
65d7909
test
EgorBo Oct 10, 2024
dded5c9
test2
EgorBo Oct 10, 2024
dca303b
test
EgorBo Oct 10, 2024
95b6301
Clean up
EgorBo Oct 10, 2024
2377322
Update lower.cpp
EgorBo Oct 10, 2024
bef38f3
clean up
EgorBo Oct 10, 2024
9fba5a3
Update lower.cpp
EgorBo Oct 10, 2024
9ccc937
Update lower.cpp
EgorBo Oct 11, 2024
2ab1e61
Merge branch 'main' of github.com:dotnet/runtime into keep-this-alive…
EgorBo Oct 11, 2024
ef49ee6
test
EgorBo Oct 11, 2024
f4061ea
Merge branch 'main' into keep-this-alive-fptr
EgorBo Nov 4, 2024
a5c4b81
Merge branch 'main' into keep-this-alive-fptr
EgorBo Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/coreclr/jit/codegenarmarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,10 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode)
GetEmitter()->emitDisableGC();
break;

case GT_STOP_NONGC:
GetEmitter()->emitEnableGC();
break;

case GT_START_PREEMPTGC:
// Kill callee saves GC registers, and create a label
// so that information gets propagated to the emitter.
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/jit/codegenloongarch64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4206,6 +4206,10 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode)
GetEmitter()->emitDisableGC();
break;

case GT_STOP_NONGC:
GetEmitter()->emitEnableGC();
break;

case GT_START_PREEMPTGC:
// Kill callee saves GC registers, and create a label
// so that information gets propagated to the emitter.
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/jit/codegenriscv64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4285,6 +4285,10 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode)
GetEmitter()->emitDisableGC();
break;

case GT_STOP_NONGC:
GetEmitter()->emitEnableGC();
break;

case GT_START_PREEMPTGC:
// Kill callee saves GC registers, and create a label
// so that information gets propagated to the emitter.
Expand Down
4 changes: 4 additions & 0 deletions src/coreclr/jit/codegenxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1873,6 +1873,10 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode)
case GT_START_NONGC:
GetEmitter()->emitDisableGC();
break;

case GT_STOP_NONGC:
GetEmitter()->emitEnableGC();
break;
#endif // !defined(JIT32_GCENCODER)

case GT_START_PREEMPTGC:
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11708,6 +11708,7 @@ class GenTreeVisitor
case GT_SETCC:
case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
case GT_START_PREEMPTGC:
case GT_PROF_HOOK:
#if defined(FEATURE_EH_WINDOWS_X86)
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4365,6 +4365,7 @@ void GenTree::VisitOperands(TVisitor visitor)
case GT_SETCC:
case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
case GT_START_PREEMPTGC:
case GT_PROF_HOOK:
#if defined(FEATURE_EH_WINDOWS_X86)
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6475,6 +6475,7 @@ bool GenTree::TryGetUse(GenTree* operand, GenTree*** pUse)
case GT_SETCC:
case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
case GT_START_PREEMPTGC:
case GT_PROF_HOOK:
#if defined(FEATURE_EH_WINDOWS_X86)
Expand Down Expand Up @@ -10011,6 +10012,7 @@ GenTreeUseEdgeIterator::GenTreeUseEdgeIterator(GenTree* node)
case GT_SETCC:
case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
case GT_START_PREEMPTGC:
case GT_PROF_HOOK:
#if defined(FEATURE_EH_WINDOWS_X86)
Expand Down Expand Up @@ -12158,6 +12160,7 @@ void Compiler::gtDispLeaf(GenTree* tree, IndentStack* indentStack)
case GT_NOP:
case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
case GT_START_PREEMPTGC:
case GT_PROF_HOOK:
case GT_CATCH_ARG:
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/gtlist.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ GTNODE(SWITCH , GenTreeOp ,0,1,GTK_UNOP|GTK_NOVALUE)
GTNODE(NO_OP , GenTree ,0,0,GTK_LEAF|GTK_NOVALUE) // A NOP that cannot be deleted.

GTNODE(START_NONGC , GenTree ,0,0,GTK_LEAF|GTK_NOVALUE|DBK_NOTHIR) // Starts a new instruction group that will be non-gc interruptible.
GTNODE(STOP_NONGC , GenTree ,0,0,GTK_LEAF|GTK_NOVALUE|DBK_NOTHIR) // Tries to end the non-gc interruptible instruction group
GTNODE(START_PREEMPTGC , GenTree ,0,0,GTK_LEAF|GTK_NOVALUE|DBK_NOTHIR) // Starts a new instruction group where preemptive GC is enabled.
GTNODE(PROF_HOOK , GenTree ,0,0,GTK_LEAF|GTK_NOVALUE|DBK_NOTHIR) // Profiler Enter/Leave/TailCall hook.

Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/liveness.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1438,6 +1438,7 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR
case GT_SWITCH:
case GT_RETFILT:
case GT_START_NONGC:
case GT_STOP_NONGC:
case GT_START_PREEMPTGC:
case GT_PROF_HOOK:
#if defined(FEATURE_EH_WINDOWS_X86)
Expand Down
13 changes: 13 additions & 0 deletions src/coreclr/jit/lower.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5551,6 +5551,19 @@ GenTree* Lowering::LowerDelegateInvoke(GenTreeCall* call)
BlockRange().Remove(thisArgNode);
BlockRange().InsertBefore(call, newThisAddr, newThis, thisArgNode);

#if !defined(TARGET_XARCH)
if (comp->GetInterruptible())
{
// If the target's backend doesn't support indirect calls with immediate operands (contained)
// and the method is marked as interruptible, we need to insert a GT_START_NONGC before the call.
// to keep the delegate object alive while we're obtaining the function pointer.
GenTree* startNonGCNode = new (comp, GT_START_NONGC) GenTree(GT_START_NONGC, TYP_VOID);
GenTree* stopNonGCNode = new (comp, GT_STOP_NONGC) GenTree(GT_STOP_NONGC, TYP_VOID);
BlockRange().InsertAfter(thisArgNode, startNonGCNode);
BlockRange().InsertAfter(call, stopNonGCNode);
}
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still do this inside LowerCall since it's LowerCall responsibility to insert the control expression returned by this function. This is adding an assumption about where LowerCall will insert that node.

Copy link
Member Author

@EgorBo EgorBo Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still do this inside LowerCall since it's LowerCall responsibility to insert the control expression returned by this function. This is adding an assumption about where LowerCall will insert that node.

but that's the point, no? LowerDelegateInvoke already inserts some trees and at the end of those we conservatively emit a START_NOGC so other functions like LowerCall itself, LowerCFGCall or LowerTailCallViaJitHelper can insert whatever they want after our NOGC and before the call. Because, presumably, they can extend the dangerous area where GC can kick in and collect our delegate

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah, that's a good point too. What I dislike here is just that it still relies on LowerCall to insert the control expression after the START_NONGC. To be conservatively correct we have to insert the START_NOGC before the last use of the delegate, since emitDisableGC() means something like "the next instruction I emit is going to make GC information wrong, so disable GC after that instruction". Hence inserting START_NONGC after thisArgNode is only correct because there ends up being another use inserted after that point by LowerCall. The main idea behind moving the logic into LowerCall is that it has the full overview of what nodes it is going to insert and where, so it can make the decision with more information. So for instance it could insert it before the gtControlExpr in the delegate case, knowing that gtControlExpr has the last use of the delegate instance in the common case.


ContainCheckIndir(newThis->AsIndir());

// the control target is
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/lsraarm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,7 @@ int LinearScan::BuildNode(GenTree* tree)

case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
case GT_PROF_HOOK:
srcCount = 0;
assert(dstCount == 0);
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/lsraarm64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,7 @@ int LinearScan::BuildNode(GenTree* tree)

case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
srcCount = 0;
assert(dstCount == 0);
break;
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/lsraloongarch64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ int LinearScan::BuildNode(GenTree* tree)

case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
srcCount = 0;
assert(dstCount == 0);
break;
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/lsrariscv64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ int LinearScan::BuildNode(GenTree* tree)

case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
srcCount = 0;
assert(dstCount == 0);
break;
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/lsraxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ int LinearScan::BuildNode(GenTree* tree)

case GT_NO_OP:
case GT_START_NONGC:
case GT_STOP_NONGC:
srcCount = 0;
assert(dstCount == 0);
break;
Expand Down
Loading