Skip to content

Add additional round-trip fixes and add additional unit tests for greater code coverage - #133188

Open
jkoritzinsky wants to merge 37 commits into
mainfrom
ilasm-more-tests
Open

jkoritzinsky wants to merge 37 commits into
mainfrom
ilasm-more-tests

Conversation

@jkoritzinsky

Copy link
Copy Markdown
Member

Creating a new PR to replace #131509.

@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.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @dotnet/jit-contrib
See info in area-owners.md if you want to be subscribed.

Copilot AI lite review requested due to automatic review settings September 3, 2026 18:19

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 review overview

🟡 Changes recommended

The CLI normalization refactor currently misclassifies value-taking options (using Option.Name vs aliases), and Options introduces a breaking public member rename that should be made compatible or internal.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1 High severity · 1 Medium severity · 1 Low severity

New issues introduced by this change (3)
Severity Finding
High severity src/​tools/​ilasm/​src/​ilasm/​NativeCommandLine.cs — optionsWithValues is populated with Option.Name (e.g. "output"), but later checked against…
Medium severity src/​tools/​ilasm/​src/​ILAssembler/​Options.cs — Options is a public type and this change replaces the existing public IsDll property with Dll.…
Low severity src/​tools/​ilasm/​tests/​ILAssembler.Tests/​CommandLineTests.cs — ModernValueOptions currently includes option.Name in addition to prefixed aliases. In…
What changed in this PR

This PR significantly expands the managed ilasm test suite and adjusts the managed IL assembler, test infrastructure, and CI/configuration to support additional IL round-trip scenarios (including a new managed-ilasm roundtrip), while fixing multiple metadata/PE emission and parsing edge cases uncovered by the added coverage.

Changes:

  • Add broad ILAssembler unit test coverage across metadata emission (typedefs/typerefs/exports/resources/events/properties/fields), parser robustness, preprocessor integration, and options/CLI behavior.
  • Update managed ilasm implementation to improve correctness/compatibility for opcode parsing, vararg/signature handling, PrivateScope naming, PE export/vtable fixup emission, and error-tolerant output behavior.
  • Extend CoreCLR test infrastructure/CI to support managedilasmroundtrip and introduce opt-out markers (ManagedIlasmRoundTripIncompatible) with accompanying documentation.
