Skip to content

Conversation

@adamperlin
Copy link
Contributor

@adamperlin adamperlin commented Jan 28, 2026

This PR adds support for generating an import section to the WasmObjectWriter. We currently import:

  • env.memory
  • __stack_pointer
  • __r2r_start
    Which are all expected to be provided by the runtime. Various data segments are then placed into __r2r_start + <segment_offset>. We use the constant expressions extension for these address calculations.

Notably this PR removes our export of the module memory, as we want to share memory with the runtime.

Lay out R2R data segments relative to imported __r2r_start global
@adamperlin adamperlin requested a review from kg January 28, 2026 23:53
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 28, 2026
@kg
Copy link
Member

kg commented Jan 28, 2026

I will try and update the test harness to be compatible with this

@dotnet-policy-service
Copy link
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 January 29, 2026 23:38
@adamperlin adamperlin marked this pull request as ready for review January 29, 2026 23:42
@adamperlin
Copy link
Contributor Author

@kg I'd love a review on this when you have a chance!

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds initial support for emitting a Wasm import section from WasmObjectWriter, and switches R2R data segment placement to use constant expressions based on an imported __r2r_start symbol.

Changes:

  • Introduces a new wasm.import object node section and emits it as Wasm section type Import.
  • Reworks combined data segment placement to use an instruction expression (global.get __r2r_start + i32.const offset + i32.add) instead of a fixed DataStartOffset.
  • Adds a small Wasm encoding model (imports, globals, memory type, expression/instruction encoding) and a helper to write UTF-8 strings with a length prefix.

Reviewed changes

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

File Description
src/coreclr/tools/Common/Compiler/ObjectWriter/WasmObjectWriter.cs Adds import section emission and uses __r2r_start + offset constant expressions for data segment placement.
src/coreclr/tools/Common/Compiler/ObjectWriter/WasmNative.cs Adds encodable abstractions/types for Wasm expressions and import encodings (memory/global types, etc.).
src/coreclr/tools/Common/Compiler/ObjectWriter/SectionWriter.cs Adds WriteUtf8WithLength helper for Wasm-style length-prefixed strings.

Copy link
Contributor

Copilot AI left a comment

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.

@kg
Copy link
Member

kg commented Jan 30, 2026

This looks fairly good, all of Copilot's feedback seemed accurate though so we need to address it.
The Encode/EncodeSize split is a little awkward but I don't see an obvious way to get rid of it other than doing something weird with empty spans.

@adamperlin
Copy link
Contributor Author

This looks fairly good, all of Copilot's feedback seemed accurate though so we need to address it. The Encode/EncodeSize split is a little awkward but I don't see an obvious way to get rid of it other than doing something weird with empty spans.

Agreed, I'd prefer to get rid of that split if we can find a clean way to do it. Copilot already caught a bug introduced because of this. Honestly, a solution using empty spans to calculate the exact length before writing might be preferrable for ensuring correctness.

Copilot AI review requested due to automatic review settings January 30, 2026 20:07
Copy link
Contributor

Copilot AI left a comment

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 5 out of 5 changed files in this pull request and generated 4 comments.

Copilot AI review requested due to automatic review settings January 30, 2026 21:37
@adamperlin adamperlin force-pushed the adamperlin/wasm-object-writer-imports branch from e3da59a to 1d3f7fc Compare January 30, 2026 21:37
Copy link
Contributor

Copilot AI left a comment

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 5 out of 5 changed files in this pull request and generated 5 comments.

Copy link
Contributor

Copilot AI left a comment

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.

@adamperlin
Copy link
Contributor Author

adamperlin commented Jan 30, 2026

This looks fairly good, all of Copilot's feedback seemed accurate though so we need to address it. The Encode/EncodeSize split is a little awkward but I don't see an obvious way to get rid of it other than doing something weird with empty spans.

Agreed, I'd prefer to get rid of that split if we can find a clean way to do it. Copilot already caught a bug introduced because of this. Honestly, a solution using empty spans to calculate the exact length before writing might be preferrable for ensuring correctness.

I think I'll address this as a refactor in a follow up. My thinking here is to create a lightweight wrapper type that can be consumed as a ref struct by all the Encode methods instead of consuming the spans directly, which is capable of either counting bytes that would be written or writing into a target span depending on if its internal span is empty or not.

@adamperlin
Copy link
Contributor Author

@kg This should be ready for another look when you have a moment!

Copy link
Contributor

Copilot AI left a comment

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 5 out of 5 changed files in this pull request and generated 6 comments.

Comment on lines +7 to +10
// This namespace implements encodings for certain Wasm expressions (instructions)
// which are used in the object writer.
// For now, these instructions are only used for constructing constant expressions
// to calculate placements for data segments based on imported globals.
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

There appears to be trailing whitespace in the comment lines here (e.g., after the period). The repo enforces trim_trailing_whitespace = true (.editorconfig:13), so please remove trailing spaces to avoid formatting-only diffs and potential CI/style checks.

Copilot uses AI. Check for mistakes.
Comment on lines +333 to +342
private void WriteImports()
{
// Calculate the minimum required memory size based on the combined data section size
ulong contentSize = (ulong)SectionByName(WasmObjectNodeSection.CombinedDataSection.Name).ContentSize;
uint dataPages = checked((uint)((contentSize + (1<<16) - 1) >> 16));
uint numPages = Math.Max(dataPages, 1); // Ensure at least one page is allocated for the minimum

_defaultImports[0] = new WasmImport("env", "memory", WasmExternalKind.Memory,
new WasmMemoryType(WasmLimitType.HasMin, numPages)); // memory limits: flags (0 = only minimum)

Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

The PR description says the new import section currently imports __stack_pointer and __r2r_start, but the implementation also changes the module to import env.memory (and removes the previously emitted memory section/export). Please update the PR description (or the code) so reviewers/consumers understand this behavioral/interface change.

Copilot uses AI. Check for mistakes.
}

private void WriteExports()
{
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

WriteExports() no longer exports the module's linear memory (previously exported as "memory"). If any downstream expects to access the instance memory via an export (common for JS tooling/debugging), this is a breaking interface change. Consider exporting the imported memory as well, or document the change and ensure consumers are updated.

Suggested change
{
{
WriteMemoryExport("memory", 0);

Copilot uses AI. Check for mistakes.
Comment on lines +215 to +217
WasmValueType ValueType;
WasmMutabilityType Mutability;

Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

These are private fields but don't follow the repo naming rule for private/internal fields (_camelCase per .editorconfig:76-83). Please rename to _valueType / _mutability (and similarly for other new private fields below) to match existing conventions.

Copilot uses AI. Check for mistakes.
Comment on lines +242 to +245
WasmLimitType LimitType;
uint Min;
uint? Max;

Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

Private fields LimitType / Min / Max don't follow the repo's _camelCase convention for private/internal fields (.editorconfig:76-83). Renaming to _limitType / _min / _max would keep this consistent with the rest of the file (e.g., _params, _returns).

Copilot uses AI. Check for mistakes.
Comment on lines +300 to +301

#nullable disable
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

#nullable disable is placed inside WasmImport and (as written) disables nullability for the remainder of the file without an obvious reason. This can hide nullability warnings in future edits; consider removing it or scoping it more explicitly (e.g., disable/restore around the specific code that needs it).

Suggested change
#nullable disable

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arch-wasm WebAssembly architecture area-crossgen2-coreclr

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants