Skip to content

Transpiler: support structs with fixed-size array fields (Option A and B)#544

Merged
jonathanpeppers merged 4 commits into
mainfrom
copilot/add-transpiler-support-structs
May 28, 2026
Merged

Transpiler: support structs with fixed-size array fields (Option A and B)#544
jonathanpeppers merged 4 commits into
mainfrom
copilot/add-transpiler-support-structs

Conversation

Copilot AI commented May 24, 2026

Copy link
Copy Markdown
Contributor

Adds transpiler support for value-type struct fields that hold a fixed-size byte array, via either C# fixed byte buf[N] (Option A, unsafe) or [InlineArray(N)] (Option B). Enables porting C code like From Below's struct cluster { ... unsigned char layout[4]; ... } without resorting to structure-of-arrays.

Changes

  • Struct layout detection (Transpiler.StructAnalysis.cs): new TryGetBufferFieldSize reads the [FixedBuffer] attribute, the field's value-type [InlineArray(N)] attribute, or the explicit ClassLayout of Roslyn's <name>e__FixedBuffer helper. Buffer fields contribute their full byte count to the struct layout so following fields land at the correct offset.
  • Ldflda lowering (IL2NESWriter.StructFields.cs): pattern-matches the full buffer-access sub-sequence (fixed buffer: outer + inner ldflda; inline array: InlineArrayElementRef/InlineArrayFirstElementRef; inline array runtime index: InlineArrayAsSpan + Span<>.get_Item) through the matching stind.i1/ldind.u1 and emits one LDA/STA at base + fieldOffset + indexAbsolute for constant indices, AbsoluteX (with LDX) for runtime indices.
  • Skip mechanism (Transpiler.cs, IL2NESWriter.cs): new SkipUntilIndex lets a single handler consume a multi-instruction IL pattern; both main and user-method dispatch loops honor it.
  • Initobj dispatched as a no-op so T cur = default; lowers correctly (consistent with existing semantics that treat struct locals as uninitialized zero-page bytes the user code fills in).
  • Parser fixes (Transpiler.ILParsing.cs): skip synthesized declaring types like <PrivateImplementationDetails> so Roslyn's emitted InlineArrayElementRef/InlineArrayAsSpan helpers aren't treated as user methods; resolve MethodSpecification whose .Method is a MethodDefinition (those helpers).
  • Tests: 4 new RoslynTests in StructsTests covering both options × {constant, runtime} indices.
  • Docs (docs/8bitworkshop-samples.md): drop "no struct support" notes; mention the new buffer-field capabilities.

Example

Both lower identically to direct zero-page LDA/STA (Absolute for [0], AbsoluteX for [i]):

// Option A
unsafe struct Cluster {
    public byte sprite;
    public byte id;
    public fixed byte layout[4];
}

// Option B
[System.Runtime.CompilerServices.InlineArray(4)]
struct Bytes4 { private byte _element0; }
struct Cluster2 { public byte sprite; public byte id; public Bytes4 layout; }

Cluster cur;
cur.layout[0] = 5;     // STA $base+2
cur.layout[i] = 99;    // LDX i; STA $base+2,X

Deferred (stretch items called out in the issue)

  • Passing a struct ref to a user method without copying (call-convention work).
  • End-to-end Mesen2 sample under samples/.
  • const 2D inline-array initializer inside a struct.

Copilot AI changed the title [WIP] Add transpiler support for structs with fixed-size array fields Transpiler: support structs with fixed-size array fields (Option A and B) May 24, 2026
Copilot AI requested a review from jonathanpeppers May 24, 2026 23:30
@jonathanpeppers jonathanpeppers marked this pull request as ready for review May 26, 2026 20:41
Copilot AI review requested due to automatic review settings May 26, 2026 20:41

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 extends dotnes’s IL→6502 pipeline to support struct fields that represent fixed-size byte buffers, enabling C# ports of C-style structs with inline byte arrays via either fixed byte buf[N] or [InlineArray(N)].

Changes:

  • Adds struct-layout analysis for fixed buffers / inline arrays and exposes buffer-field byte sizes to the IL writer.
  • Implements ldflda lowering that pattern-matches buffer element reads/writes through stind.i1 / ldind.u1, emitting a single LDA/STA using Absolute or AbsoluteX.
  • Adds a skip mechanism (SkipUntilIndex) to let a handler consume multi-instruction IL patterns, plus new Roslyn tests and updated docs.

Reviewed changes

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

Show a summary per file
File Description
src/dotnes.tests/StructsTests.cs Adds Roslyn tests for fixed-buffer and inline-array constant/runtime indexing.
src/dotnes.tasks/Utilities/Transpiler.StructAnalysis.cs Detects buffer-field byte sizes from metadata (FixedBuffer, InlineArray, layout size) and incorporates them into struct sizing.
src/dotnes.tasks/Utilities/Transpiler.ILParsing.cs Skips synthesized helper methods (e.g., inline-array helpers) and improves parsing of some MethodSpecification cases.
src/dotnes.tasks/Utilities/Transpiler.cs Plumbs BufferFieldSizes into writers and adds SkipUntilIndex handling to the main/user-method IL dispatch loops.
src/dotnes.tasks/Utilities/IL2NESWriter.StructFields.cs Implements HandleLdflda buffer access lowering + helpers to extract index/value and emit Absolute/AbsoluteX.
src/dotnes.tasks/Utilities/IL2NESWriter.ILDispatch.cs Dispatches Ldflda and treats Initobj as a no-op for struct locals.
src/dotnes.tasks/Utilities/IL2NESWriter.cs Adds BufferFieldSizes and SkipUntilIndex to support multi-instruction lowering.
docs/8bitworkshop-samples.md Updates docs to reflect the new struct buffer-field capabilities and current limitations.
Comments suppressed due to low confidence (1)

