Skip to content

Make all entries in WASM tables SymbolDefinitions - #130920

Merged
jtschuster merged 17 commits into
dotnet:mainfrom
jtschuster:jtschuster-wasm-table-symbols
Jul 31, 2026
Merged

Make all entries in WASM tables SymbolDefinitions#130920
jtschuster merged 17 commits into
dotnet:mainfrom
jtschuster:jtschuster-wasm-table-symbols

Conversation

@jtschuster

@jtschuster jtschuster commented Jul 16, 2026

Copy link
Copy Markdown
Member

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.

Copilot AI review requested due to automatic review settings July 16, 2026 20:07
@github-actions github-actions Bot added the area-crossgen2-coreclr only use for closed issues label Jul 16, 2026
@azure-pipelines

Copy link
Copy Markdown
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/_uniqueSignatures bookkeeping with per-section symbol index assignment and use it to self-resolve WASM index relocations.
  • Update well-known WASM global symbol names for READYTORUN and wire them into default import creation.
  • Add SuperPMI record support for GetWasmWellKnownGlobals and update JITEEVersionIdentifier.

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.

Comment thread src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h Outdated
Comment thread src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs
Comment thread src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs Outdated
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
Copilot AI review requested due to automatic review settings July 16, 2026 20:34
@jtschuster
jtschuster force-pushed the jtschuster-wasm-table-symbols branch from d2f13bb to 2b84bf9 Compare July 16, 2026 20:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment thread src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs
Comment thread src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs Outdated
Copilot AI review requested due to automatic review settings July 16, 2026 23:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment thread src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs Outdated
Comment thread src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs Outdated
Copilot AI review requested due to automatic review settings July 21, 2026 22:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}).");
                            }

Copilot AI review requested due to automatic review settings July 21, 2026 23:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 24, 2026 18:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

@jtschuster jtschuster added arch-wasm WebAssembly architecture area-ReadyToRun and removed area-crossgen2-coreclr only use for closed issues labels Jul 24, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara
See info in area-owners.md if you want to be subscribed.

Copilot AI review requested due to automatic review settings July 24, 2026 21:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

@jtschuster
jtschuster marked this pull request as ready for review July 24, 2026 22:38
@azure-pipelines

Copy link
Copy Markdown
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.

Comment thread src/coreclr/tools/Common/Compiler/ObjectWriter/WasmSymbolManager.cs Outdated
Copilot AI review requested due to automatic review settings July 30, 2026 18:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 adamperlin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jtschuster
jtschuster merged commit d3764e0 into dotnet:main Jul 31, 2026
106 of 108 checks passed
@github-project-automation github-project-automation Bot moved this to Done in AppModel Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arch-wasm WebAssembly architecture area-ReadyToRun

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants