-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Don't set GCState to 1 in PInvoke Epilog #38303
Conversation
The comment indicated this was necessary to keep the FrameListRoot live, but the code generated by `CreateFrameLinkUpdate` will do that if needed. Fix dotnet#38192
@dotnet/dnceng - the |
I shall investigate. |
@CarolEidt it's a timeout. It'd be nice if this were more obvious for sure but it'd be unfortunately a breaking API change so will have to wait. To see this:
|
Thanks @MattGal - is the timeout common? I've already retried once, and have retried again. I just ran the tests on my system and they completed in what seems like about the normal amount of time. |
@dotnet/jit-contrib PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few comments on comments.
src/coreclr/src/jit/liveness.cpp
Outdated
LclVarDsc* varDsc = &lvaTable[info.compLvFrameListRoot]; | ||
// 32-bit targets always pop the frame in the epilog. | ||
// For 64-bit targets, we only do this in the epilog for IL stubs; | ||
// for non-IL stubs the frame is popped after every PInvoke call. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the CLANG_FORMAT_COMMENT_ANCHOR
here?
// and we have an unmanaged p/invoke call in the method, | ||
// then we're going to run the p/invoke epilog. | ||
// So we mark the FrameRoot as used by this instruction. | ||
// This ensures that the block->bbVarUse will contain | ||
// the FrameRoot local var if is it a tracked variable. | ||
|
||
if ((tree->AsCall()->IsUnmanaged() || tree->AsCall()->IsTailCall()) && compMethodRequiresPInvokeFrame()) | ||
if ((tree->AsCall()->IsUnmanaged() || tree->AsCall()->IsTailCallViaJitHelper()) && | ||
compMethodRequiresPInvokeFrame()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just below this is another comment mentioning "the TCB". Since you redid other comments, maybe redo this one too?
Quick check of Kusto shows a work item with that naming timing out on Windows.10.Amd64.Open having happened 257 of 9366 total run times in the past 30 days. Do note that while things may work on your box, there are variances; these machines have 2 cores, they run server OS skus, and it's also unlikely you ran the test locally 9300 times. |
Remove the incorrect setting of
GCState
and fix the liveness computation forcompLvFrameListRoot
for tailcalls.Fix #38192