[crossgen2][wasm] Shrink Code Section Relocs - #132029
Conversation
…e section to allow relocation shrinkage
…r out of hot ResolveReloc function
…rom use of SectionWriter stream in ParseCodeBlobs
|
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. |
There was a problem hiding this comment.
Pull request overview
This PR updates the CoreCLR Wasm object writer to support shrinking variable-length relocations in the Wasm code section, reducing output size by rewriting padded LEB relocations to their minimal encoded lengths and updating code-blob length prefixes accordingly.
Changes:
- Refactors relocation resolution to support a source→destination stream flow, enabling in-place rewriting for shrinkable code blobs.
- Adds code-section-specific parsing and rewriting of length-prefixed code blobs to shrink variable-length relocations and update blob sizes.
- Introduces helper APIs for stream-based ULEB128 decoding and variable-length relocation writing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs | Adds code-section relocation shrinking logic and refactors relocation resolution to support rewriting into a destination stream. |
| src/coreclr/tools/Common/Compiler/ObjectWriter/Dwarf/DwarfHelper.cs | Adds a stream-based ULEB128 reader used by code-blob parsing. |
| src/coreclr/tools/Common/Compiler/DependencyAnalysis/Relocation.cs | Adds helpers to detect variable-length Wasm relocations and write minimally-sized LEB encodings. |
Suppressed comments (1)
src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs:734
- Unused local
sectionStreamis created but never used; this is dead code and will trigger an unnecessary-using/unused-variable warning. Remove it (the relocation resolution already copies fromsection.StreamintowebcilStream).
MemoryStream sectionStream = new MemoryStream((int)section.Stream.Length);
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 29e6097e-e610-430f-8ab6-68555341eb21
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs:886
- ParseCodeBlobs relies on a Debug.Assert to ensure the decoded blob size fits in the remaining stream, but in Release builds this can advance the position past the end of the stream and later cause out-of-range copies/writes with a much less actionable exception. Consider validating and throwing an InvalidDataException when the decoded size would exceed the remaining bytes.
Debug.Assert(sectionStream.Position + (long)decoded <= sectionStream.Length);
blobs.Add(new CodeBlob((long)decoded, sectionStream.Position, sectionStream.Position + (long)decoded));
sectionStream.Position += (long)decoded;
src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs:983
- ResolveCodeRelocations writes new blob-length prefixes into a fixed 5-byte buffer via WriteULEB128. If a blob size ever exceeds what fits in 5 bytes, WriteULEB128 will overrun the span and throw an IndexOutOfRangeException. Adding an explicit size check lets us fail fast with a clear error (and avoids writing invalid Wasm).
// Write the temp stream back into the original stream with a NEW length prefix, starting at writeCursor
DwarfHelper.WriteULEB128(countBuffer, (ulong)tempStream.Length);
sectionStream.Position = writeCursor;
sectionStream.Write(countBuffer, 0, (int)DwarfHelper.SizeOfULEB128((ulong)tempStream.Length));
writeCursor = sectionStream.Position; // set writeCursor to the position after the length prefix we just wrote
src/coreclr/tools/Common/Compiler/DependencyAnalysis/Relocation.cs:798
- Relocation.ActualSize casts resolvedValue to ulong for ULEB relocations without validating non-negativity. If a negative value is ever passed (e.g., via an unexpected addend), this will compute a bogus size instead of failing fast. Consider rejecting negative values and using a checked cast for the ULEB128 size calculation.
return (int)DwarfHelper.SizeOfULEB128((ulong)resolvedValue);
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 900aa3dd-9aca-42b6-afcb-a6c8146a6c61
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs:885
- ParseCodeBlobs relies on Debug.Assert to ensure the decoded blob size fits in the remaining stream. In Release builds this becomes unchecked and can lead to out-of-range span operations later. Consider throwing InvalidDataException when the section content is malformed/truncated.
ulong? decoded = DwarfHelper.ReadULEB128(sectionStream, out _);
if (decoded is null) break; // end of stream
Debug.Assert(sectionStream.Position + (long)decoded <= sectionStream.Length);
blobs.Add(new CodeBlob((long)decoded, sectionStream.Position, sectionStream.Position + (long)decoded));
src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs:1214
- ResolveRelocations copies from sectionStream's current Position without resetting it, but then assumes it copied the full section (uses sectionStream.Length) when restoring dstStream.Position. This is fragile if any future caller forgets to seek to 0. Reset the source position and track endPos from the CopyTo result.
// Otherwise, we can resolve relocations on top of the copied in section stream, since the size and layout of the stream won't be changing.
long startPos = dstStream.Position;
sectionStream.CopyTo(dstStream);
for (int i = 0; i < relocs.Count; i++)
src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs:872
- CopyOnly accepts long offsets/counts but unconditionally casts to int for AsSpan. If any caller ever passes offsets/counts > int.MaxValue (or negative), this will overflow and either throw from AsSpan or copy the wrong region. Add explicit range validation and checked casts so failures are deterministic and easier to diagnose.
This issue also appears on line 881 of the same file.
static void CopyOnly(MemoryStream src, long srcPos, MemoryStream dest, long destPos, long count)
{
ArgumentOutOfRangeException.ThrowIfNegative(count);
src.GetBuffer().AsSpan((int)srcPos, (int)count).CopyTo(dest.GetBuffer().AsSpan((int)destPos, (int)count));
}
src/coreclr/tools/Common/Compiler/ObjectWriter/Dwarf/DwarfHelper.cs:158
- ReadULEB128(Stream) has no upper bound on the number of bytes or on the shift count. For malformed data with a long continuation chain, the shift will eventually wrap (C# masks shift counts), producing an incorrect value instead of failing fast. Add a max-byte/shift guard and throw InvalidDataException when exceeded.
while (true)
{
int b = source.ReadByte();
if (b < 0)
{
src/coreclr/tools/Common/Compiler/DependencyAnalysis/Relocation.cs:799
- Relocation.ActualSize casts resolvedValue to ulong for ULEB128 relocs without checked/validation. If a negative resolvedValue ever slips through, this will compute a bogus size rather than failing consistently (WriteValue uses checked for the same cases). Use checked((ulong)resolvedValue) here as well.
case RelocType.WASM_MEMORY_ADDR_REL_LEB:
case RelocType.WASM_CLR_RESTORE_CONTEXT_EXCEPTION_TAG_LEB:
return (int)DwarfHelper.SizeOfULEB128((ulong)resolvedValue);
case RelocType.WASM_TABLE_INDEX_SLEB:
src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs:986
- After rewriting blobs, ResolveCodeRelocations doesn't verify that all relocations were consumed. If any relocation falls outside parsed blob payload ranges, it will be silently skipped. Add a post-condition assert to catch this assumption early.
}
sectionStream.SetLength(writeCursor);
|
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. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs:735
- The comment here suggests relocation offsets in the list need to be recalculated based on the section’s position, but
reloc.Offsetis still section-relative (seePendingBaseReloc/BuildBaseRelocMapusingVirtualAddress + Offset). The actual adjustment is applied viasectionStart + reloc.Offsetwhen reading/writing the combined stream. Rewording avoids confusion about theOffsetsemantics.
// We emit all Webcil sections into one stream, and copy data / resolve relocations directly into this combined stream.
// As a result, the real offsets that relocs in our list have need to be calculated based on the section's
// position within the Webcil segment
src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs:901
throw new InvalidDataException();is non-actionable when hit. Since this is a reachable validation failure, include a message that explains what was wrong (e.g., relocations present but no parsable code blobs) and which section triggered it.
if (blobs.Count == 0 && relocs.Count > 0)
{
throw new InvalidDataException();
}
src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs:871
- CoreCLR toolchain code generally avoids
recordtypes in low-level compiler/tooling code (see core-runtime instructions).record structsynthesizes extra members and equality semantics that aren’t needed here. Prefer areadonly structwith explicit properties/ctor.
private readonly record struct CodeBlob(long Size, long Start, long End);
src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs:903
- CoreCLR tooling prefers avoiding LINQ in low-level compiler code paths (allocations + harder-to-debug). This
Maxcan be a simple loop without allocations.
long maxBlobSize = blobs.Max(blob => blob.End - blob.Start);
src/coreclr/tools/Common/Compiler/DependencyAnalysis/Relocation.cs:789
ActualSizeis introduced as a new public API surface but appears unused (no references in the compiler/tools code). If it’s not needed immediately, consider removing it to avoid carrying dead code/API; otherwise, add a caller to justify it.
public static int ActualSize(RelocType relocType, long resolvedValue)
{
Debug.Assert(IsVariableLength(relocType));
|
@jtschuster could you take a look at this one? I'm hoping this won't be too hard to integrate in with your refactoring changes since it is pretty scoped to just reloc resolution. |
jtschuster
left a comment
There was a problem hiding this comment.
LGTM, and it should be easy to rebase my changes onto this.
AndyAyersMS
left a comment
There was a problem hiding this comment.
Are you planning to revert #131369 as a follow-up? That should save a bit more size.
Yes I was, thank you for the reminder. |
This PR implements a relocation shrinking optimization in the WasmObjectWriter. All relocations in Wasm code emitted by the JIT are currently padded to the max 32-bit ULEB length of 5 bytes, but their resolved values may be much shorter. This PR keeps behavior the same for non-code sections which don't have variable length relocs, while implementing a shrinking procedure to re-write each relocation in the code section to its minimal length. The LEB lengths of each code blob are updated accordingly after shrinkage.
Impact
This yields around 5% size savings for System.Private.CoreLib.