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

Jump to bottom

Don't set GCState to 1 in PInvoke Epilog #38303

Merged
merged 3 commits into from
Jun 25, 2020
Merged

Don't set GCState to 1 in PInvoke Epilog #38303

merged 3 commits into from
Jun 25, 2020

Conversation

CarolEidt
Copy link
Contributor

@CarolEidt CarolEidt commented Jun 23, 2020 •
edited
Loading

Remove the incorrect setting of GCState and fix the liveness computation for compLvFrameListRoot for tailcalls.

Fix #38192

@CarolEidt
Don't set GCState to 1 in PInvoke Epilog
8c33791 
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-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 23, 2020
@CarolEidt
Copy link
Contributor Author

@dotnet/dnceng - the CoreCLR Pri0 Runtime Tests Run Windows_NT x86 checked leg doesn't seem to complete. AFAICT there aren't any errors here.

@MattGal
Copy link
Member

MattGal commented Jun 24, 2020

@dotnet/dnceng - the CoreCLR Pri0 Runtime Tests Run Windows_NT x86 checked leg doesn't seem to complete. AFAICT there aren't any errors here.

I shall investigate.

@MattGal
Copy link
Member

MattGal commented Jun 24, 2020

@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:

  1. remove the /console to get the other logs: https://helix.dot.net/api/2019-06-17/jobs/d761b210-1a1d-4689-961b-f1fe79484357/workitems/PayloadGroup0

  2. The -3 exit code is a giveaway for timeouts but can also check the run_client log: https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-38303-merge-d761b2101a1d468996/PayloadGroup0/9e2629ef-b8b2-4720-9a04-88c386f6c7bb.log

2020-06-24T21:33:03.483Z	INFO   	job(51)	kill	Begin killing timed-out process(es)
2020-06-24T21:33:03.483Z	ERROR  	job(60)	kill	Job running for too long. Killing...
2020-06-24T21:33:03.483Z	INFO   	job(66)	kill	Finished killing timed-out process(es)
2020-06-24T21:33:03.483Z	INFO   	job(37)	wait	Work item's process exited normally.
2020-06-24T21:33:03.483Z	ERROR  	executor(652)	_execute_command	Executor timed out after 1800 seconds and was killed.
2020-06-24T21:33:03.483Z	INFO   	event(42)	send	Sending event type WorkItemTimeout
2020-06-24T21:33:03.624Z	INFO   	saferequests(87)	request_with_retry	Response complete with status code '201'
2020-06-24T21:33:03.624Z	INFO   	executor(618)	_dump_file_upload	No dump files to upload
2020-06-24T21:33:03.624Z	INFO   	executor(674)	_execute_command	Finished _execute_command, exit code: -3

@CarolEidt
Copy link
Contributor Author

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.

@CarolEidt
Copy link
Contributor Author

@dotnet/jit-contrib PTAL

AndyAyersMS
AndyAyersMS approved these changes Jun 25, 2020
View reviewed changes
Copy link
Member

@AndyAyersMS AndyAyersMS left a 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.
Copy link
Member

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?

src/coreclr/src/jit/liveness.cpp
// 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())
Copy link
Member

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?

@CarolEidt CarolEidt merged commit afc9f65 into dotnet:master Jun 25, 2020
@CarolEidt CarolEidt deleted the Fix38192 branch June 25, 2020 18:46
@MattGal
Copy link
Member

MattGal commented Jun 29, 2020

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.

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.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AndyAyersMS AndyAyersMS AndyAyersMS approved these changes

Assignees
No one assigned
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

UnmanagedCallersOnly does not work when called from a P/Invoke in Release mode
4 participants
@CarolEidt @MattGal @AndyAyersMS @Dotnet-GitSync-Bot