[mono][wasm] Pin OP_MOVE destinations in the GC pin area

src/libraries/System.Runtime/tests/System.Runtime.Tests/System/GCTests.cs

@@ -1200,5 +1200,92 @@ private unsafe static void AllocateArrayCheckPinning()
                 }
             }
         }
+
+        private sealed class GcRootBox
+        {
+            public int Value;
+            public GcRootBox Self;
+            public byte[] Payload;
+
+            public GcRootBox(int v)
+            {
+                Value = v;
+                Self = this;
+                Payload = new byte[16];
+                Payload[0] = (byte)v;
+            }
+
+            public bool IsIntact(int expected) =>
+                Value == expected && ReferenceEquals(Self, this) && Payload is not null && Payload[0] == (byte)expected;
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static GcRootBox MakeGcRootBox(int v) => new GcRootBox(v);
+
+        // Taking the address forces the caller's local to be an address-taken variable.
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static void TouchGcRootBox(ref GcRootBox b)
+        {
+            if (b is null)
+            {
+                throw new InvalidOperationException();
+            }
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static void CollectAndScribble()
+        {
+            GC.Collect();
+            // Refill the just-vacated nursery space so a stale reference reads different bytes.
+            byte[] scribble = new byte[64];
+            scribble[0] = 1;
+            GC.KeepAlive(scribble);
+        }
+
+        // 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 is not reported as a
+        // precise root, so when the GC relocates the object the alias keeps the pre-move address
+        // and silently reads whatever later reuses that memory. The invariant -- every live
+        // reference to an object denotes the same object after 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 async Task MovedAliasOfAddressTakenLocalIsRootedAcrossGC()
+        {
+            // Resume on a shallow stack: under a deep caller frame a stale copy of the reference
+            // is often found by the conservative stack scan, which pins the object and hides the bug.
+            await Task.Yield();
+            RunMovedAliasLoop();
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static void RunMovedAliasLoop()
+        {
+            // A heap slot is always fixed up when the GC relocates the object, so it is the
+            // ground truth to compare the stack alias against.
+            GcRootBox[] witness = new GcRootBox[1];
+
+            GcRootBox cur = MakeGcRootBox(0);
+            TouchGcRootBox(ref cur);
+
+            for (int i = 1; i <= 128; i++)
+            {
+                GcRootBox alias = cur;      // ref MOVE; sole remaining stack reference to box (i - 1)
+                witness[0] = cur;
+                cur = MakeGcRootBox(i);     // overwrites cur's stack slot
+                TouchGcRootBox(ref cur);
+                CollectAndScribble();
+
+                if (!ReferenceEquals(alias, witness[0]))
+                {
+                    Assert.Fail($"live local was not updated when the GC relocated the object at iteration {i}");
+                }
+                if (!alias.IsIntact(i - 1))
+                {
+                    Assert.Fail($"object referenced by a live local was lost across GC at iteration {i} (read Value={alias.Value})");
+                }
+                GC.KeepAlive(alias);
+            }
+        }
     }
 }

src/mono/mono/mini/mini-llvm.c

@@ -248,6 +248,8 @@ typedef struct {
 	int *gc_var_indexes;
 	int gc_var_indexes_len;
 	Address *gc_pin_area;
+	/* Saturating count (0-2) of definitions per vreg, used to decide if a ref move needs pinning */
+	guint8 *vreg_defcount;
 	LLVMValueRef il_state;
 	LLVMValueRef il_state_ret;
 } EmitContext;
@@ -4010,6 +4012,27 @@ emit_gc_pin (EmitContext *ctx, LLVMBuilderRef builder, int vreg)
 	LLVMValueRef addr = LLVMBuildGEP2 (builder, ctx->gc_pin_area->type, ctx->gc_pin_area->value, indexes, 2, "");
 	emit_store (builder, convert (ctx, ctx->values [vreg], IntPtrType ()), addr, TRUE);
 }