File Description
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​TypeSignatureTests.cs Adds tests for multi-dimensional array local signature shape decoding.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​TypeReferenceTests.cs Adds tests for typeref resolution scopes, mdtoken/.this/.base/.nester, module-scoped typerefs, and typelist parsing.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​TypedefTests.cs Adds typedef alias coverage for fields and custom attributes (including owner-scoped forms).
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​TypeDefinitionTests.cs Adds tests for quoted name segments, class attribute flag behavior, nested types, and interface impl emission.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​SyntaxTests.cs Adds corpus-style tests ensuring malformed/truncated inputs report diagnostics instead of throwing.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​SourceDirectiveTests.cs Strengthens verification of escape processing and adds portable PDB directive coverage across variants/multi-doc scenarios.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​SecurityTests.cs Adds coverage for permissionset syntax variants, emitted DeclSecurity metadata, and error-tolerant behavior.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​PropertyTests.cs Adds tests for property attributes/accessors/custom attributes and indexed property signatures.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​PropertyEventTests.cs New tests validating property/event maps and MethodSemantics emission.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​PreprocessorIntegrationTests.cs New integration tests for includes, conditional compilation, and macro expansion across include levels.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​ParameterTests.cs Adds tests for parameter attribute syntaxes emitting expected flags.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​NativeExportTests.cs New tests validating PE export directory/stub emission and machine-specific stub prefixes.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​ModuleTests.cs Adds tests for .file metadata/resources, module refs, bare .module, and global method/module attribute + PDB emission.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​ManifestResourceTests.cs New tests for embedded/file/assembly-backed manifest resources and deterministic/error-tolerant behavior.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​ILAssembler.Tests.csproj Adds System.CommandLine dependency and links IlasmRootCommand.cs for CLI parsing tests.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​FunctionPointerTests.cs Switches to signature decoding assertions; adds vararg fnptr sentinel coverage.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​FieldTests.cs Adds tests for trailing custom attribute binding and field attribute flag/null constant emission.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​ExportedTypeTests.cs Adds coverage for ExportedType attributes/implementations/custom attributes and TypeDefId emission.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​ExceptionHandlingTests.cs Adds extensive EH region tests (offset-based, label-based, ordering, filter/fault/finally/catch).
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​EventTests.cs Adds event accessor/attribute/other/custom-attr metadata tests and nil-type behavior.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​DocumentCompilerTests.cs New tests ensuring diagnostics have correct document paths and parser errors don’t leak scopes across documents.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​DocumentCompilerTestHelpers.cs Adds shared PEReader compile helper, token operand extraction, and metadata signature decoding provider.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​DataTests.cs Adds coverage for data directives (typed bytes, repeats, large bytearrays, float literals, syntax variants, error isolation).
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​CompilerOptionsTests.cs New tests verifying Options-driven PE/metadata behavior, debug attributes, deterministic output, keyfile handling, and error-tolerant output.
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​CommandLineTests.cs Updates modern option corpus generation to reflect IlasmRootCommand options (CLI normalization tests).
src/​tools/​ilasm/​tests/​ILAssembler.Tests/​AssemblyTests.cs Updates Options property name usage and adds additional assembly reference/identity/attribute parsing tests.
src/​tools/​ilasm/​src/​ILAssembler/​VTableExportPEBuilder.cs Fixes COR header vtable directory offset and improves vtable/export directory emission logic (including ordinal gaps).
src/​tools/​ilasm/​src/​ILAssembler/​PreprocessedTokenSource.cs Fixes include-stack EOF handling to avoid incorrectly popping the entry source.
src/​tools/​ilasm/​src/​ILAssembler/​Options.cs Renames the DLL output option property (public surface) and updates option model.
src/​tools/​ilasm/​src/​ILAssembler/​NameHelpers.cs Adds helper to normalize PrivateScope metadata names (strip $PST + token suffix).
src/​tools/​ilasm/​src/​ILAssembler/​MetadataExtensions.cs Adds ILOpCode.Unused support used by opcode parsing.
src/​tools/​ilasm/​src/​ILAssembler/​gen/​CIL.g4 Fixes token spelling and extends grammar for dotted names, mdtoken forms, unsigned native types, permissionset syntax, and opcode set.
src/​tools/​ilasm/​src/​ILAssembler/​EntityRegistry.cs Updates metadata emission behavior (PrivateScope naming, method body encoding heuristics, methodimpl owner, vararg sig rewriting, error-tolerant metadata row preservation).
src/​tools/​ilasm/​src/​ILAssembler/​DocumentCompiler.cs Enhances parser error context propagation so the visitor can avoid leaking state across erroneous contexts.
src/​tools/​ilasm/​src/​ILAssembler/​Diagnostic.cs Adds a new diagnostic ID/message for constraint owner out-of-range scenarios.
src/​tools/​ilasm/​src/​ilasm/​Program.cs Refactors CLI handling to use ParseResult explicitly and wires Normalize() with a single command instance.
src/​tools/​ilasm/​src/​ilasm/​NativeCommandLine.cs Refactors CLI normalization to derive supported options from IlasmRootCommand.
src/​tools/​ilasm/​src/​ilasm/​IlasmRootCommand.cs Removes embedded SetAction wiring (now configured from Program.Main).
src/​tools/​ilasm/​src/​ilasm/​ilasm.csproj Inlines NativeAOT runtime settings previously imported from AotCompilerCommon.props.
src/​tests/​Loader/​classloader/​regressions/​dev10_403582/​gentype.ilproj Marks the project as incompatible with managed ilasm round-trip.
src/​tests/​Loader/​classloader/​regressions/​dev10_403582/​genmeth.ilproj Marks the project incompatible with IL round-trip (and documents ildasm crash).
src/​tests/​Directory.Build.targets Emits marker files for IlasmRoundTripIncompatible and ManagedIlasmRoundTripIncompatible.
src/​tests/​Common/​CLRTest.Jit.targets Adds marker-file based skipping and introduces managed-ilasm round-trip selection logic.
src/​tests/​Common/​CLRTest.Execute.targets Makes IlasmRoundTripIncompatible imply ManagedIlasmRoundTripIncompatible.
src/​tests/​Common/​CLRTest.Execute.Batch.targets Adds managed-ilasm-only skip logic for batch execution wrapper.
src/​tests/​Common/​CLRTest.Execute.Bash.targets Adds managed-ilasm-only skip logic for bash execution wrapper.
eng/​pipelines/​common/​templates/​runtimes/​run-test-job.yml Adds managedilasmroundtrip scenario to ilasm test group.
docs/​workflow/​testing/​coreclr/​test-configuration.md Documents ManagedIlasmRoundTripIncompatible and the implication behavior.
docs/​workflow/​testing/​coreclr/​requiresprocessisolation.md Updates guidance for IL round-trip incompatibility properties and isolation requirements.
docs/​workflow/​ci/​disabling-tests.md Documents disabling managed vs all ilasm round-trip and notes merged-runner behavior.

Comment thread src/tools/ilasm/src/ilasm/NativeCommandLine.cs Outdated
Comment thread src/tools/ilasm/src/ILAssembler/Options.cs
Comment thread src/tools/ilasm/tests/ILAssembler.Tests/CommandLineTests.cs

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 review overview