src/dotnes.tasks/Utilities/Transpiler.cs:411

  • There are two statements on one line here (var labelName = ...; if (...)), which hurts readability and makes future diffs noisier. Please split this into separate lines.
                var labelName = $"{methodName}_instruction_{instruction.Offset:X2}";                if (methodWriter.CurrentBlock != null)
                    methodWriter.CurrentBlock.SetNextLabel(labelName);

Comment thread src/dotnes.tasks/Utilities/IL2NESWriter.StructFields.cs Outdated
Comment thread src/dotnes.tasks/Utilities/IL2NESWriter.StructFields.cs Outdated
Comment thread src/dotnes.tasks/Utilities/IL2NESWriter.StructFields.cs
Comment thread src/dotnes.tasks/Utilities/IL2NESWriter.StructFields.cs Outdated
Comment thread src/dotnes.tasks/Utilities/Transpiler.StructAnalysis.cs
Comment thread src/dotnes.tasks/Utilities/Transpiler.StructAnalysis.cs
Comment thread src/dotnes.tests/StructsTests.cs Outdated
- Key BufferFieldSizes by (structType, fieldName) tuple so same-named buffer fields in distinct structs no longer collide.
- Validate FixedBuffer element type is System.Byte; reject other element types explicitly instead of silently producing wrong layouts.
- Reject arithmetic in buffer index expressions (e.g. buf[i + 1]) by counting Add opcodes during the buffer-access scan.
- Throw when the index expression cannot be recognised instead of falling back to index 0.
- Remove dead storeValueRuntime flag/out param; storeValueConst already handles the only supported case.
- EmitLoadIndexIntoX: reject ldarg-sourced runtime indices outright instead of silently aliasing them to a same-numbered local.
- Tests: clarify wording (absolute RAM addresses, not zero-page).
- Transpiler.cs: split two statements that were stuck on one line.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@jonathanpeppers

Copy link
Copy Markdown
Owner

/review

@github-actions

github-actions Bot commented May 28, 2026

Copy link
Copy Markdown

dotnes PR Reviewer completed successfully!

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

Verdict: ⚠️ Needs Changes (1 error, 1 warning, 2 suggestions)

CI: ✅ All checks passing (build, smoke-test macOS, smoke-test Windows).

What this PR does

Adds transpiler support for struct buffer fields via C# fixed byte buf[N] (Option A) and [InlineArray(N)] (Option B). The implementation pattern-matches multi-instruction IL sequences (ldflda through stind.i1/ldind.u1) and emits direct LDA/STA at computed absolute addresses. Good defensive error handling with explicit TranspileException for unsupported patterns.

Issues

Sev Category Summary
Transpiler correctness Buffer field stores only work with constant values — cur.layout[0] = someVariable will fail at transpile time
⚠️ Error handling Bare catch {} in GetInlineArrayLength swallows all exceptions silently
💡 Testing Single-byte hex assertions ("8D", "AE") are too broad to catch offset bugs
💡 Code organization Skip-loop logic duplicated between main and user-method dispatch

Positives

  • Thorough defensive coding — explicit TranspileException for every unsupported pattern rather than silent miscompilation
  • Clean separation into StructAnalysis (metadata) and StructFields (emission)
  • Good test coverage of both Option A and B × constant/runtime index combinations
  • Well-documented IL pattern shapes in comments
  • Parser fixes for <PrivateImplementationDetails> and MethodSpecification resolution are correct and targeted

Generated by dotnes PR Reviewer for issue #544 · ● 17.2M

Comment thread src/dotnes.tasks/Utilities/IL2NESWriter.StructFields.cs
Comment thread src/dotnes.tasks/Utilities/IL2NESWriter.StructFields.cs Outdated
Comment thread src/dotnes.tasks/Utilities/Transpiler.StructAnalysis.cs
Comment thread src/dotnes.tasks/Utilities/Transpiler.cs Outdated
- Rewrite HandleLdflda in IL2NESWriter.StructFields.cs to properly
  classify the various IL patterns (fixed buffer with/without add,
  inline array first-element ref, inline array element ref, Span path
  for runtime indexes). This also fixes a pre-existing bug where
  `cur.layout[0] = const` wrote the value to the wrong address
  because the lone ldc.i4 was being interpreted as both index and
  value (Roslyn elides the add when the index is 0 on a fixed buffer).
- Support runtime store values: `cur.layout[idx] = local` /
  `ldsfld` now lower through a value scan + EmitLoadValueIntoA helper.
- Narrow bare `catch { }` in Transpiler.StructAnalysis.cs to
  `catch (BadImageFormatException)` so genuine bugs aren't swallowed.
- Extract IL2NESWriter.ConsumeSkip helper and use it from both
  dispatch loops so the skip-marker logic can't drift apart.
- Strengthen StructsTests assertions with multi-byte hex patterns
  that pin down both the value and the absolute address.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@jonathanpeppers jonathanpeppers merged commit 875a869 into main May 28, 2026
3 checks passed
@jonathanpeppers jonathanpeppers deleted the copilot/add-transpiler-support-structs branch May 28, 2026 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Transpiler: support structs with fixed-size array fields

3 participants