Managed ilasm: comprehensive parity fixes with native ilasm

[jkoritzinsky]

Managed ilasm parity fixes

This PR brings the managed ilasm (src/tools/ilasm) to near-parity with the native ilasm across 2,868 ilproj test files in the repo. Starting from ~2,128 managed ilasm failures, this work reduces real (non-cosmetic) differences to zero actionable items.

Summary of changes (53 commits, 23 files, +12,035 / -6,617 lines)

Parser & Grammar Fixes

• Fixed ANTLR grammar for .language, .line (QSTRING support), value/instance keywords in identifiers, multiple ddItem without braces, &label references, atOpt with integer, empty pinvokeimpl(), security attribute blob type ordering, function pointer syntax
• Added ANTLR parser error listener to DocumentCompiler for strict diagnostics
• Fixed preprocessor #define macro expansion to re-lex multi-token values
• Regenerated ANTLR parser after all grammar changes

Metadata Emission Fixes

• MemberRef → MethodDef/FieldDef resolution: Resolve local method/field references to definition tokens (matching native ilasm behavior)
• TypeRef → TypeDef resolution: Lazy TypeRef tracking with PseudoHandle for signature encoding. TypeRefs whose resolution scope matches the current assembly resolve to local TypeDef handles. Signature rewriter remaps PseudoHandle-based coded indices in all signature blobs and IL instruction tokens are backpatched.
• Field attribute flags: HasDefault, HasFieldMarshal, HasFieldRVA, PinvokeImpl
• Method attributes: Auto-instance calling convention, auto-RTSpecialName|SpecialName for .ctor/.cctor, PinvokeImpl flag
• Param emission: Always emit Param rows for explicit parameters, auto-generate A_N names for unnamed parameters
• ClassLayout: Emit for explicit layout types even without .pack/.size
• GenericParamConstraint: Sort by Owner handle during emission
• Custom attributes: Fix type-level and top-level handlers, blob prolog (WriteUInt16 not WriteInt32), module vs assembly ownership
• Stackreserve: Pass directive value to PEHeaderBuilder
• Locals: Build LOCAL_SIG standalone signature from parsed .locals declarations
• Corelib TypeRef redirect: Normalize different corelib assembly names
• Primitive type codes: Emit correct codes for well-known corelib types (System.String → String, etc.)
• Leading-dot type names: Position 0 dot is part of the name, not a namespace separator
• DebuggableAttribute: Deferred to BuildImage() so assembly refs are available
• Vararg signatures: Fix parameter count (exclude sentinels) and parent resolution

Signature Rewriter (new)

• Rewrites all signature blobs (field, method, standalone, property, TypeSpec, MethodSpec) from PseudoHandle-based TypeRef coded indices to resolved real handles
• Fixed GetModifiedType to write modifier as raw coded index (not full type encoding)
• Fixed GetArrayType to emit ELEMENT_TYPE_ARRAY prefix byte

IL Body Fixes

• TypeRef IL token backpatching for instr_type, instr_tok, and instr_field mdtoken paths
• Workaround for BlobBuilder chunk boundary corruption (InstructionEncoder produces corrupted IL when mixing OpCode with direct CodeBuilder writes across BlobBuilder chunk boundaries #127261) — increased InstructionEncoder BlobBuilder initial capacity to 4096
• Array shape encoding fix for multi-dimensional arrays (contiguous sizes/lower bounds only)
• Security attribute blob SQSTRING quote stripping

Test Coverage

• 236 unit tests (up from ~25 at start), covering all major fix categories
• Tests use CompileAndGetReader to verify metadata byte-level correctness

Comparison results (2,868 ilproj files)

Metric	Before	After
Matching (byte-identical ildasm)	0	9
Module-name-only diffs	—	1,862
Cascading-only diffs	—	676
Real primary diffs	2,128+ failures	420 (all cosmetic)
Managed ilasm failures	2,128	4 (2 TLS, 2 65K+ generics)

The remaining 420 files with "real" diffs are all non-actionable: custom attribute metadata ordering (246), PE header line ordering (44), field RVA section placement (21), assembly metadata cosmetic (19), corelib ref name cascading (12), ildasm typedef presentation (4), and override TypeRef formatting (3).

Note

This PR was authored with the assistance of GitHub Copilot.

[Copilot]

Pull request overview

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

[am11]

My model was also telling me RVA is not handled or it's some sort of huge deal:

src/tools/ilasm/KNOWN-ISSUES.md 	Documents remaining managed-ilasm limitations (TLS RVA statics).

then I insisted and it read through native ilasm implementation and implemented it anyway. It's better to just close the loop and not add KNOWN-ISSUES.md. 😅

[jkoritzinsky]

TLS RVA statics are an issue we aren't planning on fixing (we'll migrate the tests to be C++/CLI). That's why I made a known issues doc.

[jkoritzinsky]

One more parity item that we skipped:

We are not emitting .typedef entries into the TypeSpec table with an ELEMENT_TYPE_TYPEDEF signature. This is something we can do in the future to help meet parity (and could be a future design for exposed aliases, an idea from @agocke).

