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

Changes from 4 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
5 changes: 5 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,11 @@ GenTree* Lowering::LowerDelegateInvoke(GenTreeCall* call)
BlockRange().Remove(thisArgNode);
BlockRange().InsertBefore(call, newThisAddr, newThis, thisArgNode);

// Keep delegate alive
GenTree* baseClone = comp->gtCloneExpr(base);
GenTree* keepBaseAlive = comp->gtNewKeepAliveNode(baseClone);
BlockRange().InsertAfter(call, baseClone, keepBaseAlive);
Copy link
Member

@jkotas jkotas Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is this fix going to introduce CQ regressions? Ideally, the fix for this issue would be zero codegen diff on x86/x64.

We need to keep delegate alive only until we make the call. The delegate does not need to be alive after call.

Copy link
Member

Choose a reason for hiding this comment

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

For my knowledge, will all the various stubs that we may go through on the way to the final target automatically keep the right loader allocator without the JIT at minimum putting the delegate instance in a callee-saved register?

Copy link
Member

Choose a reason for hiding this comment

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

I think so. There are really only 3:

  • Shuffle thunks: hand-emitted assembly so the GC cannot kick in while they are executing
  • Multicast stubs: they keep the delegate thisptr alive. (Adding comment that this is important may be nice.)
  • Marshalled function pointers: The delegate points to itself, so calling the method on the delegate will keep it alive.

It is possible that I may be missing a corner case somewhere, but I do not expect it to be hard to fix.


ContainCheckIndir(newThis->AsIndir());

// the control target is
Expand Down
Loading