Skip to content

Managed ilasm: comprehensive parity fixes with native ilasm - #131507

Open
jkoritzinsky wants to merge 79 commits into
mainfrom
ilasm-fixups
Open

Managed ilasm: comprehensive parity fixes with native ilasm#131507
jkoritzinsky wants to merge 79 commits into
mainfrom
ilasm-fixups

Conversation

@jkoritzinsky

Copy link
Copy Markdown
Member

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.StringString, 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

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)

MetricBeforeAfter
Matching (byte-identical ildasm)09
Module-name-only diffs1,862
Cascading-only diffs676
Real primary diffs2,128+ failures420 (all cosmetic)
Managed ilasm failures2,1284 (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.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

CopilotAI 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 substantially expands managed ilasm parser/emitter parity with native ilasm, including multi-document compilation, grammar fixes, metadata/token resolution adjustments, and a large suite of new unit tests validating metadata/IL byte correctness.

Changes:

  • Extend the compiler pipeline (preprocessor + parser) to support multi-document input and stricter parser diagnostics.
  • Improve preprocessor macro expansion by re-lexing macro values into correct token streams.
  • Add broad managed ilasm regression/unit test coverage across directives, signatures, metadata emission, and IL encoding.

Reviewed changes

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

Show a summary per file
FileDescription
src/tools/ilasm/tests/ILAssembler.Tests/VTableTests.csNew tests for .vtfixup emission and .sdata layout validation.
src/tools/ilasm/tests/ILAssembler.Tests/TypeSignatureTests.csNew test for multidimensional array bounds parsing.
src/tools/ilasm/tests/ILAssembler.Tests/TypeReferenceTests.csNew tests for .this/.base/.nester diagnostics and TypeRef→TypeDef resolution/backpatching behavior.
src/tools/ilasm/tests/ILAssembler.Tests/TypedefTests.csNew tests for typedef alias resolution and missing-alias diagnostics.
src/tools/ilasm/tests/ILAssembler.Tests/SyntaxTests.csNew tests for string escape handling, numeric literal diagnostics, and parser error reporting.
src/tools/ilasm/tests/ILAssembler.Tests/SourceDirectiveTests.csNew tests for .language/.line directives and embedded PDB generation behavior.
src/tools/ilasm/tests/ILAssembler.Tests/SecurityTests.csNew tests for security directive diagnostics and DeclSecurity emission.
src/tools/ilasm/tests/ILAssembler.Tests/PropertyTests.csNew tests for property emission, initOpt constants, and property-owned custom attributes.
src/tools/ilasm/tests/ILAssembler.Tests/PreprocessedTokenSourceTests.csUpdate preprocessor construction and add macro re-lexing tests.
src/tools/ilasm/tests/ILAssembler.Tests/ParameterTests.csNew tests for param constants, param row emission, signature rewrite correctness, and ldarg-by-name regression.
src/tools/ilasm/tests/ILAssembler.Tests/ModuleTests.csNew tests for module naming defaults and module-level field handling.
src/tools/ilasm/tests/ILAssembler.Tests/MethodTests.csNew tests for vararg, overrides/MethodImpl, calling convention inference, and modopt preservation.
src/tools/ilasm/tests/ILAssembler.Tests/MemberReferenceTests.csPlaceholder test file added for future coverage.
src/tools/ilasm/tests/ILAssembler.Tests/LocalTests.csNew tests for locals/name lookup and MemberRef→Def resolution behavior.
src/tools/ilasm/tests/ILAssembler.Tests/InteropTests.csNew tests for pinvoke parsing/diagnostics and PinvokeImpl emission.
src/tools/ilasm/tests/ILAssembler.Tests/InstructionTests.csNew tests for label fixups, prefix/opcode parsing, and FieldRVA emission.
src/tools/ilasm/tests/ILAssembler.Tests/GenericTests.csNew tests for generic parameter diagnostics and constraint emission correctness.
src/tools/ilasm/tests/ILAssembler.Tests/FunctionPointerTests.csNew tests for function pointer signature encoding.
src/tools/ilasm/tests/ILAssembler.Tests/ExportedTypeTests.csNew tests for exported type diagnostics, forwarding, and .export behavior.
src/tools/ilasm/tests/ILAssembler.Tests/ExceptionHandlingTests.csNew tests for EH blocks and label/offset handling.
src/tools/ilasm/tests/ILAssembler.Tests/EventTests.csNew test ensuring event-owned custom attributes are emitted.
src/tools/ilasm/tests/ILAssembler.Tests/DocumentCompilerTestHelpers.csNew helper utilities for compilation, diagnostics, and token operand decoding.
src/tools/ilasm/tests/ILAssembler.Tests/DataTests.csNew test for invalid metadata token diagnostics.
src/tools/ilasm/tests/ILAssembler.Tests/CustomAttributeTests.csNew tests for custom attribute blob parsing and emission correctness.
src/tools/ilasm/tests/ILAssembler.Tests/ILAssembler.Tests.csprojLink in shared ILOpcode metadata for tests that decode IL bodies.
src/tools/ilasm/src/ILAssembler/PreprocessedTokenSource.csAdd macro value re-lexing, exposed defined-variable state, and constructor updates.
src/tools/ilasm/src/ILAssembler/Options.csAdd OutputFileName option to drive default module naming.
src/tools/ilasm/src/ILAssembler/NameHelpers.csTreat leading-dot type names as name (not namespace separator).
src/tools/ilasm/src/ILAssembler/NamedElementList.csAllow duplicate names with first-wins lookup behavior.
src/tools/ilasm/src/ILAssembler/gen/ilasm-generator.csprojImprove ANTLR generated path rewrite robustness.
src/tools/ilasm/src/ILAssembler/gen/CILVisitor.csRegenerated visitor surface to match grammar updates.
src/tools/ilasm/src/ILAssembler/gen/CILLexer.tokensRegenerated token map after grammar changes.
src/tools/ilasm/src/ILAssembler/gen/CILBaseVisitor.csRegenerated base visitor after grammar changes.
src/tools/ilasm/src/ILAssembler/gen/CIL.g4Major grammar adjustments (directives, tokens, keywords, vararg, byte blobs, etc.).
src/tools/ilasm/src/ILAssembler/DocumentCompiler.csAdd multi-document compile overload; add strict parser error listener; persist preprocessor defines across documents.
src/tools/ilasm/src/ILAssembler/Diagnostic.csAdd diagnostic ID/template for excessive generic parameter counts.
src/tools/ilasm/src/ILAssembler/BlobBuilderExtensions.csEmit pseudo-handle coded indices for TypeRef entities in signatures.
src/tools/ilasm/src/ilasm/Program.csRead/compile multiple input files; normalize native-style args; plumb OutputFileName into options.
src/tools/ilasm/src/ilasm/IlasmRootCommand.csAdd native-style option aliases (e.g., -OUTPUT, -DLL, etc.).
src/tools/ilasm/KNOWN-ISSUES.mdDocument current known limitation (TLS RVA statics).
Comments suppressed due to low confidence (3)

src/tools/ilasm/src/ILAssembler/NamedElementList.cs:85

  • NamedElementList now allows duplicate names (first-wins) via TryAdd, but Remove/RemoveAt still unconditionally remove the name mapping. If a non-mapped duplicate is removed, this drops the lookup entry for the still-existing first element; if the mapped element is removed, name lookup should fall back to the next remaining element with the same name. This can break name-based lookups after removals.
 public bool Remove(T item)
{
bool result = _elements.Remove(item);
if (result)
{
_elementsByName.Remove(item.Name);
}
return result;
}
public void RemoveAt(int index)
{
T element = _elements[index];
_elements.RemoveAt(index);
_elementsByName.Remove(element.Name);
}

src/tools/ilasm/tests/ILAssembler.Tests/VTableTests.cs:119

  • Same endianness issue as above: these values are read from PE bytes (little-endian). Use BinaryPrimitives.Read*LittleEndian rather than BitConverter so the test behaves correctly on big-endian targets.
    src/tools/ilasm/tests/ILAssembler.Tests/VTableTests.cs:168
  • Same endianness issue: slot count/RVA/tokens are read from PE bytes (little-endian). Using BitConverter makes the test endian-dependent; prefer BinaryPrimitives.Read*LittleEndian here too.

Comment threadsrc/tools/ilasm/src/ILAssembler/Options.cs
Comment threadsrc/tools/ilasm/tests/ILAssembler.Tests/VTableTests.cs Outdated

CopilotAI 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 wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

CopilotAI review requested due to automatic review settings July 29, 2026 00:15

CopilotAI 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 wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@dotnet-policy-servicedotnet-policy-serviceBot added the linkable-framework Issues associated with delivering a linker friendly framework label Jul 29, 2026
CopilotAI review requested due to automatic review settings July 29, 2026 00:34

CopilotAI 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 47 out of 51 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

src/tools/ilasm/tests/ILAssembler.Tests/VTableTests.cs:64

  • These reads interpret PE/CLR structures which are defined as little-endian. Using BitConverter makes the test endianness-dependent. Please use BinaryPrimitives.Read*LittleEndian here (and similarly for the other BitConverter reads later in this file).
    src/tools/ilasm/tests/ILAssembler.Tests/InstructionTests.cs:88
  • This test is reading a little-endian PE value. BitConverter makes the test endianness-dependent; use BinaryPrimitives.ReadInt32LittleEndian instead (and update the other BitConverter reads in this file as well).
    src/tools/ilasm/src/ILAssembler/Options.cs:139
  • The XML comment says this is "filename only, no directory", but the property doesn't enforce that. Either normalize to Path.GetFileName in the setter / consumer, or adjust the comment to avoid stating an invariant that isn't guaranteed.
 /// <summary>
/// Output file name (filename only, no directory). Used as default module name when no .module directive is present.
/// </summary>
public string? OutputFileName { get; set; }

CopilotAI review requested due to automatic review settings July 29, 2026 00:42

CopilotAI 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 47 out of 51 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/tools/ilasm/src/ILAssembler/Options.cs:139

  • Options.OutputFileName is new public API surface. With no linked api-approved issue in the PR metadata, this needs either API approval linkage or reducing the visibility (and adjusting tests accordingly) if it’s meant to be internal implementation detail.
 /// <summary>
/// Output file name (filename only, no directory). Used as default module name when no .module directive is present.
/// </summary>
public string? OutputFileName { get; set; }

src/tools/ilasm/src/ILAssembler/NamedElementList.cs:48

  • Add/Insert now use TryAdd so the first duplicate name wins, but the rest of the type still assumes a 1:1 mapping between name and element (e.g., indexer setter overwrites, and Remove/RemoveAt always remove the name key). With duplicates present, the name map can become stale or lose the winning entry. Consider making the name-index semantics consistent across mutation operations (e.g., only remove the key when it points to the removed element, and re-point it to the next remaining element with the same name).
 public void Add(T item)
{
_elements.Add(item);
// Use TryAdd to keep the first element for name lookup when duplicate names exist.
// This matches native ilasm behavior where duplicate generic parameter names are allowed
// and the first definition wins for name-based lookup.
_elementsByName.TryAdd(item.Name, item);
}

Comment threadsrc/tools/ilasm/src/ILAssembler/DocumentCompiler.cs Outdated
jkoritzinskyand others added 23 commits August 26, 2026 14:31
… token locations
- Fix method body fallback to emit proper header via AddMethodBody
instead of raw WriteContentTo (per @copilot)
- Validate VisitHexbyte text is valid hex before parsing, gracefully
handle non-hex ID tokens and values > 0xFF (per @copilot)
- Clone macro expansion tokens to inherit the original macro
identifier's source location for stable diagnostics (per @copilot)
- Remove duplicate Replace in ilasm-generator.csproj (per @am11)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
VisitPropDecl and VisitEventDecl checked ChildCount != 2 to filter
non-accessor declarations, but this also filtered out customAttrDecl
entries (which have 1 child). Custom attributes inside property and
event blocks were silently dropped instead of being emitted.
Handle customAttrDecl in the property/event processing loop by
visiting the custom attribute and setting its Owner to the enclosing
property or event entity.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-RESOURCES error
- Fix MethodSpec rewriter to use correct signature header byte (0x0A)
instead of SignatureAttributes.Generic (0x10) per ECMA-335.
- Fix array shape encoding to stop counting sizes/lower bounds at the
first null dimension (contiguous from start), not the last non-null.
- Restore -o and -O short aliases for --output and --optimize to avoid
breaking existing managed ilasm users/scripts.
- Throw ArgumentException on -RESOURCES= instead of silently dropping.
- Fix ilasm-generator.csproj: compute absolute grammar path in a
separate MSBuild property to avoid nested single-quote issues.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The HasFieldMarshal flag was only set when MarshallingDescriptor.Count > 0,
but the FieldMarshal row was emitted whenever MarshallingDescriptor was
non-null (even if empty). Align both to use the same Count > 0 predicate.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Addresses PR review comment r3610337318. Previously, a nested TypeRef
whose enclosing type resolved to a local TypeDef but which itself was
not found locally was emitted with the enclosing type's TypeDefinition
handle as its ResolutionScope, which is an invalid coded index and threw
ArgumentException at emission.
Record every TypeRef as a row in PseudoHandle (creation) order, matching
native ilasm which preserves all TypeRef rows, then resolve locals to
their TypeDef handles. Because PseudoHandle is the gapless 1-based index
into _typeReferences, the emitted row handle equals the PseudoHandle,
keeping rows aligned with the handles used during parsing/signature
encoding. Nested TypeRef resolution scopes now use the enclosing
TypeRef's PseudoHandle (a valid TypeReference coded index) instead of the
TypeDef it may have resolved to.
Add tests validating that resolved TypeRefs still emit rows in order and
that the nested-enclosing-local/nested-missing scenario emits a valid
ResolutionScope. Update the token-backpatching and self-assembly tests to
decode and validate the method IL operand tokens (via the shared ILOpcode
tables) instead of asserting TypeRef-row absence.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ab70f26b-7491-45bc-b4ce-6cb13c509fc2
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 81eb86ba-1424-4d1e-8fed-ca5be7b511cb
… tests
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 4307156b-37a7-495d-9af6-fe9174c05b0b
Keep legacy command line compatibility in the normalization layer without exposing old-style aliases in generated help. Add regression coverage for native -OUT syntax and apply the requested source cleanups.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e2b8e8da-8e3f-4a26-9c8b-73857fabfa6c
Process recursively expanded tokens before the remaining outer macro tokens and cover nested multi-token expansion with a regression test.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 69313a92-28b5-4e32-8be8-ea4f6e44a5c8
Reject malformed attached values before System.CommandLine can treat them as positional input files, while preserving supported modern short option forms.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 69313a92-28b5-4e32-8be8-ea4f6e44a5c8
Derive PE, MVID, and portable PDB identifiers from content in deterministic mode, including custom vtable and export images, and add byte-identical regressions.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 69313a92-28b5-4e32-8be8-ea4f6e44a5c8
Patch deterministic MVIDs after PE serialization, parse quoted dotted-name segments, and report unsupported TLS RVA data declarations with a dedicated diagnostic.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 69313a92-28b5-4e32-8be8-ea4f6e44a5c8
Set executable, architecture, and DLL COFF characteristics explicitly and cover output-kind and mapped FieldRVA images.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 69313a92-28b5-4e32-8be8-ea4f6e44a5c8
Reject ambiguous or unknown legacy options before System.CommandLine can reinterpret them, preserve modern parser contexts, and expand compatibility coverage.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 69313a92-28b5-4e32-8be8-ea4f6e44a5c8
@jkoritzinsky

Copy link
Copy Markdown
MemberAuthor

OK stack is now updated. Once we get #132346 in, I'll start on those remaining issues.

Comment threadsrc/tools/ilasm/src/ilasm/NativeCommandLine.cs Outdated
Use the root command option schema for value arity and aliases, and pass ParseResult directly into Program so the schema can be shared with tests.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 69313a92-28b5-4e32-8be8-ea4f6e44a5c8
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-ILTools-coreclrlinkable-frameworkIssues associated with delivering a linker friendly framework

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

6 participants

@jkoritzinsky@MichalStrehovsky@am11@jkotas