🟡 Changes recommended

The new fat-header forcing logic in EntityRegistry can cause .maxstack-only methods (without .zeroinit) to emit InitLocals, altering semantics/metadata in a way that should be corrected or explicitly justified and locked in with tests.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 2 Medium severity

New issues introduced by this change (1)
Severity Finding
Medium severity src/​tools/​ilasm/​src/​ILAssembler/​EntityRegistry.csforceFatHeader triggers when methodDef.MaxStack < 8 (i.e., .maxstack was explicitly…
Pre-existing issues (1)
Severity Finding
Medium severity src/​tools/​ilasm/​src/​ILAssembler/​Options.cs — Options is a public type and this change replaces the existing public IsDll property with Dll.… View comment
Issues resolved since last review (2)
Severity Finding
Low severity src/​tools/​ilasm/​tests/​ILAssembler.Tests/​CommandLineTests.cs — ModernValueOptions currently includes option.Name in addition to prefixed aliases. In… View resolved comment
High severity src/​tools/​ilasm/​src/​ilasm/​NativeCommandLine.cs — optionsWithValues is populated with Option.Name (e.g. "output"), but later checked against… View resolved comment

Comment thread src/tools/ilasm/src/ILAssembler/EntityRegistry.cs Outdated
jkoritzinsky added a commit that referenced this pull request Sep 4, 2026
…ing. (#133202)

The existing implementation was leading to file lock conflicts on
#133188 and other managed ILASM PRs.
Base automatically changed from ilasm-fixups to main September 9, 2026 16:42
Copilot AI review requested due to automatic review settings September 9, 2026 16:42

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 review overview

🟡 Changes recommended

There are correctness issues in newly modified code paths (notably CLI normalization validation and method-body header emission) that can contradict the PR’s own tests/expected behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 4 Medium severity

New issues introduced by this change (2)
Severity Finding
Medium severity src/​tools/​ilasm/​src/​ILAssembler/​EntityRegistry.cs — The fallback body emission also forces InitLocals when forceFatHeader is true. That can change…
Medium severity src/​tools/​ilasm/​src/​ilasm/​NativeCommandLine.cs-O:false / -g:true / -q:true should be treated as valid modern short-option forms (the unit…
Pre-existing issues (2)
Severity Finding
Medium severity src/​tools/​ilasm/​src/​ILAssembler/​EntityRegistry.csforceFatHeader triggers when methodDef.MaxStack < 8 (i.e., .maxstack was explicitly… View comment
Medium severity src/​tools/​ilasm/​src/​ILAssembler/​Options.cs — Options is a public type and this change replaces the existing public IsDll property with Dll.… View comment
Suppressed comments (2)

src/tools/ilasm/src/ILAssembler/EntityRegistry.cs:387

  • When forceFatHeader is true, this code forces the InitLocals flag on even if the original IL didn’t request .locals init. That changes emitted method-body semantics/metadata; forcing a fat header should not require changing the InitLocals bit (you already pass hasDynamicStackAllocation || forceFatHeader to avoid a tiny header).
                        MethodBodyAttributes encodingAttributes = methodDef.BodyAttributes;
                        if (forceFatHeader)
                        {
                            encodingAttributes |= MethodBodyAttributes.InitLocals;
                        }

src/tools/ilasm/src/ILAssembler/EntityRegistry.cs:425

  • This fallback path forces InitLocals when forceFatHeader is true, which can change the emitted method-body header semantics (even for methods that didn’t specify .locals init). It should be possible to keep the original BodyAttributes and rely on hasDynamicStackAllocation || forceFatHeader to force a fat header when needed.
                            localsSigHandle,
                            forceFatHeader ? methodDef.BodyAttributes | MethodBodyAttributes.InitLocals : methodDef.BodyAttributes,
                            hasDynamicStackAllocation: methodDef.HasDynamicStackAllocation || forceFatHeader);

Comment thread src/tools/ilasm/src/ILAssembler/EntityRegistry.cs Outdated
Comment thread src/tools/ilasm/src/ilasm/NativeCommandLine.cs Outdated
Comment thread docs/workflow/ci/disabling-tests.md
Copilot AI review requested due to automatic review settings September 9, 2026 22:29

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 review overview

🔵 Needs a closer look

It spans low-level PE/metadata emission, grammar/parser behavior, test-harness execution logic, and CI scenario expansion, which warrants human validation of compatibility and unintended behavioral changes.

Review tier: Lite
Findings: 4 Medium severity

Pre-existing issues (4)
Severity Finding
Medium severity src/​tools/​ilasm/​src/​ilasm/​NativeCommandLine.cs-O:false / -g:true / -q:true should be treated as valid modern short-option forms (the unit… View comment
Medium severity src/​tools/​ilasm/​src/​ILAssembler/​EntityRegistry.cs — The fallback body emission also forces InitLocals when forceFatHeader is true. That can change… View comment
Medium severity src/​tools/​ilasm/​src/​ILAssembler/​EntityRegistry.csforceFatHeader triggers when methodDef.MaxStack < 8 (i.e., .maxstack was explicitly… View comment
Medium severity src/​tools/​ilasm/​src/​ILAssembler/​Options.cs — Options is a public type and this change replaces the existing public IsDll property with Dll.… View comment

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 review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review tier: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (2)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Remove active conditional blocks when an include ends

src/​tools/​ilasm/​src/​ILAssembler/​PreprocessedTokenSource.cs:111

When an included file reaches EOF with an active #ifdef, its per-source count is positive, but this only pops _includeSourceStack; the corresponding entries remain in _activeIfDefBlocks. If the including file is itself inside an active block, its next #else or #endif then pops the stale included-file entry and corrupts the conditional state, producing spurious diagnostics. Remove the active entries belonging to the popped source before continuing.

[!NOTE] This comment was generated by GitHub Copilot.

Low severity Qualify the process-isolation summary for managed ILASM markers

docs/​workflow/​testing/​coreclr/​requiresprocessisolation.md:212

This table now says that ManagedIlasmRoundTripIncompatible always requires process isolation, but the preceding section explicitly documents that shared-library projects use the merged-runner marker and do not need it. Please qualify this row for runnable/per-process tests (or otherwise reconcile the table) so the documented configuration guidance is not contradictory.

Comment thread src/tools/ilasm/src/ILAssembler/VTableFixupSupport.cs
Clarified that shared-library projects do not need process isolation for the property related to disabling tests. Added a note that these workarounds are temporary until bugs are fixed.
Copilot AI review requested due to automatic review settings September 15, 2026 22:50

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 review overview

🔵 Needs a closer look

Three moderate findings and one nit remain unresolved.

Review tier: Lite
Findings: None

Resolved since last review (1)

Explain the coupling between the custom PE builder's COR header offset and System.Reflection.Metadata's managed text layout.

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

Copilot-Session: 806a996a-9fcc-42b7-a902-cac954582b24
Match native ilasm's observable InitLocals behavior for short localloc methods whose ildasm text cannot otherwise represent the original header flag.

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

Copilot-Session: 806a996a-9fcc-42b7-a902-cac954582b24
Runtime test assets are built architecture-neutral, so emit the temporary x86 OOM exclusions without TargetArchitecture conditions.

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

Copilot-Session: 806a996a-9fcc-42b7-a902-cac954582b24
Copilot AI review requested due to automatic review settings September 16, 2026 18:52

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 review overview

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review effort: Lite
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Medium severity Handle whole quoted dotted names in the visitor

src/​tools/​ilasm/​src/​ILAssembler/​gen/​CIL.g4:411

The new direct SQSTRING alternative is not handled by GrammarVisitor.VisitDottedName: that method only special-cases DOTTEDNAME and otherwise joins dottedNamePart() values. A whole quoted name such as 'Contoso.Tools..ctor' therefore produces an empty name, so the added QuotedTypeName_WithConsecutiveDots_SplitsAtLastDot case fails and valid quoted type names are emitted incorrectly. Add an explicit context.SQSTRING() path that parses the quoted token.

Comment thread src/tools/ilasm/src/ILAssembler/EntityRegistry.cs
Comment thread src/tools/ilasm/src/ILAssembler/EntityRegistry.cs Outdated
Comment thread src/tests/JIT/Directed/Directed_do.csproj Outdated
Simplify native fat-header compatibility, conservatively preserve InitLocals through the encoder, remove redundant localloc tracking, and unify OOM exclusion descriptions.

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

Copilot-Session: 806a996a-9fcc-42b7-a902-cac954582b24
Managed ilasm exceeds practical memory limits while assembling HugeField1, so emit the same unconditional marker used by the other OOM cases.

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

Copilot-Session: 806a996a-9fcc-42b7-a902-cac954582b24
Copilot AI review requested due to automatic review settings September 17, 2026 18: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.

Copilot review overview

🟡 Changes recommended

Parser generation, quoted-name visiting, and dotted-name splitting issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)

Comment thread src/tools/ilasm/src/ILAssembler/gen/CIL.g4
<DebugType>Full</DebugType>
<Optimize>False</Optimize>
<!-- Managed ilasm consumes excessive amount of memory -->
<ManagedIlasmRoundTripIncompatible>true</ManagedIlasmRoundTripIncompatible>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like HugeField2 needs skipping too (failed in CI)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants