Uh oh!
There was an error while loading. Please reload this page.
Make wasm composite ReadyToRun images inspectable with R2RDump - #132650
Open
lewing wants to merge 4 commits into
Open
Make wasm composite ReadyToRun images inspectable with R2RDump#132650lewing wants to merge 4 commits into
lewing wants to merge 4 commits into
Conversation
R2RDump could not read a wasm/webcil composite R2R image, and crossgen2 --map crashed on one, leaving no output image at all. R2RDump: WebcilImageReader.TryGetReadyToRunHeader required CorFlags.ILLibrary before consulting the CLI header's ManagedNativeHeader directory, and always reported isComposite=false. A composite's COR header has no ILLibrary flag - CopiedCorHeaderNode only ORs it in on the _module != null path - so the header was never found and R2RDump threw "The file is not a ReadyToRun image". Webcil has no export table, so unlike PE a composite cannot publish an RTR_HEADER export; both single-assembly and composite webcil images locate the header through ManagedNativeHeader, which is what WebcilDecoder::FindReadyToRunHeader already does in the runtime. Locate the header the same way and derive isComposite from the absence of ILLibrary, matching PE's polarity. R2RDump: NativeCuckooFilter validated 16 byte alignment of the file offsets of the AttributePresence section. The runtime validates the section's RVA and size instead (NativeCuckooFilter in nativeformatreader.h). The distinction is latent on PE, where a file offset is congruent to its RVA modulo 16 because section and file alignment are both multiples of 16, but a webcil payload embedded in a wasm wrapper does not start at a 16 byte aligned file offset, so every file offset is shifted and the check failed on a correctly aligned image. Validate the RVA and the filter length instead, which is exactly equivalent to the previous check on PE and correct for webcil. The RVA is taken as a constructor parameter so the invariant stays with the type that depends on it. The runtime additionally requires the filter size to be a power of two; that is deliberately not ported, since refusing to dump an otherwise readable filter is less useful than dumping it. crossgen2: WebCilObjectWriter never populated the output section layout, unlike the PE, COFF, ELF and Mach-O writers. MapFileBuilder indexes that list by section index, so --map threw ArgumentOutOfRangeException and, because ReadyToRunObjectWriter deletes the output on failure, no image was emitted. Record one entry per section so --map and image emission can coexist. Section lengths come from the stream length rather than the padded on-disk size, as in the other writers. ComputeChecksums is overridden to reject checksum relocations, which nothing emits for wasm today: the recorded file positions are relative to the webcil segment rather than to the output file, so the base implementation would patch the checksum into the wrong place. Adds two regression tests: WasmCompositeModule compiles a wasm composite and asserts the reader recognizes it, and CuckooFilterAlignmentIsValidatedAgainstTheRva covers a filter whose RVA is aligned but whose file offset is not. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enables inspection and map generation for WebAssembly composite ReadyToRun images.
Changes:
- Supports composite Webcil ReadyToRun header discovery.
- Corrects Cuckoo filter alignment validation.
- Records Webcil output layouts and adds regression coverage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Review summary |
|---|---|
src/coreclr/tools/r2rdump/TextDumper.cs | Passes the filter RVA for validation. |
src/coreclr/tools/Common/Compiler/ObjectWriter/WebCilObjectWriter.cs | Adds layout and checksum handling. Moderate concerns remain about map offsets and stale post-shrink node locations (2 and 3 votes). |
src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/WebcilImageReader.cs | Supports composite Webcil header discovery. |
src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/NativeHashtable.cs | Updates Cuckoo filter validation. Moderate concern remains about the unapproved public constructor change (3 votes). |
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCases/R2RTestSuites.cs | Adds composite and alignment tests; nit: the test does not pass --map to exercise the affected path (3 votes). |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
lewing added a commit
to lewing/runtime
that referenced
this pull request
Aug 26, 2026
Wasm composite emission is not deterministic: two runs of the same crossgen2 binary, same mode, over the same inputs produce images that differ. The repo already treats this as a bug elsewhere -- src/tests/readytorun/determinism byte-compares crossgen2 output and fails on any difference -- so wasm composite violates an invariant that is explicitly tested on other targets. The consequence is what matters day to day: a composite cannot be reproduced from its recipe, only replaced. Anything derived from one -- offsets, indices, token-to-index bindings -- is bound to that exact file rather than to the command that produced it, so derived constants must carry the image's sha256sum. Rebuilding "the same" image and reusing an old constant is a silent way to be wrong, and was very nearly one here. Also record that R2RDump rejects a wasm composite outright until dotnet#132650 merges. Its error names the image, so it reads as a malformed artifact rather than a reader limitation, and that misreading has already cost time. Note the -r glob trap in the same place: a glob expands to one match and drops the rest with a message that is easy to miss, surfacing later as an unrelated-looking decode failure. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2b684576-6420-4809-bf5b-d4d12072ef97
lewing added a commit
to lewing/runtime
that referenced
this pull request
Aug 26, 2026
The previous entry stated R2RDump's composite failure as "broken until dotnet#132650 merges", which tied a durable fact about the artifact to the fate of a specific PR. Those have different lifetimes: the diagnosis -- the image is valid and the reader checks the wrong coordinate space -- rests on properties of the composite itself and holds regardless of how the reader is fixed. Only the remedy can go stale. State them separately, and give the diagnosis in falsifiable form: four facts a reader can check against any composite by parsing the CLI header, without building anything. Correctly aligned AttributePresence RVA, power-of-two size satisfying the nativeformatreader.h check R2RDump omits, a valid RTR header, and a webcil payload at a file offset that is not 16-byte aligned -- which is the entire reason a file-offset check fails on an image whose RVAs are fine. A reader who doubts the entry can now settle it rather than trust it. Name the shared category while both examples are adjacent: this and the -r glob trap are both messages that point away from their cause, so the time is lost downstream, investigating whatever the message named. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2b684576-6420-4809-bf5b-d4d12072ef97
The equivalent support has landed on main with correct post-shrink offsets. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 827f138b-e780-418d-933b-5171054acad8
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 827f138b-e780-418d-933b-5171054acad8
Validate the filter RVA at the r2rdump call site while retaining size validation in the existing public constructor. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 827f138b-e780-418d-933b-5171054acad8
Contributor
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
lewing
marked this pull request as ready for review
August 28, 2026 23:30
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
| // matters for a Webcil image embedded at an unaligned WASM offset. | ||
| if ((section.RelativeVirtualAddress & 0xF) != 0) | ||
| { | ||
| throw new BadImageFormatException(); |
Comment on lines
+259
to
262
| if (((_filterEndOffset - _filterStartOffset) & 0xF) != 0) | ||
| { | ||
| // Native cuckoo filters must be aligned at 16byte boundaries within the PE file | ||
| throw new System.BadImageFormatException(); | ||
| } |
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 freeto 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.
Wasm composite ReadyToRun images could not be inspected with
R2RDump. The reader rejected a composite before reaching its ReadyToRun header, and fixing that exposed an alignment check that treated an embedded Webcil file offset as though it were an RVA.Changes
ManagedNativeHeaderdirectory. Webcil has no export table for theRTR_HEADERmechanism used by PE composites.isCompositefrom the absence ofCorFlags.ILLibrary, matching the existing PE polarity.NativeCuckooFilterconstructor is preserved.Scope
The original PR also addressed a
crossgen2 --mapfailure. That support has since landed in #132693 with correct post-shrink wasm offsets, so this PR now leavesWebCilObjectWriterunchanged and focuses only on R2RDump inspection.Validation
./build.sh clr+libs+host./build.sh -s clr.r2rtests -lc Debug -c Debug -testThe browser-wasm test leg could not be run locally because the configured Emscripten toolchain selected Xcode Python 3.9, while the installed Emscripten requires Python 3.10 or newer.
Note
This pull request description was generated with GitHub Copilot.