Guest debugging: ensure that instrumentation-stored GC refs are updated by moving GC

[cfallin]

(carried over from this review discussion on #11769)

Currently, guest-debug instrumentation stores all local/stack values in a stackslot for observability by the debugger. This works well for primitives like integers/floats/vectors, and is fine for GC refs when the GC is not moving (i.e. tracing only observes the roots read-only) because the values also exist elsewhere in the program; but will be a problem once the GC becomes a moving GC.

[github-actions]

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "wasmtime:ref-types"

Thus the following users have been cc'd because of the following labels:

• fitzgen: wasmtime:ref-types

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[cfallin]

Now that I write this out, I think actually we need to root GC refs properly in the instrumentation slot; DCE could otherwise remove the parts of the original program that would keep the ref rooted.

[fitzgen]

Semi-related to this issue: we could extend ir::UserStackMapEntry to have an opaque, user-defined u32 if necessary. Right now, the only way to differentiate different entries is via their type, but if we need to have different kinds of stack map entries that happen to have the same ir::Type, we could use this hypothetical u32.

But actually I think maybe all that is necessary here is to call the same underlying DFG APIs that the safepoints-insertion pass in the frontend does, so that the locals and operand stack slots that exist for the debug instrumentation have associated stack map entries recorded.

[cfallin]

Ah, so I've implemented actual rooting here as part of the draft PR that follows instrumentation; it ended up being simpler to use the frame tables to walk the slots than to try to conmingle the two concepts somehow in the metadata or implementation. That commit has a Fixes #11841 that should close this once the followup happens :-)

[fitzgen]

That seems like a fair amount of machinery that I think could be replaced by a call to https://docs.rs/cranelift-codegen/0.124.1/cranelift_codegen/ir/dfg/struct.DataFlowGraph.html#method.append_user_stack_map_entry at the appropriate places and then we get to reuse the existing stack maps infrastructure? Introducing new FP-indexing and pointer arithmetic and all that seems a bit extra if it isn't actually necessary in this case.

it ended up being simpler to use the frame tables to walk the slots than to try to conmingle the two concepts somehow in the metadata or implementation.

I feel like I must be misunderstanding something because I feel that the approach in that commit is the one that is mixing up two concepts: the GC's stack walking now needs to query and interpret two sources of truth (stack map metadata and debug info metadata) to find live GC references on the stack. On the flipside, I would expect that by adding stack map entries for the stack slot offsets that contain live GC refs, we keep one source of truth for stack tracing, and we do not conflate debug info and stack maps. The debugger uses debug info, the GC uses stack maps.

[cfallin]

Ah, it appears that I had misremembered how stackmaps worked -- I had been imagining there was a separate stackslot and I'd have to add some new kind of entry to refer to my stackslot; but indeed not the case, I can refer to any stackslot+offset. I'll update it -- thanks!

[cfallin]

This was closed in #11769.