[am11]

Could you merge from main (post #127096 merge), so we can see the results in the CI? LGTM overall! 👍

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "b6bb5da48a6864368e4710d0ed3838e0ca15ee90",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "1f84bc9339681b4d27eeeaf80c27e18340b25524",
  "last_reviewed_commit": "ce5bf8b3efcab86a02ce9baeef6bf3395d6f07be",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "1f84bc9339681b4d27eeeaf80c27e18340b25524",
  "last_recorded_worker_run_id": "29675328747",
  "review_attempt_commit": "b6bb5da48a6864368e4710d0ed3838e0ca15ee90",
  "review_attempt_base_ref": "main",
  "review_attempt_count": 1,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "ce5bf8b3efcab86a02ce9baeef6bf3395d6f07be",
      "review_id": 4730553471
    }
  ]
}

[github-actions]

Holistic Review

Motivation: Well-justified. The managed ilasm is intended to replace the native ilasm, and closing the remaining ~2,128 real behavioral differences against 2,868 in-repo ilproj test files is necessary for that transition. The problem is concrete and the parity gap is measured, not speculative.

Approach: Broadly sound. Grammar/parser fixes are regenerated (not hand-edited generated files), metadata/signature emission is corrected to match ECMA-335 and native ilasm behavior, and a new PseudoHandle-based TypeRef→TypeDef resolution + signature-rewriter mechanism is a reasonable design for late token resolution. The native-flag compatibility shim (NormalizeNativeArgs) and multi-document compilation refactor are appropriate. The large diff is dominated by regenerated ANTLR output; the hand-authored surface is comparatively small.

Summary: ⚠️ Needs Human Review. The direction and the bulk of the implementation look correct and are backed by extensive byte-level tests, but a few areas warrant maintainer attention: (1) an emission path that can pass a TypeDefinition handle where only a ResolutionScope coded index is valid (potential throw / corrupt metadata for nested-type references); (2) signature-rewrite fallbacks that silently return blobs still containing un-remapped PseudoHandle coded indices on decode failure; and (3) the flagship TypeRef→TypeDef IL-token backpatching category is asserted only indirectly in tests. None are clearly release-blocking, but a domain expert should confirm the resolution-scope edge case and the rewriter fallback safety.

---

Detailed Findings

⚠️ Metadata emission — Nested TypeRef resolution scope (inline)

See the inline comment on EntityRegistry.cs line 242. A nested TypeRef whose outer type resolved to a local TypeDef but whose nested member is not found locally is still recorded in the TypeRef table, and on emission a TypeDefinition handle is passed as the ResolutionScope — which the coded-index encoder rejects. Highest-priority item to confirm.

⚠️ Signature rewriter — silent fallback to un-remapped blob

In EntityRegistry.cs, RewriteSignatureBlob (and the analogous RewriteTypeSpecBlob/RewriteMethodSpecBlob) use catch { return original; }. Because ResolveTypeReferences drops many TypeRef rows (resolved to TypeDef), a PseudoHandle's row number no longer equals the entity's final table row — the rewriter is precisely what remaps pseudo→real handles. On any decode failure, returning original emits a blob still carrying pseudo-handle tokens pointing at wrong/non-existent rows: silently corrupt metadata rather than a diagnosable error. Consider surfacing a diagnostic (or asserting) instead of swallowing the failure. This is a broad/cross-cutting concern spanning several methods, so it is reported here rather than inline.

💡 GetModifiedType reads modifier.Handle unconditionally

In EntityRegistry.cs GetModifiedType, CodedIndex.TypeDefOrRefOrSpec(modifier.Handle) is written unconditionally. If a custom modifier ever arrives blob-backed (nil Handle), this emits coded index 0 instead of failing. Custom modifiers are normally handle-backed, so this is low-confidence — worth an assert.

⚠️ Test quality — TypeRef→TypeDef IL backpatching under-asserted

