Let's wait on ildasm until we finish ilasm.

In any case, I was thinking that we should build ildasm on the managed type system and the ILImporter infrastructure over there so we can reuse a bunch of code and have less to maintain in the future.

build ildasm on the managed type system

Managed type system expects metadata for all dependencies that is not compatible with how ildasm works. You would be constantly fighting against that.

BTW: We have a little managed IL disassembler as a debugging aid: https://github.com/dotnet/runtime/blob/main/src/coreclr/tools/Common/TypeSystem/IL/ILDisassembler.cs . It was not built using ILImporter since it would make it a lot more complicated.

I was able to rewire it with:
<Import Project="$(CoreClrProjectRoot)tools\ILVerification\ILVerification.projitems" />

https://github.com/am11/runtime/tree/feature/managed-ildasm2

It's a bit cleaner. Do you want this S.R.M version or push that one?

🤖 Copilot Code Review — PR #123848

Holistic Assessment

Motivation: Adding a managed ildasm tool complements the recently merged managed ilasm (#122112) and provides cross-platform IL inspection capabilities. The motivation is sound.

Approach: The project structure (library + CLI + tests) mirrors the existing ilasm tool, which is good for consistency. Using System.Reflection.Metadata and System.Reflection.PortableExecutable is the right choice for managed PE/metadata access.

Summary: ⚠️ Needs Human Review. This is a solid foundation for a managed ildasm, but there are correctness issues that need attention before merge. The PR is honest about its incompleteness (many TODOs), but some existing code has bugs that will produce incorrect output.

Detailed Findings

❌ Branch Target Calculation — ECMA-335 Violation

InstructionDisassembler.cs lines ~578-592

Per ECMA-335 §III.1.7.1, branch offsets are relative to the beginning of the next instruction, not the operand position. The current implementation captures offset before reading the operand:

private static string FormatBranchShort(string mnemonic, ref BlobReader reader)
{
    int offset = reader.Offset;  // Position of operand
    sbyte delta = reader.ReadSByte();
    int target = offset + delta;  // Bug: should be (offset + 1) + delta
    return $"{mnemonic} IL_{target:X4}";
}

Fix:

private static string FormatBranchShort(string mnemonic, ref BlobReader reader)
{
    sbyte delta = reader.ReadSByte();
    int target = reader.Offset + delta;  // reader.Offset is now past the operand
    return $"{mnemonic} IL_{target:X4}";
}

Same issue in FormatBranchLong and FormatSwitch. This affects all branch instructions.

❌ Resource Management — Stream Leak

Disassembler.cs lines ~51-56

The file stream is opened but never tracked for disposal:

var stream = File.OpenRead(filePath);
_peReader = new PEReader(stream);

PEReader(Stream) by default takes ownership of the stream and will dispose it. However, looking at the PEReader source, the default PEStreamOptions is None, which means it does take ownership. I need to verify this is correct.

Actually, after checking: PEReader(stream) with no options defaults to taking ownership. So this might be okay, but it would be clearer to make this explicit:

_peReader = new PEReader(stream, PEStreamOptions.Default);

💡 Suggestion: Add a comment or use explicit options to clarify stream ownership.

⚠️ Unused Options — Dead Code

Disassembler.cs

The _options field is stored but never used in any disassembly method. This is misleading since the Options class defines 20+ properties (ShowBytes, ShowTokens, etc.) that would affect output.

Action: Either wire up the options to control behavior, or remove the field and add a TODO noting that options support is not yet implemented.

⚠️ TypeDisassembler.GetModifiers() — Hardcoded "auto ansi"

TypeDisassembler.cs lines ~1001-1028

The method always appends "auto ansi " regardless of actual TypeAttributes.StringFormatMask:

result += "auto ansi ";  // Always hardcoded

Should check the actual string format:

var layout = (attributes & TypeAttributes.LayoutMask) switch
{
    TypeAttributes.AutoLayout => "auto ",
    TypeAttributes.SequentialLayout => "sequential ",
    TypeAttributes.ExplicitLayout => "explicit ",
    _ => ""
};

var stringFormat = (attributes & TypeAttributes.StringFormatMask) switch
{
    TypeAttributes.AnsiClass => "ansi ",
    TypeAttributes.UnicodeClass => "unicode ",
    TypeAttributes.AutoClass => "autochar ",
    _ => ""
};

⚠️ Incomplete Output — Missing Types

TypeDisassembler.cs

1. WriteField() outputs .field {visibility}{modifiers}{name} but missing the field type. Valid IL requires the type.

2. WriteMethod() outputs .method {visibility}{modifiers}{name}() cil managed but missing return type and parameter types.

These are marked with TODOs in the code, which is fine for an initial scaffolding PR, but the output is not valid IL syntax without them.

⚠️ Test Coverage Gaps

InstructionDisassemblerTests.cs

1. new InstructionDisassembler(null!) — passes null MetadataReader which will crash on any token-resolving instruction (ldstr, call, newobj, etc.).

2. No tests for branch instructions — would have caught the offset calculation bug.

3. No tests for switch instruction.

Recommendation: Add at least one branch instruction test with a known target to verify offset calculation:

// br.s with delta=+3 from offset 1 (opcode at 0, operand at 1, next at 2)
// Target should be 2 + 3 = 5
[InlineData(new byte[] { 0x2B, 0x03 }, "br.s IL_0005")]

💡 IldasmRootCommand Option Syntax

IldasmRootCommand.cs

The Option constructor syntax:

new Option<string?>("-o", "--output") { Description = "..." }

This uses C# 12 collection initializer syntax for aliases. Verify this works with the version of System.CommandLine referenced. The more traditional pattern is:

new Option<string?>(new[] { "-o", "--output" }, "Description")

💡 Consider IsAotCompatible for CLI Project

ildasm.csproj

The library project has <IsAotCompatible>true</IsAotCompatible> but the CLI project does not. Consider adding it for consistency if the tool should support NativeAOT deployment.

✅ Positive Observations

1. Good project structure: Matches the existing ilasm tool layout
2. Comprehensive opcode coverage: All 218 IL opcodes are handled in the switch
3. Appropriate use of S.R.Metadata: Uses the right APIs for managed PE access
4. Solution file uses slnx format: Modern solution format
5. Pipeline updated: Correctly adds the new folder to the ilasm CI trigger

Summary

This is a reasonable first step toward a managed ildasm. The critical issue is the branch target calculation bug which produces incorrect output for all branch instructions. The other issues (incomplete output, unused options) are acknowledged TODOs and acceptable for initial scaffolding.

Recommended actions before merge:

1. Fix branch offset calculation in FormatBranchShort, FormatBranchLong, FormatSwitch
2. Add a branch instruction test to prevent regression
3. Consider clarifying stream ownership or adding explicit PEStreamOptions

Review generated with multi-model analysis (Claude Opus 4.5 contributed). GPT-5.2 and Gemini agents timed out or did not produce results.