+
+/*
+ * A ref OP_MOVE aliases its source instead of producing a new value, so it only needs a pin
+ * slot of its own when the source can stop being rooted while the alias is still live. A vreg
+ * with a single definition keeps its pin slot for the rest of the method, but an address-taken
+ * variable has no pin slot at all: it lives in a stack slot which this method or a callee can
+ * overwrite at any point.
+ */
+static gboolean
+move_needs_gc_pin (EmitContext *ctx, MonoInst *ins)
+{
+	MonoCompile *cfg = ctx->cfg;
+	MonoInst *var;
+
+	if (!ctx->vreg_defcount || ins->sreg1 < 0 || (guint32)ins->sreg1 >= cfg->next_vreg)
+		return TRUE;
+	if (ctx->vreg_defcount [ins->sreg1] > 1)
+		return TRUE;
+	var = get_vreg_to_inst (cfg, ins->sreg1);
+	return var && (var->flags & (MONO_INST_VOLATILE | MONO_INST_INDIRECT | MONO_INST_IS_DEAD));
+}
 #endif
 
 /*
@@ -4057,6 +4080,17 @@ emit_entry_bb (EmitContext *ctx, LLVMBuilderRef builder)
 	LLVMTypeRef pin_area_type = LLVMArrayType (IntPtrType (), ngc_vars);
 	LLVMValueRef gc_pin_area = build_alloca_llvm_type_name (ctx, pin_area_type, 0, "gc_pin");
 	ctx->gc_pin_area = create_address (ctx, gc_pin_area, pin_area_type);
+
+	/* Runs after the OP_LDADDR pass in emit_method_inner, so MONO_INST_INDIRECT is already set */
+	ctx->vreg_defcount = g_new0 (guint8, cfg->next_vreg);
+	for (MonoBasicBlock *dbb = cfg->bb_entry; dbb; dbb = dbb->next_bb) {
+		for (MonoInst *dins = dbb->code; dins; dins = dins->next) {
+			if (dins->dreg >= 0 && (guint32)dins->dreg < cfg->next_vreg &&
+				LLVM_INS_INFO (dins->opcode) [MONO_INST_DEST] != ' ' &&
+				ctx->vreg_defcount [dins->dreg] < 2)
+				ctx->vreg_defcount [dins->dreg] ++;
+		}
+	}
 #endif
 
 	/*
@@ -12752,7 +12786,18 @@ MONO_RESTORE_WARNING
 				emit_volatile_store (ctx, ins->dreg);
 #ifdef TARGET_WASM
 			//if (vreg_is_ref (cfg, ins->dreg) && ctx->values [ins->dreg])
-			if (vreg_is_ref (cfg, ins->dreg) && ctx->values [ins->dreg] && ins->opcode != OP_MOVE && ins->opcode != OP_AOTCONST)
+			/*
+			 * OP_MOVE dests need pinning too. A MOVE emits no LLVM instruction, so the dest is an SSA
+			 * alias of the source and used to rely on the source staying rooted. That fails when the
+			 * source is an address-taken variable: emit_gc_pin skips those (they live in their stack
+			 * slot instead), so overwriting the variable drops the only root and leaves the alias
+			 * holding an object rooted nowhere. See dotnet/runtime#130592.
+			 *
+			 * OP_AOTCONST stays excluded - those dests are GOT loads of ldstr literals and type/method
+			 * handles, already rooted by the loader's interned tables.
+			 */
+			if (vreg_is_ref (cfg, ins->dreg) && ctx->values [ins->dreg] && ins->opcode != OP_AOTCONST &&
+				(ins->opcode != OP_MOVE || move_needs_gc_pin (ctx, ins)))
 				emit_gc_pin (ctx, builder, ins->dreg);
 #endif
 		}
@@ -12913,6 +12958,7 @@ free_ctx (EmitContext *ctx)
 	g_free (ctx->is_dead);
 	g_free (ctx->unreachable);
 	g_free (ctx->gc_var_indexes);
+	g_free (ctx->vreg_defcount);
 	g_free (ctx->param_etypes);
 	g_ptr_array_free (ctx->phi_values, TRUE);
 	g_free (ctx->bblocks);