Make all entries in WASM tables SymbolDefinitions - #130920
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 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 WASM object writer to assign and resolve WASM structural indices (function/type/table) via a shared symbol-index table, and extends SuperPMI recording support for getWasmWellKnownGlobals alongside a JIT/EE interface GUID bump.
Changes:
- Replace ad-hoc
_uniqueSymbols/_uniqueSignaturesbookkeeping with per-section symbol index assignment and use it to self-resolve WASM index relocations. - Update well-known WASM global symbol names for
READYTORUNand wire them into default import creation. - Add SuperPMI record support for
GetWasmWellKnownGlobalsand updateJITEEVersionIdentifier.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h | Adds a recGetWasmWellKnownGlobals declaration (currently duplicated in the header). |
| src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs | Centralizes symbol index assignment and uses it for WASM index relocations; updates exports/elements counts to use derived method count. |
| src/coreclr/tools/Common/Compiler/ObjectWriter/ObjectWriter.cs | Makes EmitSymbolDefinition virtual to allow format-specific overrides. |
| src/coreclr/tools/Common/Compiler/DependencyAnalysis/WasmWellKnownGlobalSymbolNode.cs | Switches well-known global import names based on READYTORUN. |
| src/coreclr/inc/jiteeversionguid.h | Updates the JIT/EE interface version GUID. |
And use the symbol definitions for reloc lookups. This creates a single location for all named entries in each section of the module, and makes _uniqueSignatures, _uniqueSymbols, and _methodCount unnecessary.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2b88048c-7bbb-40bb-ae53-e990e71779c0
d2f13bb to
2b84bf9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs:931
- Throwing NotImplementedException for a missing WASM symbol registration is misleading here: this is an internal invariant violation (the writer produced a relocation referencing an unregistered symbol), not an unimplemented feature. Using InvalidOperationException (or InvalidDataException if you consider this a malformed image) will make failures easier to triage and avoids implying feature gaps.
if (!_wasmSymbolManager.TryGetSymbol(reloc.SymbolName, out WasmSymbol symbol))
{
throw new NotImplementedException($"No wasm index registered for symbol '{reloc.SymbolName}' (relocation {reloc.Type}).");
}
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
|
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. |
…eArray to avoid manual casting from IndexSpace to int
…ter-wasm-table-symbols
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/coreclr/tools/Common/Compiler/ObjectWriter/WasmSymbolManager.cs:114
- GetDefinitions currently yields both imports and definitions for the requested index space (it doesn’t filter out Entry.IsImport). Given the method name and the presence of GetImportCount/GetDefinitionCount, this is easy to misread and could lead to subtle bugs if a future caller expects only definitions (e.g., Global/Tag spaces always have imports first). Consider filtering out imports here (or renaming the API / adding a separate GetAllSymbols).
foreach (var entry in _entries.Values)
{
if (entry.IndexSpace == indexSpace)
{
yield return Resolve(entry);
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/WasmR2RAssert.cs:219
- CheckFunctionExports assumes every function-section entry is exported (it requires function export count == definedFunctionCount and then enforces contiguous indices). That makes this test brittle against the existing TODO in WasmObjectWriter about changing export policy (e.g., exporting only public methods) even if index-space ordering remains correct. Suggest validating only that exported function indices are in the defined-function range (>= importedFunctionCount and < importedFunctionCount + definedFunctionCount) and are unique.
if (functionIndices.Count != definedFunctionCount)
{
failures.Add(
$"Found {functionIndices.Count} function exports for {definedFunctionCount} " +
"function-section entries.");
adamperlin
left a comment
There was a problem hiding this comment.
I think this looks good to me! Thanks for this change, the handling of these kinds of indexed symbols looks much cleaner and less error prone in the object writer now.
In the WasmObjectWriter, instead of tracking symbols in different sections using adhoc fields, extract the tracking of symbols and their corresponding indices in physical sections or logical wasm index spaces into a separate class,
WasmSymbolManager.This also allows Wasm index relocations to share a case in ResolveRelocations.
Adds additional testing to ensure relocations are resolved such that the module's imports are first in each logical index space. This ordering is why the SymbolManager "freezes" the imports sections once index space has assigned indices.