[Wasm RyuJit] annotate (some) local accesses with var nums#124880
[Wasm RyuJit] annotate (some) local accesses with var nums#124880AndyAyersMS wants to merge 5 commits intodotnet:mainfrom
Conversation
This was an attempt to reproduce some of the extra annotations we see in native dumps, for locals that are spilled to the shadow stack.
github-actions
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
Adds extra disassembly annotations for WASM RyuJit to make shadow-stack local loads/stores easier to correlate with JIT locals, similar to what native dumps show.
Changes:
- Teach
emitIns_S(WASM) to attach local/offset metadata to emitted instructions when debug/disasm info is enabled. - Extend WASM instruction display (
emitDispIns) to print local annotations forIF_MEMARGinstructions. - Initialize
instrDescDebugInfo::idVarRefOffstoBAD_VAR_NUMfor WASM so localV00can be distinguished from “no info”.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/coreclr/jit/emitwasm.cpp | Records local/offset metadata during emission and prints it in WASM disassembly for memory-argument instructions. |
| src/coreclr/jit/emit.cpp | Sets a WASM-specific sentinel default for debug-only local-reference metadata. |
|
This only annotates loads right now since stores do not go through The debuginfo-only fields I am reusing seem largely dead. Maybe they should be cleaned up on other architectures. @dotnet/jit-contrib worth doing anyways? |
I think this is a good change, but we should add new fields instead of reusing/overloading the existing half-dead ones. E. g. |
Instead of reusing existing idVarRefOffs/idVarRefOffs2 fields, introduce new idLclNum and idLclOffset fields in instrDescDebugInfo for WASM to store local variable number and field offset. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Initialize idLclNum to 0, store varx+1 when setting, and subtract 1 when displaying. This avoids depending on BAD_VAR_NUM as a sentinel. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
instrDescDebugInfo is already fully zero-initialized via memset, so the explicit assignment is unnecessary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@kg PTAL |
| unsigned log2align = emitGetAlignHintLog2(id); | ||
| cnsval_ssize_t offset = emitGetInsSC(id); | ||
| printf(" %u %llu", log2align, (uint64_t)offset); | ||
| dispLclVarInfoIfAny(); |
There was a problem hiding this comment.
The IF_ULEB128 family of formats should get the same treatment (used for local address offsets).
| info->idNum = emitInsCount; | ||
| info->idSize = sz; | ||
|
|
||
| id->idDebugOnlyInfo(info); |
There was a problem hiding this comment.
Nit: if we called the constructor for info properly here (using placement_t I suppose), and assigned the fields their default values via initializers, we wouldn't need the 'offset by one' trick, and could delete the asserts above too.
This was an attempt to reproduce some of the extra annotations we see in native dumps, for locals that are spilled to the shadow stack.