Skip to content

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

Merged
CarolEidt merged 3 commits intodotnet:masterfrom
CarolEidt:Fix38192
Jun 25, 2020
Merged

Don't set GCState to 1 in PInvoke Epilog#38303
CarolEidt merged 3 commits intodotnet:masterfrom
CarolEidt:Fix38192

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

CarolEidt 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.

@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

CarolEidt commented Jun 25, 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.

@CarolEidt
Copy link
Contributor Author

CarolEidt commented Jun 25, 2020

@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

@AndyAyersMS AndyAyersMS Jun 25, 2020

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

if ((tree->AsCall()->IsUnmanaged() || tree->AsCall()->IsTailCall()) && compMethodRequiresPInvokeFrame())
if ((tree->AsCall()->IsUnmanaged() || tree->AsCall()->IsTailCallViaJitHelper()) &&
compMethodRequiresPInvokeFrame())
Copy link
Member

@AndyAyersMS AndyAyersMS Jun 25, 2020

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