Uh oh!
There was an error while loading. Please reload this page.
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
danmoseley
commented
Aug 10, 2022
Linking original #73032 |
jkoritzinsky
commented
Aug 10, 2022
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 |
jakobbotsch
left a comment
There was a problem hiding this comment.
JIT changes still look fine to me. I will trigger gcstress on this as we (the jit team) were seeing failures there.
jakobbotsch
commented
Aug 11, 2022
/azp run runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jakobbotsch
commented
Aug 11, 2022
Most of the gcstress failures are known: #69092, #73679, #73808. |
jkoritzinsky
commented
Aug 15, 2022
/azp run runtime-coreclr gcstress0x3-gcstress0xc |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jkoritzinsky
commented
Aug 16, 2022
@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? |
jakobbotsch
commented
Aug 17, 2022
Indeed it is not related to your PR, it is being fixed by #74037. |
| 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
Uh oh!
There was an error while loading. Please reload this page.
jkoritzinsky
commented
Aug 18, 2022
@jakobbotsch looks like I'm still seeing SuperPMI failures even though that seems to have been fixed. Any ideas? |
jakobbotsch
commented
Aug 18, 2022
@dotnet/jit-contrib anyone can investigate? (I'm OOF) |
jkoritzinsky
commented
Aug 23, 2022
Only failure is a build timeout that looks unrelated, so merging this in. |
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.