[mono][wasm] Pin OP_MOVE destinations in the GC pin area - #132306
[mono][wasm] Pin OP_MOVE destinations in the GC pin area#132306pavelsavara wants to merge 2 commits into
OP_MOVE destinations in the GC pin area#132306Conversation
|
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. |
There was a problem hiding this comment.
Pull request overview
Adds a new regression test intended to guard against the wasm/Mono LLVM-AOT OP_MOVE alias rooting issue described in #130592, by asserting that an object reachable from a live local survives GC across repeated address-taken-local reassignment + collection cycles.
Changes:
- Add a new Directed JIT test project under
src/tests/JIT/Directed/gcpin/. - Add a new xUnit test (
MoveAliasGcRoot) that stress-loops throughalias = cur; cur = Make(i); GC.Collect()and validates the aliased object remains intact.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/tests/JIT/Directed/gcpin/MoveAliasGcRoot.csproj | New test project definition for the regression test. |
| src/tests/JIT/Directed/gcpin/MoveAliasGcRoot.cs | New xUnit regression test exercising MOVE-alias rooting across GC. |
| <Project Sdk="Microsoft.NET.Sdk"> | ||
| <PropertyGroup> | ||
| <Optimize>True</Optimize> | ||
| </PropertyGroup> | ||
| <ItemGroup> |
| // On the Mono LLVM-AOT wasm backend, GC references are kept scannable by storing each | ||
| // ref vreg into a linear-memory "gc pin" area, because wasm value-stack locals are | ||
| // invisible to SGen's conservative stack scan. OP_MOVE dests used to be skipped there, | ||
| // on the assumption the source's pin slot covers them. | ||
| // |
|
/azp run runtime-wasm |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
OP_MOVE destinations in the GC pin area
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/tests/JIT/Directed/Directed_ro.csproj:23
- PR title/description indicate a runtime fix to wasm AOT codegen (mini-llvm.c emit_gc_pin logic for OP_MOVE), but this PR currently only adds the new gcpin test project reference here. In the current tree, src/mono/mono/mini/mini-llvm.c still has the old condition
... && ins->opcode != OP_MOVE && ins->opcode != OP_AOTCONST(around mini-llvm.c:12755), so the described fix does not appear to be included. Either add the runtime change to the PR or update the PR title/description to match what’s actually being submitted.
<MergedWrapperProjectReference Include="FaultHandlers\Nesting\Nesting.ilproj" />
<MergedWrapperProjectReference Include="FaultHandlers\Simple\Simple.ilproj" />
<MergedWrapperProjectReference Include="gcpin\MoveAliasGcRoot.csproj" />
<MergedWrapperProjectReference Include="IL\leave\leave1.ilproj" />
|
/azp run runtime-wasm |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
91dcdde to
da9673f
Compare
|
/azp run runtime-wasm |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/GCTests.cs:1265
- The failure message interpolation allocates a new string on every loop iteration (even when the test passes). Consider constructing the message only on failure to avoid unnecessary allocations/extra work in a GC-focused test.
Assert.True(alias.IsIntact(i - 1),
$"object referenced by a live local was lost across GC at iteration {i} (read Value={alias.Value})");
GC.KeepAlive(alias);
| // Regression test for https://github.com/dotnet/runtime/issues/130592. On the Mono | ||
| // wasm LLVM-AOT backend a ref OP_MOVE alias of an address-taken local could be left | ||
| // rooted nowhere once the local was reassigned, letting the aliased object be collected | ||
| // while a live local still referenced it. The invariant -- an object reachable through a | ||
| // live local survives a collection -- holds on every runtime, so this only has teeth on | ||
| // wasm AOT (it must be in the browser Mono smoke set to run there). |
|
/azp run runtime-wasm |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run runtime-wasm |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/GCTests.cs:1256
- The PR description says the wasm LLVM-AOT codegen change was made (pin OP_MOVE destinations conditionally via a helper like
move_needs_gc_pin), but the current tree still unconditionally skips OP_MOVE at the only TARGET_WASMemit_gc_pincall site insrc/mono/mono/mini/mini-llvm.c(it still hasins->opcode != OP_MOVE && ins->opcode != OP_AOTCONST). If this PR is meant to fix the codegen half of #130592, the runtime-side change appears to be missing from this branch.
// Regression test for https://github.com/dotnet/runtime/issues/130592. On the Mono
// wasm LLVM-AOT backend a ref OP_MOVE alias of an address-taken local could be left
// rooted nowhere once the local was reassigned, letting the aliased object be collected
// while a live local still referenced it. The invariant -- an object reachable through a
// live local survives a collection -- holds on every runtime, so this only has teeth on
// wasm AOT (it must be in the browser Mono smoke set to run there).
[Fact]
public static void MovedAliasOfAddressTakenLocalIsRootedAcrossGC()
|
/azp run runtime-wasm |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Testing ....
Fixes the AOT codegen half of #130592.
Related #132180
Objects that are still referenced by a live local can be collected in Mono LLVM-AOT
browser-wasm code. The fix stops excluding
OP_MOVEdestinations from the GC pin area,but only for the moves that actually need it, so the size cost is negligible
(+0.15% of AOT code, +186 bytes compressed).
Background: how GC references stay alive in wasm AOT code
This part is unusual enough that the bug does not make sense without it.
On normal targets, SGen finds GC references by scanning the machine stack
conservatively: anything on the stack that looks like a heap pointer is treated as a
root, and the object it points to is pinned so the collector will not move it.
WebAssembly has no addressable machine stack. Local variables live in wasm locals
— slots in the VM's own execution state. They are not part of linear memory, so a
conservative scan of linear memory structurally cannot see them. A reference that lives
only in a wasm local is invisible to the GC.
Mono works around this with a GC pin area. For each method, the AOT compiler
allocates a small array in the method's linear-memory frame (
emit_entry_bb), reservingone slot per reference-typed virtual register. Every time a ref vreg is assigned, the
compiler emits a volatile store of that value into its slot (
emit_gc_pin). Because thepin area is in linear memory, the conservative scan finds it, and the objects are
pinned.
So on wasm the rule is: a reference is only a GC root if something wrote it into the pin
area (or into some other linear-memory location). If the compiler skips that store, the
value is invisible to the GC even though the program is still using it.
Two details matter later:
emit_gc_pindeliberately skips address-taken (volatile/indirect) variables andreturns early. Those variables already live in their own linear-memory stack slot
(
emit_volatile_storewrites them there), so a pin slot would be redundant. They arenot given a pin slot at all.
OP_MOVEin Mono's IR emits no LLVM instruction.values[dreg] = values[sreg1]simply makes the destination an SSA alias of the source. There is no new value, so the
original code assumed there was no new root to record.
The bug
OP_MOVEdestinations were excluded fromemit_gc_pin, on the assumption that thesource is already rooted, so the alias is covered.
That assumption breaks when the source is an address-taken variable:
emit_gc_pinskips it, as above). Its only root is itslinear-memory stack slot.
alias = sourceis anOP_MOVE, so under the old conditionaliasgot no pin storeeither.
aliasexists only as a wasm local.source = something_else) overwrites that stack slot — theonly root for the previous object.
The previous object is now referenced solely by
alias, which lives in a wasm local thatthe conservative scan cannot see. It is not pinned and not reachable, so the next
collection frees it (or moves it without updating the alias). The program then keeps using
a dangling reference.
That produces exactly the symptoms in #130592: reads returning garbage,
mono_class_get_flags: unexpected GC filler class, out-of-bounds accesses, and hangs inside a collection.Reproduction
src/tests/JIT/Directed/gcpin/MoveAliasGcRoot.cs:The asserted invariant — an object reachable through a live local survives a collection —
holds on every runtime, so this test is safe to run everywhere. It only has teeth on
wasm AOT.
Measured on
browser-wasm,Release,RunAOTCompilation=true, via theconsole-nodesample:
lost at iteration 28 (read Value=3652889), byte-identical each runNotes:
with the same garbage value every run.
MONO_GC_DEBUG=clear-at-gconly makes it fail sooner (iteration 5 instead of 28).
Collect()from one smallarray to 32 made the failure disappear, presumably by changing promotion behaviour. The
committed form is the one verified to fail.
How common is the shape?
A temporary census in the AOT compiler over the five assemblies AOT'd by the sample
(including
System.Private.CoreLib):OP_MOVEsOP_MOVEs totalSo the vulnerable pattern is not exotic; it appears in a few hundred CoreLib methods
alone. Whether a given site actually strands an object additionally depends on the object
being the sole reference across a collection, which is why it surfaces as rare, random
corruption rather than a consistent failure.
These two rows are also what the conditional rule targets: roughly 2,300 of the 12,047 ref
moves need a pin slot, and the other ~80% are provably covered by their source and are
skipped. That ratio is what produces the size numbers below.
History of this code
Useful context, because the exclusion being removed was never a correctness decision.
!= OP_MOVE, described as "Avoid storing arguments and results of moves into the gc_pin area." No correctness argument was given.OP_MOVEskip and the volatile/dead skip, but not the argument skip, with the note: "The OP_ARG one causes random crashes."!= OP_AOTCONST, which is genuinely safe — those are GOT loads ofldstrliterals and type/method handles, already rooted by the loader's interned tables.In other words: the
OP_MOVEexclusion came from a size micro-optimization batch that wasalready partially reverted for causing random crashes, and the sibling exclusion from the
same batch was dropped for exactly the class of symptom seen here. This change removes the
remaining unsound member of that batch.
The fix
Pin
OP_MOVEdestinations, but only when the source can stop being rooted while the aliasis still live.
OP_AOTCONSTkeeps its blanket exclusion.A move is skipped only when the source is provably rooted for the rest of the method:
vreg_defcount <= 1). Such a vreg's pin slot is written once andnever overwritten, so the object stays pinned.
emit_entry_bbcounts definitions in oneextra walk over the IR, saturating at 2; it runs after the
OP_LDADDRpass, soMONO_INST_INDIRECTis already set by then. Arguments have zero IR definitions and arepinned in the prologue, so they fall into this bucket too.
MONO_INST_VOLATILE/INDIRECT/IS_DEADvariables have no pinslot at all and live in a stack slot that this method or a callee can overwrite through
the escaped pointer — which is exactly the bug — so they are always pinned.
Everything else is pinned, so the rule is conservative by default: anything unknown
(no def-count array, out-of-range vreg) returns
TRUE.The rule is also transitively sound for chains of moves.
c = MOVE b; b = MOVE abottomsout at a real definition: if
ais single-def and not address-taken, its slot roots theobject for the whole method; if it is not,
bgets pinned and — being single-def itself —keeps its slot for the rest of the method, covering
c.Writing a MOVE destination's slot is safe by construction:
emit_entry_bbalready reservesa pin slot for every non-volatile, non-dead ref vreg, using the same predicate
emit_gc_pinfilters on. A MOVE destination therefore writes a slot that was alreadyallocated and left empty. It cannot collide with the source's slot, cannot grow the frame,
and cannot shift any other vreg's index.
Why not just pin every ref move?
That was the first version of this change and it also fixes the bug, but it costs about
9.5x more code size for no additional correctness (see below). Both variants were built
and measured; the numbers for the unconditional variant are kept in the table for
comparison.
Binary size impact
Measured on the
console-nodesample,browser-wasm,Release,RunAOTCompilation=true,AOT'ing 5 assemblies including
System.Private.CoreLib. All three configurations were builtfrom an identical tree with a full
mono+libsrebuild and the identical app source; only thepin condition differs. The baseline was reproduced twice, byte-identical.
Final shipped artifact —
dotnet.native.wasm:Brotli is what users actually download, and there the cost of this PR is 186 bytes —
indistinguishable from noise.
AOT-compiled managed code only (per-assembly object files), which isolates the codegen
change from the fixed runtime portion:
aot-instances.dll.oSystem.Private.CoreLib.dll.oSystem.Runtime.InteropServices.JavaScript.dll.oSystem.Console.dll.oWasm.Console.Node.Sample.dll.oSo the conditional rule costs +0.15% of AOT code size instead of +1.45%, a 9.5x
reduction, while still fixing the bug (verified: the repro passes 2/2 with this version).
For reference, the original PR reported ~3.4 MB on a 65.5 MB application (~5%).