GC info in webassembly R2R files was not found/decoded properly#128860
Open
davidwrighton wants to merge 3 commits into
Open
GC info in webassembly R2R files was not found/decoded properly#128860davidwrighton wants to merge 3 commits into
davidwrighton wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the ReadyToRun toolchain (R2RDump and ILCompiler ReadyToRun/Reflection readers) to better handle WebAssembly R2R payloads, particularly around GC-info encoding/decoding and diagnostics.
Changes:
- Add a new
--dump-rvaswitch to R2RDump to dump raw bytes at a specifiedRVA:Size. - Adjust WebAssembly-specific GC-info handling (skip fixed stack parameter scratch area decoding; remove 4-byte alignment/padding in the writer path).
- Avoid throwing for Wasm32 debug register name decoding by returning a placeholder string.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/tools/r2rdump/TextDumper.cs | Changes raw byte dumping behavior for unwind info (now attempts dump on all platforms). |
| src/coreclr/tools/r2rdump/R2RDumpRootCommand.cs | Introduces the --dump-rva CLI option. |
| src/coreclr/tools/r2rdump/Program.cs | Implements parsing/handling for --dump-rva. |
| src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/GCInfoTypes.cs | Adds a per-target flag controlling whether fixed stack parameter scratch area is encoded/decoded. |
| src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/DebugInfo.cs | Returns a placeholder register name for Wasm32 instead of throwing. |
| src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs | Conditions decoding of stack outgoing/scratch area on the new GC-info trait flag. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodGCInfoNode.cs | Skips 4-byte alignment/padding for Wasm32 when laying out / emitting runtime function GC-info blobs. |
Comments suppressed due to low confidence (2)
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodGCInfoNode.cs:67
CalculateFuncletOffsetsstill reserves 4 bytes for the personality routine on Wasm (if (arch != X86) offset += sizeof(uint)), butEncodeDataCoreexplicitly does not emit a personality routine component forTargetArchitecture.Wasm32. This makes the offsets inconsistent with the actual encoded layout and risks emitting incorrect runtime function GC-info pointers for Wasm. The personality routine size reservation should match theEncodeDataCorecondition (!= X86 && != Wasm32).
if (factory.Target.Architecture != TargetArchitecture.Wasm32)
{
offset += (-offset & 3); // 4-alignment for the personality routine
}
if (factory.Target.Architecture != TargetArchitecture.X86)
{
offset += sizeof(uint); // personality routine
}
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/MethodGCInfoNode.cs:95
- Same offset/layout mismatch for cold funclets:
CalculateFuncletOffsetsreserves 4 bytes for a personality routine on Wasm, butEncodeDataCoredoesn’t emit one forTargetArchitecture.Wasm32. Adjust the condition here as well so cold funclet offsets match the encoded data.
if (factory.Target.Architecture != TargetArchitecture.Wasm32)
{
offset += (-offset & 3); // 4-alignment for the personality routine
}
if (factory.Target.Architecture != TargetArchitecture.X86)
{
offset += sizeof(uint); // personality routine
}
Comment on lines
+205
to
+212
| string[] parts = rvaSpec.Split(':'); | ||
| if (parts.Length != 2) | ||
| { | ||
| throw new ArgumentException($"Invalid --dump-rva format '{rvaSpec}'. Expected RVA:Size (e.g. 0x1234:0x20)"); | ||
| } | ||
|
|
||
| int rva = Convert.ToInt32(parts[0].Trim(), 16); | ||
| uint size = Convert.ToUInt32(parts[1].Trim(), 16); |
Comment on lines
261
to
266
| _writer.WriteLine("UnwindInfo:"); | ||
| _writer.Write(rtf.UnwindInfo); | ||
| if (_model.Raw && !isWasm) | ||
| if (_model.Raw) | ||
| { | ||
| DumpBytes(rtf.UnwindRVA, (uint)rtf.UnwindInfo.Size); | ||
| } |
Comment on lines
87
to
92
| case Machine.RiscV64: | ||
| return ((RiscV64.Registers)regnum).ToString(); | ||
| case WasmMachine.Wasm32: | ||
| throw new NotImplementedException($"No implementation for machine type {machine}."); | ||
| return "NYI"; | ||
| default: | ||
| throw new NotImplementedException($"No implementation for machine type {machine}."); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Drive-by work