DocumentCompilerTests.cs adds strong, byte-level coverage across most fix categories (signatures, layout, custom attributes, function pointers, arrays, locals, corelib redirect, #define relexing) — this is the right approach for an assembler. However, the flagship "TypeRef→TypeDef IL-token backpatching" tests (TypeRefInILToken_/Castclass_/Ldtoken_/FieldMdtoken_BackpatchedAfterResolution) only assert that the TypeRef table no longer contains the type name, or that rva > 0. They do not decode the method-body operand to confirm the token's table is TypeDef (0x02) — which is the actual backpatch behavior. CatchClause_SelfAssemblyTypeRef_ResolvesToTypeDef shows the correct pattern via catchRegion.CatchType.Kind; the IL-operand tests should decode the operand similarly. Secondary: FunctionPointer_PtrToFnPtr_EmitsPtrThenFnPtr uses Assert.Contains over the whole blob and cannot catch a PTR/FNPTR ordering regression; several *_ParsedCorrectly tests only assert Assert.Empty(diagnostics) without verifying emitted bytes; and duplicative [Fact] clusters could be consolidated into [Theory]/[InlineData] per repo conventions. None are correctness-breaking.

✅ Native-flag shim and multi-document refactor

Program.NormalizeNativeArgs correctly translates single-dash, case-insensitive native flags (including -DEBUG=IMPL/OPT) and explicitly rejects unsupported -RESOURCES= with a clear error. The DocumentCompiler refactor to compile an ImmutableArray<SourceText> (rather than concatenating inputs) is cleaner, threads DefinedVariables between documents, adds a parser error listener with per-source diagnostics, and guards missing source lookups. NameHelpers.SplitDottedNameToNamespaceAndName correctly treats a position-0 dot as part of the name, and NamedElementList switching to TryAdd (first-wins) matches native handling of duplicate generic-parameter names. KNOWN-ISSUES.md transparently documents the unimplemented TLS RVA statics gap.

💡 Note

One hand-authored area (GrammarVisitor.cs, the largest non-generated change at ~964 lines) was not fully re-analyzed by an assisting pass within time; a human reviewer should give the vararg signature parameter-count, custom-attribute blob prolog, and array-shape encoding changes there a focused look, cross-referenced against the corresponding new tests.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 787.3 AIC · ⌖ 11.6 AIC · ⊞ 10K

[Copilot]

Pull request overview

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

[github-actions]

Holistic Review

Motivation: Unchanged and well-justified. The managed ilasm is intended to replace native ilasm, and closing the remaining behavioral differences against the in-repo ilproj test corpus is necessary for that transition. This incremental commit narrows a concrete correctness gap in TypeRef emission that was flagged in the prior review.

Approach: Unchanged overall, and the single new commit refines the flagship TypeRef→TypeDef resolution design in a principled way. Rather than dropping TypeRef rows that resolve to local TypeDefs, every TypeRef is now recorded in _typeReferences (PseudoHandle) order and only afterward resolved to a TypeDef handle. Because the emission loop iterates the same list in the same order, the emitted TypeRef row handle equals the PseudoHandle used during signature/IL encoding, and nested-TypeRef ResolutionScope now uses the enclosing TypeRef's PseudoHandle (a valid coded index) instead of a TypeDefinition handle. This matches native ilasm's preservation of all TypeRef rows and eliminates the invalid-coded-index emission path.

Summary: ⚠️ Needs Human Review — improved. This commit directly resolves the two highest-priority items from the initial review: the nested-TypeRef ResolutionScope crash (finding #1) and the under-asserted IL-token backpatching tests (finding #4). The fix is correct in the reviewed scope: the PseudoHandle-equals-row-handle invariant holds by construction, the new IL-decoding test helper handles prefix1 two-byte opcodes and the variable-length switch_ correctly, and the new/updated tests decode actual operand tokens and assert valid ResolutionScopes (including the enclosing-local/nested-missing edge case). The remaining open items from the prior review (silent signature-rewriter fallback, GetModifiedType unconditional handle read, and a focused human look at GrammarVisitor.cs) are in code unchanged by this commit and are not re-raised as new findings here.

---

Assessment History

• review 4730553471 reviewed commit ce5bf8b with verdict ⚠️ Needs Human Review. Current verdict is also ⚠️ Needs Human Review, but the assessment is changed: the new commit b6bb5da ("emit all TypeRef rows even when resolved to local TypeDefs") resolves that review's finding #1 by recording all TypeRef rows in PseudoHandle order and scoping nested TypeRefs to the enclosing TypeRef's PseudoHandle instead of an invalid TypeDefinition ResolutionScope, and resolves finding #4 by rewriting the backpatching/self-assembly tests to decode method-body operand tokens (via the shared ILOpcode tables) and assert TypeDef/FieldDef/TypeRef token kinds and valid ResolutionScopes. The verdict remains Needs Human Review only because that review's finding #2 (signature-rewriter catch { return original; } fallback) and the GrammarVisitor.cs follow-up are in unchanged code and still merit maintainer confirmation.

Detailed Findings

No new actionable findings in the incremental scope. The single new commit is a targeted, correct fix backed by decode-level tests; no issues were identified in the changed lines.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 114.8 AIC · ⌖ 10.6 AIC · ⊞ 10K

[am11]

Failures look related (probably path normalization win vs. unix?)

[jkoritzinsky]

Failures are in native ilasm, so pre-existing.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/tools/ilasm/src/ilasm/Program.cs:322

• NormalizeNativeArgs is executed before parsing/invocation. Throwing here (e.g., for -RESOURCES=) will bypass System.CommandLine error handling and produce an unhandled exception / stack trace. Prefer letting the parser report this as an unrecognized option (or handle it in a controlled way) rather than throwing.

            if (arg.StartsWith("-RESOURCES=", StringComparison.OrdinalIgnoreCase))
            {
                throw new ArgumentException($"Unsupported native option '{arg}'. The managed ilasm implementation does not support -RESOURCES.");
            }

[jkoritzinsky]

@copilot Fix the code for all comments in this review thread.

Do not make changes beyond what is described in the linked review thread.