[wasm][coreCLR] R2R: VM-side load of a flat webcil composite image#131016
[wasm][coreCLR] R2R: VM-side load of a flat webcil composite image#131016lewing wants to merge 2 commits into
Conversation
|
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. |
|
Tagging subscribers to this area: @agocke |
a884ea1 to
d21c292
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds CoreCLR VM support for loading a flat Webcil-in-Wasm composite ReadyToRun (R2R) image by adjusting a few PE/R2R assumptions (exported RTR header, RVA addressing, component detection routing) and adding a WASM-specific safeguard against double-applying relocations over a shared host-provided buffer.
Changes:
- Teach composite-owner lookup to treat Webcil images as RVA-addressable (alongside mapped PE images).
- Route
IsComponentAssembly()through the active decoder viaDECODER_DISPATCH. - Add a WASM-only guard to avoid reapplying base relocations to a shared Webcil buffer.
- Fall back to
GetReadyToRunHeader()whenRTR_HEADERexport is absent (WASM/Webcil composite).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/vm/nativeimage.cpp | Adds a WASM/Webcil-specific fallback to obtain the R2R header via the decoder when no RTR_HEADER export exists. |
| src/coreclr/vm/readytoruninfo.cpp | Adjusts composite-owner string resolution to treat Webcil as “mapped-like” for RVA addressing. |
| src/coreclr/vm/peimagelayout.inl | Switches IsComponentAssembly() to decoder dispatch for PE/Webcil parity. |
| src/coreclr/vm/peimagelayout.cpp | Adds a WASM-only guard to prevent double relocation of a shared host-probed Webcil buffer. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/coreclr/vm/readytoruninfo.cpp:543
- For Webcil images, ReadyToRun section "VirtualAddress" is still an RVA that must be translated through the Webcil section table (like WebcilDecoder::GetRvaData does). Using GetBase()+virtualAddress assumes an identity RVA->file-offset mapping that WebcilDecoder does not guarantee, and will point into the Webcil header/incorrect data when PointerToRawData != VirtualAddress.
Suggested: keep the direct base+RVA path only for mapped PE images, and use GetRvaData for Webcil.
LPCUTF8 ownerCompositeExecutableName = NULL;
if (pLayout->IsMapped() || pLayout->IsWebcilFormat())
{
// Mapped PE images and (flat) webcil images are RVA-addressable directly from the base:
// webcil is flat-mapped so the R2R section VirtualAddress maps 1:1 onto GetBase()+VA.
d21c292 to
b5705a1
Compare
b5705a1 to
7e433ae
Compare
| #ifdef TARGET_WASM | ||
| // Host-probed webcil R2R images share one in-memory buffer, so the binder can open the same | ||
| // image twice (identity probe + load) and relocate it twice. WASM table-index relocations are | ||
| // additive (index += tableBase), so re-relocating doubles tableBase and corrupts indirect calls. | ||
| // Skip if this base was already relocated. INTERIM: production should relocate the buffer once at | ||
| // the host-probe boundary and skip webcil here; not synchronized (single-threaded wasm runtime). | ||
| typedef SetSHash< TADDR, |
There was a problem hiding this comment.
Resolved. Worth stating plainly: webcil is wasm-only — FEATURE_WEBCIL is set only for CLR_CMAKE_TARGET_BROWSER OR CLR_CMAKE_TARGET_WASI (clrfeatures.cmake), and off-wasm IsWebcilFormat() is hardcoded FALSE. And wasm webcil is flat-mapped by construction — WasmObjectWriter sets PointerToRawData == VirtualAddress for every section:
// Webcil files are flat-mapped ... the pointer to raw data for each section
// is also the same as the virtual address.
pointerToRawData += alignedSectionSize;
virtualAddress += virtualSize; // same increment, same origin
So there is no "general webcil" case, and RvaToOffset(rva) == rva always holds → GetBase() + rva ≡ GetRvaData(rva) for every webcil the VM can see. The relocation loop's GetBase() + rva (pre-existing) is correct; I've added a comment there documenting the flat-mapped invariant, and reworded the readytoruninfo.cpp comment so it no longer implies a portability concern. Kept GetRvaData there as the format-correct idiom (also avoids GetRvaData's rva == 0 NULL behavior in the reloc loop's raw-arithmetic path).
Note
Reply drafted with GitHub Copilot assistance.
crossgen2 emits the wasm startup R2R composite as a flat webcil-in-wasm image, but the R2R/PE-decoder load path assumes a mapped PE (a named RTR_HEADER export and IsMapped() RVA addressing), so the VM cannot locate the R2R header or address sections in a flat webcil composite. Add the minimal OS-neutral (TARGET_WASM / FEATURE_PORTABLE_ENTRYPOINTS) plumbing to load it: - nativeimage.cpp: fall back to the decoder's R2R header when there is no named RTR_HEADER export. - readytoruninfo.cpp: treat flat webcil as RVA-addressable from base. - peimagelayout.inl: route IsComponentAssembly() through the webcil decoder. - peimagelayout.cpp: guard against re-applying base relocations to a host-probed shared buffer (additive wasm table-index relocs would double). Applies to both wasm targets (browser and wasi). Part of dotnet#130524, addresses the composite-loadability open question in dotnet#130519. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2902374a-f352-4c46-bcec-8a35c97733ae
7e433ae to
c21bec4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/coreclr/vm/peimagelayout.cpp:261
- The relocation de-dup set is accessed without synchronization, but the WASM build can be configured with multithreading (e.g., WasmEnableThreads => FEATURE_MULTITHREADING). In that configuration, concurrent loads of the same shared webcil buffer could race on SetSHash mutation, or even both pass the Contains() check and apply relocations twice. Consider taking a lightweight lock when IsWebcilFormat() is true and holding it for the duration of ApplyBaseRelocations so at most one thread can relocate+record a given base at a time.
typedef SetSHash< TADDR,
NoRemoveSHashTraits< NonDacAwareSHashTraits< SetSHashTraits<TADDR> > > > RelocatedWebcilSet;
static RelocatedWebcilSet s_relocatedWebcilBases;
const bool isHostProbedWebcil = IsWebcilFormat();
const TADDR webcilBase = isHostProbedWebcil ? (TADDR)GetBase() : (TADDR)0;
The peimagelayout.inl change routes PEImageLayout::IsComponentAssembly() through DECODER_DISPATCH to WebcilDecoder::IsComponentAssembly(), but that was stubbed to return FALSE, leaving the dispatch inert: readytoruninfo.cpp gates composite R2R load on IsComponentAssembly(), so every webcil composite was reported as non-component. Implement it from the R2R COMPONENT flag, matching the PE decoder path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2902374a-f352-4c46-bcec-8a35c97733ae
|
Added The Note Comment drafted with GitHub Copilot assistance. |
| #ifdef TARGET_WASM | ||
| // Host-probed webcil R2R images share one in-memory buffer, so the binder can open the same | ||
| // image twice (identity probe + load) and relocate it twice. WASM table-index relocations are | ||
| // additive (index += tableBase), so re-relocating doubles tableBase and corrupts indirect calls. | ||
| // Skip if this base was already relocated. INTERIM: production should relocate the buffer once at | ||
| // the host-probe boundary and skip webcil here; not synchronized (single-threaded wasm runtime). | ||
| typedef SetSHash< TADDR, | ||
| NoRemoveSHashTraits< NonDacAwareSHashTraits< SetSHashTraits<TADDR> > > > RelocatedWebcilSet; | ||
| static RelocatedWebcilSet s_relocatedWebcilBases; | ||
| const bool isHostProbedWebcil = IsWebcilFormat(); | ||
| const TADDR webcilBase = isHostProbedWebcil ? (TADDR)GetBase() : (TADDR)0; | ||
| if (isHostProbedWebcil && s_relocatedWebcilBases.Contains(webcilBase)) | ||
| return; | ||
| #endif // TARGET_WASM |
| typedef SetSHash< TADDR, | ||
| NoRemoveSHashTraits< NonDacAwareSHashTraits< SetSHashTraits<TADDR> > > > RelocatedWebcilSet; | ||
| static RelocatedWebcilSet s_relocatedWebcilBases; |
| BOOL IsComponentAssembly() const | ||
| { | ||
| // A webcil composite's component assemblies carry the R2R COMPONENT flag; the flat FALSE | ||
| // default would make PEImageLayout::IsComponentAssembly() (routed here via DECODER_DISPATCH) | ||
| // report every webcil image as non-component, disabling composite R2R load. | ||
| return HasReadyToRunHeader() && (GetReadyToRunHeader()->CoreHeader.Flags & READYTORUN_FLAG_COMPONENT) != 0; | ||
| } |
What
Minimal, OS-neutral VM plumbing so the runtime can load a flat webcil-in-wasm composite R2R image. crossgen2 emits the wasm startup R2R composite as a flat webcil image, but the R2R/PE-decoder load path assumes a mapped PE — a named
RTR_HEADERexport andIsMapped()RVA addressing — so today the VM can't find the R2R header or address sections in a flat webcil composite. Four small touch-points close that gap:vm/nativeimage.cppRTR_HEADERexport → fall back to the decoder's R2R headervm/readytoruninfo.cppIsWebcilFormat()alongsideIsMapped())vm/peimagelayout.inlIsComponentAssembly()through the webcil decoder (DECODER_DISPATCH)vm/peimagelayout.cppGuarded on
TARGET_WASM/FEATURE_PORTABLE_ENTRYPOINTS, so the same code serves browser and wasi from one path.Why
Part of #130524. Addresses the composite-loadability open question in #130519 ("whether the composite container is fully browser-loadable today or needs work"). This is the VM-consumer half — the producer/packaging side stays with #130519.
Feedback wanted — the relocation guard
The one design question in this PR is the
peimagelayout.cppguard. Host-probed webcil R2R images are backed by a single shared in-memory buffer, not a copy-on-write file mapping. The assembly binder can open such an image more than once (identity probe + load) as independentPEImage/layout objects over the same buffer, and each open would re-runApplyBaseRelocationson that memory. WASM table-index relocations are additive (index += tableBase), so a second application doublestableBaseand pushes indirect-call indices out of the function table.This PR guards it with a VM-local set of already-relocated base addresses (
SetSHash<TADDR>). Because the duplicate opens are distinct objects sharing one buffer, a per-PEImageLayout/per-PEImageflag wouldn't dedup them — the state has to key on the shared resource. It is intentionally not synchronized (relies on the current single-threaded wasm runtime) and is marked INTERIM in-code.The cleaner production direction is probably to relocate the host-probed buffer once at the point it's handed to the runtime and skip webcil in
ApplyBaseRelocationsentirely — but that reaches into the host/probe boundary, which is out of scope for this VM-only load change. Guidance on the preferred layer here is the main thing I'm looking for.Scope / out of scope
Out: composite production/packaging (#130519); the CoreLib-R2R execution gates — QCall/PInvoke, class-init, etc. (#129850); SIMD instruction-set reporting; the host-side merge/probe mechanism.
Testing
./build.sh clr -os wasi -c Release(theTARGET_WASMbranches are only exercised by a wasm build).System.Private.CoreLibR2R image (~49k R2R functions) loads and dispatches through exactly these four changes.Note
This PR (code and description) was drafted with GitHub Copilot assistance.