Add ILAst output to ilspycmd debug builds - #4013
Conversation
christophwille
left a comment
There was a problem hiding this comment.
Automated review (Claude Code, /code-review) of --ilast / --after-transform.
Built the branch and ran the ILSpyCmd.Tests suite locally (28/28 pass), then probed the built tool by hand. Conventions (headers, ASCII, TDD, Assisted-by: trailer) look clean. Ten inline findings below: the first six are correctness issues reproduced on the branch, the last four are test-strength and UI-parity/reuse.
Correctness
-t/ whole-assembly--ilastnever dumps property/event accessors (ITypeDefinition.Methodsfilters them out).ILReader.DebugInfoisn't set, so-usepdbis inert for the ILAst dump (V_0vssumin the C# output of the same command).-owith multiple inputs leaks/truncates all but the last.ilastfile (verified: 318464 vs 318957 bytes).- Only
RunTransformsis guarded; a bad method body aborts the whole dump. - Transform crashes go to stdout with exit code 0, so
--ignore-decompilation-errorshas no meaning here. - Settings pipeline runs twice (duplicate diagnostics on stderr).
Tests / parity
7-8. Two tests pass even if the feature under test regresses (header line / EX_USAGE ambiguity).
9. Both BlockILTransform entries display identically; LoopDetection is unfindable.
10. Writing options differ from the UI's ILAst pane (no field/logic sugar).
| Console.Error.WriteLine(error); | ||
| return ProgramExitCodes.EX_DATAERR; | ||
| } | ||
| methods = typeDefinition.Methods; |
There was a problem hiding this comment.
ITypeDefinition.Methods (and type.Methods in the whole-assembly branch below) is built by MetadataTypeDefinition dropping every method that has MethodSemantics, i.e. property get/set and event add/remove accessors never appear here. So -t / whole-assembly --ilast silently skips accessor bodies even though the help text promises every method.
Verified: ilspycmd ilspycmd.dll -t ICSharpCode.ILSpyCmd.ILSpyCmdProgram --ilast prints 30 methods and no get_*/set_* bodies; a transform bug in a getter is only reachable via -m M:...get_X.
Iterating the metadata handles (GetMethods() on the type handle / metadata.MethodDefinitions for the assembly case) and resolving each via MainModule.GetDefinition covers them.
| output.WriteLine($"// ILAst after {transformCount} of {TransformCount} transforms ({TransformNames[transformCount - 1]})"); | ||
|
|
||
| var reader = new ILReader(decompiler.TypeSystem.MainModule) { | ||
| UseDebugSymbols = settings.UseDebugSymbols, |
There was a problem hiding this comment.
UseDebugSymbols alone does nothing: ILReader only consults the PDB when DebugInfo is set (see CSharpDecompiler.DecompileBody, which also passes DebugInfo = DebugInfoProvider). Without it, -usepdb is inert for the ILAst dump and local names / PDB extra type info never reach the ILAst.
Verified: --ilast -usepdb -m M:...SumLoop prints V_0/V_1 while the C# output of the same command uses sum/i, so the dumped ILAst isn't what the real pipeline is fed.
var reader = new ILReader(decompiler.TypeSystem.MainModule) {
UseDebugSymbols = settings.UseDebugSymbols,
DebugInfo = decompiler.DebugInfoProvider,
...
};| if (outputDirectory != null) | ||
| { | ||
| string outputName = Path.GetFileNameWithoutExtension(fileName); | ||
| output = File.CreateText(Path.Combine(outputDirectory, outputName) + ".ilast"); |
There was a problem hiding this comment.
With -o this reassigns the shared captured output writer per input file without disposing the previous one; only the last writer is closed by OnExecuteAsync's finally, so earlier .ilast files lose their buffered tail.
Verified: ilspycmd A.dll B.dll --ilast -o out wrote out/A.ilast at 318464 bytes ending mid-line vs 318957 bytes when A.dll is dumped alone.
The DumpTable branch in this same method already handles this with a per-file using var writer = File.CreateText(...); same pattern here.
| UseDebugSymbols = settings.UseDebugSymbols, | ||
| UseRefLocalsForAccurateOrderOfEvaluation = settings.UseRefLocalsForAccurateOrderOfEvaluation, | ||
| }; | ||
| var body = metadataFile.GetMethodBody(methodDefinition.RelativeVirtualAddress); |
There was a problem hiding this comment.
Only RunTransforms is inside the try; GetMethodBody, ReadIL, CreateILTransformContext and WriteTo are unguarded and ShowILAst has no per-method catch, so a single malformed body escapes to OnExecuteAsync's catch-all and aborts the whole dump with EX_SOFTWARE. CSharpDecompiler.DecompileBody isolates BadImageFormatException per method for exactly this reason.
Scenario: whole-assembly --ilast on an obfuscated assembly with one bad body RVA stops right after that method's header line was written; everything after it is missing and the truncated file looks complete.
| // Showing how far the pipeline got is the point of this command, so a crashing | ||
| // transform prints its exception and then the partially transformed function | ||
| // rather than aborting the whole dump. | ||
| output.WriteLine(ex.ToString()); |
There was a problem hiding this comment.
Printing the partial function is useful, but the crash is written into the normal output stream and the method returns normally: ShowILAst always returns 0 and nothing goes through ReportDecompilationErrors / ExitCodeForDecompilationErrors, so --ignore-decompilation-errors has no meaning for this command and scripts can't detect the failure.
Scenario: a CI step runs ilspycmd lib.dll --ilast -o out after a transform change; a NullReferenceException in a transform is buried in out/lib.ilast, stderr is empty, exit code is 0.
Suggest also collecting the exception (or at least a count) and feeding it through the existing error-reporting path so the exit code reflects it.
| return ProgramExitCodes.EX_USAGE; | ||
| } | ||
|
|
||
| var settings = GetSettings(new PEFile(assemblyFileName)); |
There was a problem hiding this comment.
GetDecompiler already calls GetSettings, so this runs the settings pipeline a second time (opening another PEFile, repeating its stderr side effects).
Verified: --ilast -ds Bogus=1 -m ... prints "Decompiler setting 'Bogus' is unknown." twice; a broken --ilspy-settingsfile is reported twice as well.
Build the settings once and hand them to the decompiler, or read them back from decompiler.Settings.
| Assert.That(full.ExitCode, Is.EqualTo(0), full.Error); | ||
| Assert.That(partial.ExitCode, Is.EqualTo(0), partial.Error); | ||
| Assert.That(partial.Output, Does.Contain(nameof(ILAstSample.SumLoop))); | ||
| Assert.That(partial.Output, Is.Not.EqualTo(full.Output)); |
There was a problem hiding this comment.
This (and TransformCanBeSelectedByIndex) only asserts partial.Output != full.Output, but WriteMethod's header line embeds the requested count (// ILAst after 3 of 38 transforms (ILInlining)), so the outputs differ even if --after-transform stopped nothing. If transforms.Take(transformCount) regressed to running the full pipeline, both tests would still pass.
Assert on body content instead, e.g. that the partial output has no loop/structured block after ILInlining while the full one does, or on the header text plus a body property.
|
|
||
| Assert.That(result.ExitCode, Is.EqualTo(ProgramExitCodes.EX_USAGE)); | ||
| Assert.That(result.Error, Does.Contain("SplitVariables")); | ||
| Assert.That(result.Error, Does.Contain("2")); |
There was a problem hiding this comment.
The unknown-name and out-of-range branches also return EX_USAGE and print the pipeline listing, which contains "SplitVariables" and the digit "2", so this test can't distinguish the ambiguity message from the others. Dropping the occurrences.Length > 1 branch (falling through to "Unknown transform") keeps it green.
Assert on something specific to the ambiguity path, e.g. Does.Contain("runs 3 times") or the exact index list.
| /// after individually. | ||
| /// </summary> | ||
| public static IReadOnlyList<string> TransformNames { get; } = | ||
| CSharpDecompiler.GetILTransforms().Select(t => t.GetType().Name).ToArray(); |
There was a problem hiding this comment.
GetType().Name makes both BlockILTransform entries appear as identical bare BlockILTransform rows in DescribePipeline, in the ambiguity error and in the dump header, and LoopDetection/ConditionDetection are never mentioned anywhere.
Verified: --after-transform LoopDetection -> "Unknown transform" with a listing that never contains LoopDetection; --after-transform BlockILTransform -> "runs 2 times, at index 22, 25" with no way to tell which one holds ConditionDetection.
ILFunction.RunTransforms already labels these entries with BlockILTransform.ToString() (which lists the nested names); reusing t is BlockILTransform b ? b.ToString() : t.GetType().Name for display fixes all three places.
| class ILAstDumper | ||
| { | ||
| readonly IReadOnlyList<IILTransform> transforms = CSharpDecompiler.GetILTransforms(); | ||
| readonly ILAstWritingOptions writingOptions = new ILAstWritingOptions(); |
There was a problem hiding this comment.
The UI's ILAst language writes with UseFieldSugar = true, UseLogicOperationSugar = true (DebugStepsPaneModel.WritingOptions), while this uses the defaults (both false). So the same method renders ldobj(ldflda ...) / nested if-blocks on the CLI vs ldfld / logic.and in the UI pane; the "same representation as the UI" claim in the class doc doesn't hold and UI-vs-CLI diffs are noisy.
One-line fix: initialize the options with both sugars true (or expose switches).
The decompiler's intermediate representation was reachable only through the GUI's ILAst language, so anyone debugging a transform or a matcher had to do it by hand in the UI. --ilast writes the same representation to stdout, and --after-transform truncates the pipeline at a chosen point, which makes the effect of a single transform diffable and scriptable. Debug-only, like the UI language it mirrors: ILAst serves ILSpy's own development, not the users of the released tool, so it stays out of the shipped NuGet package and out of the README's option list. Nested per-block transforms stay unaddressable: they run inside a BlockILTransform entry, and reaching them needs the Stepper, which is compiled out of release builds anyway. Assisted-by: Claude:claude-opus-5[1m]:Claude Code
The dump is only useful if it shows the same thing the real pipeline is fed and covers everything a transform bug can hide in, so: accessor bodies are reached through the metadata handles (ITypeDefinition.Methods hides every method that has method semantics), the PDB reaches the ILReader (UseDebugSymbols alone is inert without DebugInfo), and the writing options carry the same sugar as the UI's ILAst pane. A method whose body cannot be read, transformed or written no longer aborts the run, and the failure now travels through the decompilation-error path, so a crashing transform is visible in stderr and in the exit code instead of being buried in the output a script just collected. BlockILTransform entries name the transforms they contain: two of them ran as identical rows before, and asking for a nested transform by name reported it as unknown while the listing did in fact run it. Assisted-by: Claude:claude-opus-5[1m]:Claude Code
373231c to
0566a81
Compare
The decompiler's intermediate representation was only reachable through the GUI's ILAst
language, so debugging a transform or a matcher meant driving the UI by hand.
--ilastwritesthe same representation to stdout, and
--after-transform <name-or-index>truncates the pipelineat a chosen point, which makes the effect of a single transform diffable and scriptable:
What is dumped follows the usual selection options:
-ma single method,-tevery method of atype, neither one every method of the assembly. Methods are enumerated over the metadata handles
rather than
ITypeDefinition.Methods, so property and event accessors are included.-usepdbreaches the
ILReader, so the dump carries the same local names the real pipeline sees, and-owrites one
<assembly>.ilastper input file.A method whose body cannot be read, transformed or written does not abort the run: the partial
function is printed with the exception, and the failure travels through the same
decompilation-error path as the other commands, so it shows up on stderr and in the exit code
unless
--ignore-decompilation-errorsis passed.--after-transformtakes a transform name or a 1-based pipeline index, and prints the pipelinewhen it cannot resolve one. Entries that are a
BlockILTransformlist the transforms they run,which is both what tells two such entries apart and what makes the nested transforms visible;
asking for one of those by name reports which entry runs it. Stopping after a nested transform
individually still needs the
Stepper, which is compiled out of release builds anyway.The writing options match the UI's ILAst pane (field and logic-operation sugar), so output from
the two can be diffed directly.
Both options are
#if DEBUG-only, like the UI language they mirror: ILAst serves ILSpy's owndevelopment, not users of the released tool, so they stay out of the shipped NuGet package and
out of the README's option list.
Covered by 9 tests in
ICSharpCode.ILSpyCmd.Tests/ILAstOptionTests.cs(31 in the project); fullsolution builds clean.
Written by an AI agent (Claude) on Siegfried's behalf.