Repository navigation
[Interpreter] Emit a safepoint for a branch to itself, a backward leave and a backward switch - #135467
[Interpreter] Emit a safepoint for a branch to itself, a backward leave and a backward switch#135467pcshrosbree wants to merge 4 commits into
Conversation
…ve and a backward switch The interpreter emits a safepoint at method entry and before a backward branch. Three ways of closing a loop got none, so a thread spinning in such a loop was never suspended and a GC on another thread did not complete: - a branch to itself: EmitBranch tested ilOffset < 0, and the offset of `L: br L` relative to the start of the instruction is 0; - a backward leave, which closes a loop whose body ends in a protected region; - a switch with a backward target. Emit the safepoint (and the sampling point) for all three through a shared helper. A branch to itself can be the first instruction of a try region, which puts its safepoint there too. The safepoint stored the start of the instruction as the frame's ip while HandleThreadAbort raised the abort through managed dispatch, which treats that ip as a return address and moves it back before looking up the EH clauses, so the region's handlers were skipped. Record the next instruction's ip while HandleThreadAbort runs, as INTOP_CALL does. Fixes dotnet#135465
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @agocke |
| return canceled; | ||
| } | ||
|
|
||
| public static bool TestLoopSafepoints() |
There was a problem hiding this comment.
This test suite was designed for interpreter bringup, we shouldn't really add any more tests to it. This test could be extracted as an independent runtime test.
There was a problem hiding this comment.
Done in 44fe2f9: the test is now src/tests/Regressions/coreclr/GitHub_135465, and Interpreter.cs is back to what main has. The project is RequiresProcessIsolation and UnloadabilityIncompatible because the loops never exit, and it sets Optimize because only optimized IL closes the loops this way.
Note
This reply was prepared with AI assistance (Anthropic Claude).
| try | ||
| { | ||
| #pragma warning disable SYSLIB0046 // ControlledExecution.Run is obsolete | ||
| System.Runtime.ControlledExecution.Run(loop, cts.Token); |
There was a problem hiding this comment.
I don't believe we should be using this obsolete API in testing. I believe we can have a test that was hanging before while now it returns normally. We shouldn't need to do any thread aborting.
There was a problem hiding this comment.
Does the test for this bug need to call ControlledExecution at all? It should be enough to just call GC.Collect. If GC.Collect hangs, the test is going to fail with timeout so the bug will be identified.
There was a problem hiding this comment.
Agreed, done in 44fe2f9. Each loop now runs on a background thread and the test only calls GC.Collect(): it hangs there without the fix and returns with it. Nothing is aborted.
That left the INTOP_SAFEPOINT change without a test, and it matters only for thread aborts, so I took it out of this PR; I'll bring it with the fix for #135463. The PR now touches only compiler.cpp, compiler.h and the new test, and I've updated the description.
Re-validated on linux-x64, osx-arm64 and windows-x64 (Checked) under DOTNET_InterpMode 1, 2 and 3: the test hangs at the PR's base and passes with the fix, and leaving out any one of the three compiler changes hangs it in the matching loop.
Note
This reply was prepared with AI assistance (Anthropic Claude).
Replace TestLoopSafepoints in the interpreter bring-up suite with src/tests/Regressions/coreclr/GitHub_135465. The new test does not abort the looping threads: it runs each loop on a background thread and checks that a GC completes. With no thread abort left to test, drop the INTOP_SAFEPOINT ip change; it belongs with the fix for thread aborts.
| // the ip of this instruction, an abort at a safepoint that is the first instruction of a | ||
| // try region would skip the region's handlers. | ||
| pFrame->ip = ip + 1; | ||
| CallWithSEHWrapper( |
There was a problem hiding this comment.
It seems that other cases that use CallWithSEHWrapper would have the same issue if they occurred as the first instruction in a try region. E.g. the INTOP_CALLVIRT in the cache miss case.
It seems a better fix would be to pass InterpreterFrame* to the CallWithSEHWrapper and call pInterpreterFrame->SetIsFaulting(true); before calling the DispatchManagedException from the CallWithSEHWrapper.
There was a problem hiding this comment.
Agreed, and that shape works. I took this hunk out of the PR in 44fe2f9: it only matters for thread aborts, and the test here no longer aborts anything.
I tried your version on top of this PR's compiler change: CallWithSEHWrapper takes the InterpreterFrame* and calls SetIsFaulting(true) before DispatchManagedException, with the frame passed at INTOP_SAFEPOINT and at the INTOP_CALLVIRT cache miss. An abort that lands on the safepoint at the start of try { while (true) { } } finally { ... } then runs the finally under DOTNET_InterpMode 1, 2 and 3 on linux-x64, osx-arm64 and windows-x64 (Checked). Without it the finally is skipped. The JIT skips that finally too, so today this is about the interpreter attributing the exception correctly rather than a difference from compiled code.
I'll send it as its own PR once this one is in, because its test needs the self-branch safepoint. From reading the code, not every CallWithSEHWrapper caller can pass the frame as it stands: PrepareInterpreterCode runs after ip has been advanced, so pFrame->ip is already a return address there. I'll go through the callers in that PR.
Note
This reply was prepared with AI assistance (Anthropic Claude).
|
Tagging subscribers to this area: @JulieLeeMSFT, @BrzVlad, @janvorli |
| <!-- The interpreter is CoreCLR-VM specific --> | ||
| <CLRTestTargetUnsupported Condition="'$(RuntimeFlavor)' != 'coreclr'">true</CLRTestTargetUnsupported> | ||
| <NativeAotIncompatible>true</NativeAotIncompatible> |
There was a problem hiding this comment.
| <!-- The interpreter is CoreCLR-VM specific --> | |
| <CLRTestTargetUnsupported Condition="'$(RuntimeFlavor)' != 'coreclr'">true</CLRTestTargetUnsupported> | |
| <NativeAotIncompatible>true</NativeAotIncompatible> |
The test should pass on all runtime flavors. We want to make sure that this bug does not exists on all runtime flavors.
There was a problem hiding this comment.
Done in 7264ed4. It also passes as a NativeAOT build (linux-x64, Checked); I haven't run it on Mono. From source inspection, I expect the Mono interpreter to hang on the branch to itself and the backward switch under hybrid/cooperative suspension.
It is a relatively expensive test for a niche case.
The test is not specific to the CoreCLR interpreter: on any runtime, GC.Collect() returns only if the thread spinning in the loop can be suspended. Drop the exclusion of runtime flavors other than CoreCLR, with its comment, and NativeAotIncompatible.
Fixes #135465.
An interpreted thread polls for suspension at
INTOP_SAFEPOINT, which the compiler emits at method entry and before a backward branch. Three ways of closing a loop got none, so a thread spinning in such a loop is never suspended and a GC on another thread does not complete:EmitBranchgets the offset relative to the start of the branch instruction and testedilOffset < 0.while (true) { }compiles toL: br.s L, whose offset is 0. This is the case in the issue.leave. With optimization the C# compiler points theleaveat the end of a protected region straight at the loop head, sowhile (true) { try { ... } catch (...) { ... } }is closed by aleave, not by abr.EmitLeavemarked the target as a loop head but emitted no safepoint.switchwith a backward target. Theswitchcase did the same. The C# compiler produces one forL: switch (x) { case 0: goto L; ... }.The three now share a helper,
EmitLoopSafepoint, which emits whatEmitBranchemitted inline (the safepoint and the sampling point).Test:
src/tests/Regressions/coreclr/GitHub_135465starts four loops in turn, each on its own background thread: an empty loop, a loop closed by aleaveout of a try, one closed by aleavethrough a finally, and one closed by aswitch. Once a loop is running, the main thread callsGC.Collect(). Three things to know about it:GC.Collect(); it does not fail.Optimize.RequiresProcessIsolationandUnloadabilityIncompatible, and the threads end with the process.Validation (Checked runtime, at this PR's base), with the test run under the JIT and under
DOTNET_InterpMode1, 2 and 3:GC.Collect()with the first loop runningGC.Collect()with the first loop runningGC.Collect()with the first loop runningUnder the JIT the test passes with and without the fix. On linux-x64 and osx-arm64 it also passes through the merged
Regressionsrunner, which is how CI starts it.Each part is needed, and for one loop only. With one part left out at a time (
DOTNET_InterpMode=1), on all three platforms:EmitBranchchangeEmitLeavechangeleaveswitchchangeswitchOn windows-x64 the test assembly was the one built on macOS.
The first revision of this PR also changed the ip that
INTOP_SAFEPOINTrecords while a thread abort is raised. That matters only for thread aborts, and its test usedControlledExecution, so I took both out; that change belongs with #135463.Note
This PR was prepared with AI assistance (Anthropic Claude, reviewed by OpenAI Codex). The builds and runs it cites were done at the PR's base commit.