Enable inlining P/Invokes into try blocks with no catch or filter clauses V2 #73661
Conversation
…ch or filter clauses (dotnet#73032)" (dotnet#73551)" This reverts commit 2a14280.
…s beforehand on ARM32
|
Linking original #73032 |
|
Since this is fully green on CI with the original offending test re-enabled, I'd like to get this merged by end of week if possible |
|
/azp run runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Most of the gcstress failures are known: #69092, #73679, #73808. |
|
/azp run runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@jakobbotsch I don't understand how the change I made could have caused AVs during SuperPMI replay. Are there any known current issues that this PR might be interacting with? |
Indeed it is not related to your PR, it is being fixed by #74037. |
src/coreclr/vm/exceptionhandling.cpp
Outdated
| if ((GetSP(pDispatcherContext->ContextRecord) < (TADDR)pICF) | ||
| && ((UINT_PTR)pICF < uCallerSP) | ||
| && InlinedCallFrame::FrameHasActiveCall(pICF)) | ||
| && ((UINT_PTR)pICF < uCallerSP)) |
There was a problem hiding this comment.
So we don't link the ICF frame for the whole stub on any platform in any case anymore?
There was a problem hiding this comment.
We still link it for the whole stub on 32-bit platforms. This condition didn't change between main and this PR (the V1 version of the change added the IsActiveCall check, but the V2 version doesn't include it).
There was a problem hiding this comment.
I'll get rid of the diff here since I'm not changing the logic any more
|
@jakobbotsch looks like I'm still seeing SuperPMI failures even though that seems to have been fixed. Any ideas? |
@dotnet/jit-contrib anyone can investigate? (I'm OOF) |
|
Only failure is a build timeout that looks unrelated, so merging this in. |
|
Possible improvements: |
ghost
locked as resolved and limited conversation to collaborators
Try 2 at enabling P/Invoke inlining into try blocks where we don't resume within the same method call.
The first commit is the last attempt. The second commit is the change to fix the break. In particular, we change the code to only check if the return address is non-null in the 64-bit-only branch, so the code for ARM32 stays exactly the same as in main.
I've run this test locally about 30 times and couldn't reproduce the crash with the fix, so I believe this fix works.