[am11]

• Add src/tools/ildasm with CLI, library, and test projects
• Implement all 218 IL opcodes in InstructionDisassembler
• Add Options class and wire CLI arguments to library
• Output basic assembly/module/type/method structure
• Add TODOs for remaining work (token resolution, etc.)

[jkoritzinsky]

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.

[jkotas]

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.

[am11]

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?

[stephentoub]

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

[am11]

they are not small.

It's ~10 additional classes and if we skip 1-2MB boilerplate, that'd make sense? (plus the exemption of a separate Generating native image step)

[jkotas]

We are shipping ilasm/ildasm/ilverify as separate tools packages. I think we want to continue doing that. If we were to build all these tools into one binary, we would be shipping dead weight in each of the packages. It does not make sense to me.

[am11]

If we were to build all these tools into one binary, we would be shipping dead weight in each of the packages.

The size measurements suggest that dead weight would be a small fraction #128553 (comment) (5.5+3.1+4 vs. 6.2 MB).

[jkotas]

Why would we want to ship any dead weight in these packages in the first place? This size optimizations makes sense only for binaries that ship bundled in the SDK. It does not make sense for tools that ship as independent packages.

[am11]

We could alternatively ship these tools together in a single "iltools" NuGet package.

As for granularity, these tools already feature dozens of switches where most users only utilize a fraction of the functionality. The rest is essentially dead weight for their repeated usage anyway, so adding a few more AOT-compiled classes changes very little.

(Note: Avoiding the few seconds of overhead from each Generate native image step during runtime builds is also a nice side-effect, though not the main driver here).

[dotnet-policy-service]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[jkoritzinsky]

@am11 we're just about finished with managed ilasm, so I'm starting to look at managed ildasm now. Can you merge main into this PR and resolve any conflicts and I'll get it in so we can get started on this?

[azure-pipelines]

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

[am11]

@jkoritzinsky merged main and synced the skeleton with managed ilasm: same project/packaging layout (ildasm.props, ildasm_inbuild.csproj, Microsoft.NETCore.ILDAsm.Package.csproj) and the same native command line handling. Both tools build under a new tools.iltools subset; ildasm stays out of clr.tools/clr.packages for now since Microsoft.NETCore.ILDAsm still ships the native one. While at it, also fixed NativeAotToolPackage.targets on Unix: NuGet treats an extensionless PackagePath as a folder, so the ilasm package had the binary at runtimes/<rid>/native/ilasm/ilasm.

[jkotas]

ildasm_inbuild.csproj

Do we actually need ildasm_inbuild for ildasm?

[am11]

Do we actually need ildasm_inbuild for ildasm?

It's for parity with ilasm: Microsoft.NET.Sdk.IL restores both runtime.<host-rid>.Microsoft.NETCore.ILAsm and .ILDAsm, so once the managed ildasm replaces the native package in coming days, the Windows cross-build legs (BuildHostILTools) need the host-RID ildasm package the same way. The native pkgproj covers that today via builds.targets.

[jkoritzinsky]

@am11 can you make a separate PR with the NativeAotToolPackage.targets fixes so we can get that in faster?

[am11]

Opened #136755 for the NativeAotToolPackage.targets fix.

[am11]

@jkoritzinsky, shall we move forward with this structure? I can provide matching implementation to native ildasm after this goes in.

[jkoritzinsky]

A few nits but otherwise LGTM