Uh oh!
There was an error while loading. Please reload this page.
Improve managed ilasm test coverage - #131509
Open
jkoritzinsky wants to merge 14 commits into
Open
Conversation
|
Azure Pipelines: 16 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
jkoritzinsky
marked this pull request as ready for review
July 28, 2026 23:33
|
Azure Pipelines: 16 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR significantly expands the managed ilasm unit test suite to validate emitted metadata/PE structures (exports, resources, PDBs, signatures, marshalling, etc.), and adjusts the managed ilasm emitter/parser to satisfy the newly-covered scenarios.
Changes:
- Add many new ILAssembler.Tests cases that decode emitted metadata/PE/PDB and validate specific directives and operand forms.
- Fix/extend ilasm implementation details in
GrammarVisitor(e.g., custom attribute serialization,.modulehandling, sequence points, callconv explicit/instance) andVTableExportPEBuilder(vtable fixup directory offset + export table layout). - Improve test infra helpers to compile IL text and decode IL token operands/signatures more easily.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/ilasm/tests/ILAssembler.Tests/VTableTests.cs | Minor formatting-only change. |
| src/tools/ilasm/tests/ILAssembler.Tests/TypeSignatureTests.cs | Add tests for multidimensional array local signature shapes. |
| src/tools/ilasm/tests/ILAssembler.Tests/TypeReferenceTests.cs | Add tests for .this/.base/.nester/mdtoken type refs, module-scoped refs, typelist parsing. |
| src/tools/ilasm/tests/ILAssembler.Tests/TypedefTests.cs | Add tests for .typedef aliases for fields and custom attributes. |
| src/tools/ilasm/tests/ILAssembler.Tests/TypeDefinitionTests.cs | Add tests for type name splitting, class flags/attributes, nested types, interface impl metadata. |
| src/tools/ilasm/tests/ILAssembler.Tests/SourceDirectiveTests.cs | Strengthen .line/.language coverage and validate emitted embedded PDB details + string escape correctness. |
| src/tools/ilasm/tests/ILAssembler.Tests/SecurityTests.cs | Add extensive .permissionset emission/shape tests (incl. tolerant-mode behavior). |
| src/tools/ilasm/tests/ILAssembler.Tests/PropertyTests.cs | Add property attribute/accessor/signature metadata validation. |
| src/tools/ilasm/tests/ILAssembler.Tests/PropertyEventTests.cs | New test file covering PropertyMap/EventMap + MethodSemantics emission. |
| src/tools/ilasm/tests/ILAssembler.Tests/PreprocessorIntegrationTests.cs | New integration tests for nested includes, conditional compilation, multi-token macros. |
| src/tools/ilasm/tests/ILAssembler.Tests/ParameterTests.cs | Add tests for parameter attribute forms and raw flag parsing. |
| src/tools/ilasm/tests/ILAssembler.Tests/NativeExportTests.cs | New tests validating when exports are (not) emitted and verifying export directory/stubs. |
| src/tools/ilasm/tests/ILAssembler.Tests/ModuleTests.cs | Add tests for .file, .module extern, bare .module, and global method/module metadata + PDB. |
| src/tools/ilasm/tests/ILAssembler.Tests/MethodTests.cs | Add tests for method attributes/impl flags, method body directives, entrypoint token, override forms. |
| src/tools/ilasm/tests/ILAssembler.Tests/MemberReferenceTests.cs | Add tests for vararg MemberRef signatures, generic-instantiation MemberRefs, method spec emission. |
| src/tools/ilasm/tests/ILAssembler.Tests/ManifestResourceTests.cs | New tests for embedded/assembly/file-backed manifest resources + tolerant/deterministic behaviors. |
| src/tools/ilasm/tests/ILAssembler.Tests/InteropTests.cs | Add marshalling and P/Invoke import metadata tests (incl. complex native type payloads). |
| src/tools/ilasm/tests/ILAssembler.Tests/InstructionTests.cs | Add coverage for float bit-pattern literals, calli sigs, ldstr forms, switch table forms, token operands. |
| src/tools/ilasm/tests/ILAssembler.Tests/GenericTests.cs | Add generic parameter name/bounds/variance/custom attribute/constraint metadata tests. |
| src/tools/ilasm/tests/ILAssembler.Tests/FunctionPointerTests.cs | Update function pointer signature assertions to use decoded signatures; add vararg fnptr test. |
| src/tools/ilasm/tests/ILAssembler.Tests/FieldTests.cs | Add tests for field attribute flags, volatile/literal/nullref constant emission. |
| src/tools/ilasm/tests/ILAssembler.Tests/ExportedTypeTests.cs | Add tests for exported type attributes/implementations + custom attributes. |
| src/tools/ilasm/tests/ILAssembler.Tests/ExceptionHandlingTests.cs | Add tests validating EH region bounds/kinds for multiple clause forms. |
| src/tools/ilasm/tests/ILAssembler.Tests/EventTests.cs | Add tests for event raise accessor, event “other” accessor, and attribute emission. |
| src/tools/ilasm/tests/ILAssembler.Tests/DocumentCompilerTests.cs | New test validating diagnostics pathing across multi-document compilation in tolerant mode. |
| src/tools/ilasm/tests/ILAssembler.Tests/DocumentCompilerTestHelpers.cs | Add decoder + richer compile helpers and IL token operand extraction. |
| src/tools/ilasm/tests/ILAssembler.Tests/DataTests.cs | Add tests for .data directive typed byte emission, repeats, and class-scoped .data. |
| src/tools/ilasm/tests/ILAssembler.Tests/CustomAttributeTests.cs | Add broad structured custom attribute decoding tests (arrays, boxed values, owner targeting, type args). |
| src/tools/ilasm/tests/ILAssembler.Tests/CompilerOptionsTests.cs | New tests validating Options-driven PE/header/corflags/debug/pdb/determinism/keyfile behaviors. |
| src/tools/ilasm/tests/ILAssembler.Tests/AssemblyTests.cs | Add tests for assembly reference identity metadata and aliased reference/assembly attributes. |
| src/tools/ilasm/src/ILAssembler/VTableExportPEBuilder.cs | Fix COR header vtable fixup directory offset; refactor vtable slot serialization and export table layout. |
| src/tools/ilasm/src/ILAssembler/GrammarVisitor.cs | Numerous parser/emitter fixes to support new metadata/PDB/custom-attribute/directive scenarios. |
Comments suppressed due to low confidence (1)
src/tools/ilasm/src/ILAssembler/GrammarVisitor.cs:2924
- Same as above:
ToImmutable()copies;MoveToImmutable()can transfer the backing array without allocating, which is preferable in a potentially hot parsing path.
return new(builder.ToImmutable().SerializeSequence());
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Contributor
Tagging subscribers to this area: @JulieLeeMSFT, @dotnet/jit-contrib |
jkoritzinskyforce-pushed
the
ilasm-more-tests
branch
from
July 29, 2026 00:05
0223376 to
15983d0CompareContributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/tools/ilasm/tests/ILAssembler.Tests/TypeSignatureTests.cs:92
- This test assumes the locals signature is always
StandAloneSigrow 1. Using the method body'sLocalSignaturehandle avoids tying the assertion to metadata row ordering.
var signature = reader.GetStandaloneSignature(MetadataTokens.StandaloneSignatureHandle(1));
src/tools/ilasm/tests/ILAssembler.Tests/TypeSignatureTests.cs:125
- Hard-coding
StandaloneSignatureHandle(1)here couples the test to metadata emission order. Use the method body'sLocalSignatureto make the test robust against unrelated StandAloneSig additions.
var signature = reader.GetStandaloneSignature(MetadataTokens.StandaloneSignatureHandle(1));
src/tools/ilasm/tests/ILAssembler.Tests/TypeSignatureTests.cs:66
- This test hard-codes
StandaloneSignatureHandle(1), which makes it brittle if the compiler ever emits another StandAloneSig row before the locals signature. Prefer using the method body'sLocalSignaturehandle so the test is resilient to unrelated metadata changes.
This issue also appears in the following locations of the same file:
- line 92
- line 125
var signature = reader.GetStandaloneSignature(MetadataTokens.StandaloneSignatureHandle(1));
jkoritzinskyforce-pushed
the
ilasm-more-tests
branch
from
July 29, 2026 00:34
15983d0 to
3719330CompareContributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/tools/ilasm/src/ILAssembler/GrammarVisitor.cs:5573
- In structured custom attribute encoding, a boxed
object(...)value must include theSerializationTypeCode.TaggedObject(0x51) prefix, followed by the embedded value’s FieldOrPropType and value bytes. The currentobject(...)branch writes only the embedded type/value and omits the TaggedObject prefix, which will make decodingobjectarguments invalid (and can breakobject[]element encoding too).
if (context.serInit() is CILParser.SerInitContext serInit)
{
Debug.Assert(context.OBJECT() is not null);
BlobBuilder taggedObjectBlob = new();
WriteCustomAttributeFieldOrPropType(taggedObjectBlob, serInit);
taggedObjectBlob.LinkSuffix(VisitSerInit(serInit).Value);
return new(taggedObjectBlob);
Uh oh!
There was an error while loading. Please reload this page.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/tools/ilasm/tests/ILAssembler.Tests/DocumentCompilerTestHelpers.cs:123
GetTokenOperandsreads the method body without validatingRelativeVirtualAddress > 0, so callers get an exception fromPEReader.GetMethodBodyinstead of a clear test failure message when the target method has no body (e.g., runtime/abstract methods). This helper already has a good pattern inGetFirstTokenOperand; it should mirror that assert for consistency and debuggability.
var method = reader.MethodDefinitions
.Select(reader.GetMethodDefinition)
.First(definition => reader.GetString(definition.Name) == methodName);
byte[] il = pe.GetMethodBody(method.RelativeVirtualAddress).GetILBytes()!;
var operands = ImmutableArray.CreateBuilder<int>();
jkoritzinskyforce-pushed
the
ilasm-more-tests
branch
from
August 24, 2026 21:28
e6fc74b to
2b7777dCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
jkoritzinskyforce-pushed
the
ilasm-more-tests
branch
from
August 26, 2026 20:30
2b7777d to
86bd0c2CompareContributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 45 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
src/tools/ilasm/src/ILAssembler/EntityRegistry.cs:418
- The fallback
AddMethodBody(...)paths also OR inMethodBodyAttributes.InitLocalswhenforceFatHeaderis true. For the same reason as the main path, this can set InitLocals even when it wasn’t requested; it should be sufficient to pass the existing attributes and usehasDynamicStackAllocationto prevent a tiny header.
localsSigHandle,
forceFatHeader ? methodDef.BodyAttributes | MethodBodyAttributes.InitLocals : methodDef.BodyAttributes,
hasDynamicStackAllocation: methodDef.HasDynamicStackAllocation || forceFatHeader);
Comment on lines
+393
to
+404
| MethodBodyAttributes encodingAttributes = methodDef.BodyAttributes; | ||
| if (forceFatHeader) | ||
| { | ||
| encodingAttributes |= MethodBodyAttributes.InitLocals; | ||
| } | ||
| bodyOffset = bodyStreamEncoder.AddMethodBody( | ||
| methodDef.MethodBody, | ||
| methodDef.MaxStack, | ||
| localsSigHandle, | ||
| methodDef.BodyAttributes); | ||
| encodingAttributes, | ||
| methodDef.HasDynamicStackAllocation || forceFatHeader); |
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+18
to
+29
| <PropertyGroup> | ||
| <!-- Keep these NativeAOT runtime settings in sync with the first PropertyGroup in AotCompilerCommon.props. --> | ||
| <ServerGarbageCollection>true</ServerGarbageCollection> | ||
| <TieredCompilation>false</TieredCompilation> | ||
| <EventSourceSupport>true</EventSourceSupport> | ||
| <OptimizationPreference>Speed</OptimizationPreference> | ||
| <ControlFlowGuard>Guard</ControlFlowGuard> | ||
| <InvariantGlobalization>true</InvariantGlobalization> | ||
| <StripSymbols Condition="'$(KeepNativeSymbols)' == 'true'">false</StripSymbols> | ||
| <XmlResolverIsNetworkingEnabledByDefault>false</XmlResolverIsNetworkingEnabledByDefault> | ||
| <StackTraceLineNumberSupport>true</StackTraceLineNumberSupport> | ||
| </PropertyGroup> |
Member
There was a problem hiding this comment.
This looks like valid feedback to me. Why do we need to have this copy?
MemberAuthor
There was a problem hiding this comment.
AotCompilerCommon.props also copies all of the JITs, which was exacerbating some of the CI failures I was seeing on the ilasm pipeline. I'll refactor out AotCompilerCommon.props into an "AotTool.props" that contains the AOT config settings and keep the original file for the info to copy JITs.
jkoritzinskyforce-pushed
the
ilasm-more-tests
branch
from
August 26, 2026 20:46
86bd0c2 to
a30e6e2Compare
Comment on lines
+524
to
528
| for (int ordinal = baseOrdinal; ordinal <= maxOrdinal; ordinal++) | ||
| { | ||
| int stubIndex = Array.FindIndex(exportsArray, e => e.Ordinal == export.Ordinal); | ||
| builder.WriteInt32(exportStubRvas[stubIndex]); | ||
| int stubIndex = Array.FindIndex(exportsArray, export => export.Ordinal == ordinal); | ||
| builder.WriteInt32(stubIndex >= 0 ? exportStubRvas[stubIndex] : 0); | ||
| } |
Use content-based portable PDB IDs for deterministic builds and add in-process coverage for byte-identical PDB output and non-empty PDB ID GUIDs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fb9f1ef-f721-49e0-bf48-4a7a9458d53a
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fb9f1ef-f721-49e0-bf48-4a7a9458d53a
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fb9f1ef-f721-49e0-bf48-4a7a9458d53a
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fb9f1ef-f721-49e0-bf48-4a7a9458d53a
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fb9f1ef-f721-49e0-bf48-4a7a9458d53a
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 106f96fe-da32-41d4-8aa5-7431edb36fc6
…ompiler, it's a compiler that's AOT compiled)
Add feature-focused metadata and PE validation tests and fix emitter issues exposed by them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4307156b-37a7-495d-9af6-fe9174c05b0b
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
… attribute encoding When an object(...) element in an object[] custom attribute had nested object() wrappers (e.g. object(object(bool(true)))), VisitSerInit's object() branch would call WriteCustomAttributeFieldOrPropType on the inner object() context, producing TaggedObject (0x51) as the element type code. The .NET metadata decoder does not accept 0x51 as a FieldOrPropType for object[] elements and throws BadImageFormatException. Fix VisitObjSeq to fully unwrap any object(...) nesting using a while loop, so that WriteCustomAttributeFieldOrPropType is always called with the innermost concrete typed element (bool, int32, string, etc.). This also makes type-byte handling uniform - always done by VisitObjSeq rather than split between caller and callee. Add a regression test CustomAttribute_ObjectArrayWithNestedObjectWrapper _DecodesProperly that exercises object[](object(bool(true)) object(int32 (42))) and verifies the attribute decodes correctly.
Use comma separators between double-quoted GUID arguments so the tests match the supported .language grammar. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: adc580d5-8f1c-4772-bfc2-7a943aa0959d
Pre-size floating-point sequence builders and transfer their arrays without copying. Handle empty floating-point and string sequences and cover their custom attribute encoding. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: adc580d5-8f1c-4772-bfc2-7a943aa0959d
jkoritzinskyforce-pushed
the
ilasm-more-tests
branch
from
August 26, 2026 22:04
a30e6e2 to
67af88aComparejkotas
reviewed
Aug 29, 2026
| foreach (GenericParameterEntity genericParam in GetSeenEntities(TableIndex.GenericParam)) | ||
| { | ||
| // GenericParam index is stored as a 2-byte value; skip params beyond the limit | ||
| // COMPAT: Native ilasm ignores generic parameters whose indices exceed the 2-byte metadata limit. |
Member
There was a problem hiding this comment.
Should this fail with an error instead?
I do not think we need to preserve every native ilasm bug. We should fix these bugs as we run into them.
MemberAuthor
There was a problem hiding this comment.
My goal was to get us to a clean ilasm round trip run without adding support for a "managed ilasm incompatible" flag. I'll add such a flag for the test case that fails because of this (and revert this compat quirk) so we can review if the test is interesting as-is.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add feature-focused metadata and PE validation tests and fix emitter issues exposed by them.
With these unit tests (amongst others added in the stack) we can have more confidence in the managed ilasm implementation. In the future, we may even be able to move ilasm round-trip testing to an "outerloop" experience for ilasm instead of being the primary validation pipeline.
This PR also includes fixes and corresponding tests for failures identified with ildasm/ilasm round-trip testing (all except one, which is addressed by #132346).
Stack created with GitHub Stacks CLI • Give Feedback 💬
Note
This PR description was generated with GitHub Copilot.