[wasm] Report on-frame GC slots as pinned

[lewing]

Fixes #131373

Problem

On wasm, a GC reference loaded from its linear-stack home onto the wasm operand stack is a copy the GC can neither see nor update. If the object is relocated at a safepoint, the GC fixes up the home slot but the pushed copy keeps the stale pre-move address, and the consumer dereferences it.

The wasm ABI already specifies the model that prevents this — docs/design/coreclr/botr/clr-abi.md, "GC References at Call Sites":

…all GC references live after the call (and all untracked GC references, which are effectively always live) must be saved to the linear stack. These GC references will be reported as pinned to the GC so that if they normally live in Wasm locals those locals do not need to be updated after the call.

GC-typed locals already satisfy the "saved to the linear stack" half: they are denied wasm locals via DoNotEnregisterReason::WasmGCVisibility and homed on the frame. But they were not being reported pinned — in gcMakeRegPtrTable they are encoded GC_SLOT_UNTRACKED, and GC_SLOT_PINNED was only added when varDsc->lvPinned is set, which ordinary GC locals never set. So the referent moved and copies on the operand stack went stale.

The symptom was an intermittent NullReferenceException in RuntimeCustomAttributeData..ctor and similar reflection paths during xUnit discovery under wasm R2R.

Fix

Report on-frame GC slots as pinned on wasm. Non-wasm targets are untouched, and this adds no code to the generated image — only GC-info flags. These slots were already reported and scanned as GC roots, so there is no new zero-initialization requirement; it only tells the GC not to relocate.

fgWasmSpillRefs is unchanged and remains load-bearing: refs with no frame home (call results, indirections, computed byrefs) still need spilling, and pinning cannot cover them.

Validation

Measured on a browser R2R rig with the pinning change as the only variable (identical fgwasm.cpp on both sides; fixed = pinning, unfixed = gcencode.cpp reverted).

	unfixed	fixed
Runtime_131373	fails 3/3	passes 3/3
NRE, 5-seed xUnit discovery	3–8 NREs/seed; 3 seeds crash before completing	0 NREs, clean 12166/12166
Bcl.Memory / Unsafe	—	550/550, 128/128

Cost, versus the same composite without pinning: +22,480 bytes, entirely GC-info flags. Peak RSS under an allocation-churn workload with forced compacting collects was ~344.8 MB pinned vs ~348.7 MB unpinned — within noise. Pervasive pinning does not show up as footprint growth here, because the pinned set at any GC point is a small transient per-frame population rather than a persistent un-compactable mass. That is one synthetic workload and RSS is coarse, so it bounds the risk rather than eliminating it.

Instrumenting the spill pass confirms the two mechanisms address the same population and are complementary: GT_LCL_VAR spills drop from 13,018 across 5,167 methods to 431 across 170, with RuntimeCustomAttributeData falling out of the set entirely. The residual 431 are address-exposed locals, which fgWasmSpillRefs still spills by design. A further 15,684 spills are call results and indirections, which have no home slot and so are unaffected.

Regression test

Runtime_131373 stores through a ref object parameter whose value comes from an allocating call, so the store address is a bare GT_LCL_VAR pushed across a safepoint.

Three variations look correct but silently false-pass, which is worth recording:

• A class field store doesn't work. obj.Field = Call() lowers to STOREIND(ADD(LCL_VAR obj, offset), CALL) because the field is at a non-zero offset (the method table pointer occupies 0). That byref ADD has no linear-stack home and was already spilled. Storing through a ref parameter has no ADD.
• A call argument doesn't work. fgMorphArgs spills an earlier argument to a temp when a later one contains a call, so the load happens after the safepoint.
• Allocation pressure is required. A forced compacting collect on a small unfragmented heap has nothing to relocate, so the pushed copy stays accidentally valid.

The test is also deliberately a synchronous [Fact]: the merged CoreCLR runner emits a bare Type.Method(); statement (XUnitWrapperGenerator/ITestInfo.cs) and never awaits, so an async Task test would have its Task discarded along with its assertion failures.

Note that this fix leaves R2R instruction bytes unchanged — the difference is GC_SLOT_PINNED in the embedded GC info — so the behavioural fail/pass is the load-bearing evidence rather than an image comparison.

History

This PR originally fixed the hole in fgWasmSpillRefs, by spilling GT_LCL_VAR-sourced refs into pinned slots and narrowing the spill set to exclude direct call operands. That worked, but as @AndyAyersMS pointed out it re-spills values that already have linear-stack homes — the defect is the missing pinned reporting, one layer down. The measurements above bear that out: the reporting fix is 268,644 bytes smaller than the spill approach on the same composite.

Note

This change was authored with GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
Successfully started running 6 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

This PR updates the WebAssembly-specific fgWasmSpillRefs pass to ensure GC refs/byrefs that are live across calls are spilled to pinned shadow-stack slots even when the value originates from a local, and adds a wasm R2R regression test covering the scenario.

Changes:

• Extend the spill-candidate tracking to include GT_LCL_VAR ref/byref nodes (so they can be pinned/spilled across nested calls).
• Add a narrowing step at each call site intended to avoid spilling certain non-address-exposed locals that are directly consumed by the call.
• Add a new JitBlue regression test (Runtime_131373) with AlwaysUseCrossGen2 on the browser leg to ensure wasm R2R coverage.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/coreclr/jit/fgwasm.cpp	Adjusts wasm spill pass to treat GT_LCL_VAR refs as spill candidates and attempts to narrow spills for call-consumed locals.
src/tests/JIT/Regression/JitBlue/Runtime_131373/Runtime_131373.cs	Adds a regression reproducer exercising a local-held ref across an allocating call in wasm R2R.
src/tests/JIT/Regression/JitBlue/Runtime_131373/Runtime_131373.csproj	Configures the test to force CrossGen2 for TargetOS=browser so it runs as wasm R2R on the browser leg.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/tests/JIT/Regression/JitBlue/Runtime_131373/Runtime_131373.cs:78

• Allocate currently always performs allocation churn and a forced compacting GC.Collect, even on non-browser legs where the test isn't intended to validate wasm R2R behavior. This can make the test suite significantly slower on other platforms. Consider guarding the expensive churn/collect behind an OperatingSystem.IsBrowser() check so only the browser leg pays the cost.

    private static object Allocate(int i)
    {
        for (int j = 0; j < 48; j++)
        {
            byte[] garbage = new byte[8192];
            GC.KeepAlive(garbage);
        }

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/tests/JIT/Regression/JitBlue/Runtime_131373/Runtime_131373.cs:44

• The test only exercises the intended wasm R2R path on the browser leg (via AlwaysUseCrossGen2), but currently runs unconditionally. Given the heavy allocation/GC behavior in this test, consider making it browser-only using ConditionalFact + PlatformDetection to avoid slowing down non-browser test legs that don’t provide coverage here.

using System;
using System.Runtime.CompilerServices;
using Xunit;

public class Runtime_131373
{
    public sealed class Box
    {
        public object Slot;
        public int Tag;
    }

    [Fact]
    public static void TestEntryPoint()

src/tests/JIT/Regression/JitBlue/Runtime_131373/Runtime_131373.cs:47

• This test currently does 400 iterations, each forcing a compacting Gen2 GC (via Allocate), plus substantial allocation churn. That’s likely to be very slow (especially on wasm/node) and risks test timeouts; the PR description indicates the failure happens on the first iteration, so the outer loop can likely be reduced to a small count while still guarding the regression.

    {
        for (int i = 0; i < 400; i++)
        {

[dotnet-policy-service]

Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara
See info in area-owners.md if you want to be subscribed.

[azure-pipelines]

Azure Pipelines:
Successfully started running 6 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[AndyAyersMS]

GC-typed locals live on the shadow stack (they are not eligible to be "register allocated" aka kept in Wasm locals) and should already be getting reported as pinned from their shadow stack homes.

If this is not happening, we should fix the GC reporting, rather than adding code to re-spill things that are already essentially spilled.

[lewing]

You're right, and the ABI says so explicitly — I was fixing this a layer too high.

all GC references live after the call (and all untracked GC references, which are effectively always live) must be saved to the linear stack. These GC references will be reported as pinned to the GC so that if they normally live in Wasm locals those locals do not need to be updated after the call.

(docs/design/coreclr/botr/clr-abi.md, "GC References at Call Sites")

It isn't happening today. On-frame GC locals are encoded GC_SLOT_UNTRACKED and only pick up GC_SLOT_PINNED when varDsc->lvPinned is set (gcencode.cpp:4182-4194), which ordinary GC locals never set. So the home is reported and updated, the object moves, and the copy pushed onto the Wasm operand stack goes stale — which is the NRE we were chasing.

So I'll pivot this to the reporting fix. One question on the right layer before I do:

Should on-frame Wasm GC roots get GC_SLOT_PINNED directly in gcMakeRegPtrTable while tracked slot lifetimes stay disabled? Or do you want them marked pinned earlier, despite the lvPinned => !lvTracked invariant in liveness.cpp:430-438?

The encoder looks like the less invasive layer given that Wasm already forces noTrackedGCSlots, but you'd know whether that conflicts with where tracked-slot support is headed.

One thing I'd keep from this PR either way: the regression test. It fails deterministically without a fix and passes with one, and it should be equally valid against the reporting fix.

Note

This reply was drafted with GitHub Copilot.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/coreclr/jit/gcencode.cpp:4192

• The new wasm pinning comment reads as if it applies generally to “refs homed on the linear stack”, but this logic currently only runs for stack slots reported via the untracked section. Since wasm forces noTrackedGCSlots = true, that’s effectively all on-frame GC locals today; if tracked GC slots are ever enabled, the same pinning rule would need to be applied in gcMakeVarPtrTable (tracked-lifetime path) to avoid reintroducing this behavior gap.

            // Per the wasm ABI refs homed on the linear stack are reported pinned, since copies of them
            //  on the wasm operand stack are invisible to the GC. See "GC References at Call Sites".
#ifndef TARGET_WASM

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[AndyAyersMS]

Code change LGTM.

I don't think we need the detailed comments in the test case.

[lewing]

Thanks — comments cut in 3f6f612.

Worth noting the header block had also gone stale: it still described the original fgWasmSpillRefs diagnosis rather than the reporting fix, so it was actively misleading as well as long. Left two lines on what goes wrong and that it only reproduces under wasm R2R, plus one line on the csproj explaining why crossgen is forced on the browser leg (without it that property looks removable).

One correction to an earlier reply of mine on the gcencode.cpp thread, so the record is accurate: I said I'd added a note about the noTrackedGCSlots dependency and a forward reference on the TODO-WASM. I subsequently removed both when trimming, on the grounds that they were speculating about a change nobody has made and would rot. The underlying hazard is real though — if tracked GC slots are ever enabled on wasm, these locals would be reported by gcMakeVarPtrTable, which pins only on pinned_OFFSET_FLAG, and the pinning would silently stop happening with nothing on another target to catch it. Happy to file that as a follow-up issue against the TODO-WASM rather than carry it as a comment, if you think it's worth tracking.

Note

This reply was drafted with GitHub Copilot.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/coreclr/jit/gcencode.cpp:4194

• The wasm-specific pinning behavior here depends on wasm currently forcing noTrackedGCSlots=true (so on-frame GC refs/byrefs are reported via this untracked section). The new comment doesn’t mention that dependency or that enabling tracked GC slots would require applying the same pinning rule in gcMakeVarPtrTable, which risks the original issue silently coming back when the TODO is addressed.

            // Per the wasm ABI refs homed on the linear stack are reported pinned, since copies of them
            //  on the wasm operand stack are invisible to the GC. See "GC References at Call Sites".
#ifndef TARGET_WASM
            if (varDsc->lvPinned)
#endif // !TARGET_WASM

[jakobbotsch]

I thought lvPinned was expected to be added here:

runtime/src/coreclr/jit/fgwasm.cpp

Line 1919 in bd240d9

varDsc->lvPinned = true;

How do we end up with a local live across a safe point without the logic here kicking in?

[lewing]

Because that path is never reached for a plain local — fgWasmSpillRefs explicitly skips non-address-exposed GT_LCL_VAR:

If a value is just a GT_LCL_VAR that isn't address-exposed, by construction we ensure that it won't be mutated between its def (here) and its use (the call that would produce a spill) and we won't need to spill it.

That reasoning holds for IR mutation but not for relocation, so the skip is where the hole comes from.

Removing the skip is exactly what this PR did originally — locals then flowed to the lvPinned slot you linked. @AndyAyersMS's point was that it re-spills a value that already has a linear-stack home, and pinning that home gets the same effect without the extra slot, store, reload and block-end zeroing. Measured on the framework composite: +22,480 bytes for pinning vs +291,124 for the spill version.

Note

This reply was drafted with GitHub Copilot.

[jakobbotsch]

Hmm, that makes sense. But:

Removing the skip is exactly what this PR did originally — locals then flowed to the lvPinned slot you linked. @AndyAyersMS's point was that it re-spills a value that already has a linear-stack home, and pinning that home gets the same effect without the extra slot, store, reload and block-end zeroing. Measured on the framework composite: +22,480 bytes for pinning vs +291,124 for the spill version.

Yes, it makes sense that pinning stuff will make the code smaller, but that is a trade off with GC performance and fragmentation. Doesn't this PR essentially report everything as pinned always? If we are going to do that then reloads from the shadow stack seem unnecessary in the first place.

[jakobbotsch]

Can we just mark the locals we identify above as lvPinned directly?

[AndyAyersMS]

We aren't reloading (at least we shouldn't be), just spilling.

[jakobbotsch]

We aren't reloading (at least we shouldn't be), just spilling.

I mean that if we are going to pin all references on the shadow stack, then we should be able to register allocate them too (keeping them both in the shadow stack and in the wasm local).

[lewing]

We could set Wasm only stack pinning flag in a variant of the initial fix that we respect in slot pinning instead of doing it unconditionally?

[jakobbotsch]

Explicitly marking the locals as pinned may have other side effects so may not be as clean as it might sound.

I suppose we could have a new bit on LclVarDsc if we are worried about it.

[lewing]

I'll prototype that but my primary goal at the moment is to get tests stood up without crashing so if there isn't consensus I can open and issue and a different pr to track design resolution?

[jakobbotsch]

If Andy thinks pinning everything is the best, then feel free to go with that, but it felt like a big hammer to me and seemed like something we had been avoiding previously.

[jakobbotsch]

GC-typed locals live on the shadow stack (they are not eligible to be "register allocated" aka kept in Wasm locals) and should already be getting reported as pinned from their shadow stack homes.

Hmm, maybe I am mistaken, but I thought we were reloading these locals from the shadow stack at uses. Why do they need to be pinned?

[lewing]

They are reloaded at every use, but the load lands where the wasm value stack requires, which can be before an intervening safepoint — see genCodeForLclVar:

we can't "reload at the point of use", since that would require inserting instructions into the middle of an already-emitted instruction group. Instead, we order the nodes in a way that obeys the value stack constraints of WASM precisely.

For STOREIND(addr, CALL), genCodeForStoreInd pushes the address, then runs the call. The pushed copy is stale afterwards and can't be reloaded — the call's result is on top of it. The ABI example for x(a, y(b)) has the same shape: local.get a, then call_indirect to y with a still on the operand stack.

Measured: 13,018 such values across 5,167 framework methods; the test here fails on the first iteration without pinning.

Your question does point at the alternative, though — "we may want to un-nest calls, relying on a Wasm local instead of the Wasm stack" would make reload-at-use literally true and drop the need to pin these. Much larger change, and it wouldn't remove fgWasmSpillRefs (call results have no frame home — 15,684 of those). Happy either way if you and @AndyAyersMS would rather wait for that.

Note

This reply was drafted with GitHub Copilot.

[jakobbotsch]

They are reloaded at every use, but the load lands where the wasm value stack requires, which can be before an intervening safepoint — see genCodeForLclVar:

we can't "reload at the point of use", since that would require inserting instructions into the middle of an already-emitted instruction group. Instead, we order the nodes in a way that obeys the value stack constraints of WASM precisely.

For STOREIND(addr, CALL), genCodeForStoreInd pushes the address, then runs the call. The pushed copy is stale afterwards and can't be reloaded — the call's result is on top of it. The ABI example for x(a, y(b)) has the same shape: local.get a, then call_indirect to y with a still on the operand stack.

The code I linked above is supposed to handle that. When a LIR edge spans a GC safe point, the code is meant to store it into a pinned local and replace the use by that pinned local.

EDIT: Ah ok, I see you responded in the comment.

[lewing]

Opened #131402 as a draft alternative to this, since the objection above is a fair one and easier to judge side by side than in the abstract.

It pins only the locals whose operand-stack copy actually spans a safepoint, rather than every on-frame GC slot: 21,161 vs 141,029 pinned (15.0%, a 6.7× reduction), and slightly smaller (+14,256 B vs +22,512 B) on the same composite against a common baseline. Correctness is equal — same probe 3/3, same 5-seed NRE check clean — so "held across a call" turns out not to be too narrow a criterion.

It also doesn't reuse lvPinned, per @AndyAyersMS's point, and that concern is now measured rather than predicted: a variant that does reuse it fails 8 assertions on !varDsc->lvPinned || !varDsc->lvTracked during Emit GC+EH tables under a checked crossgen, where #131402 is EC=0. So the separate bit is necessary rather than defensive.

One thing I'd keep honest: #131402 reduces the number of pins, not their duration — that's still until the local is overwritten or the frame returns. GC-pressure measurement couldn't separate the two variants on the available workloads, so the static pin count is the defensible number, not a runtime claim.

Only one of the two should be taken. Happy either way — this one is approved and smaller, #131402 is more targeted and directly answers the objection.

Note

This comment was drafted with GitHub Copilot.

[AndyAyersMS]

We aren't reloading (at least we shouldn't be), just spilling.

I guess this comment was confusing. We have to reload but we do it just after spilling (there is no store.tee sort of construct that would make an implicit dup) to keep the stack.

Keeping GC refs in locals (basically doing a general sort of "GC write through") seems like an appealing thing to try, but for now I simply want to get something in place that works, so we can enable CI testing. Once we're good there it will be easier to experiment with alternatives.