Improve control flow decompilation (fixes #1133) #1176

Chicken-Bones · 2018-06-12T09:17:10Z

A solution to #1133 involving repeated inlining, and block exit merging with appropriate keyword prioritisation

Add a ControlFlowSimplification step after SplitVariables Enable dead code removal in some unit tests

dgrunwald · 2018-06-12T19:51:58Z

I haven't had time to look at the code yet; but I compared the decompiler output with a few test assemblies, and it's clear that this is a massive improvement. Thanks!

Total number of gotos in the roundtrip tests is down from 781 to 200.
I'll try to get this reviewed ASAP so that we can include this in the 3.2 release.

Chicken-Bones · 2018-06-12T20:46:01Z

That's great to hear. Sorry it took so long, I had to put it on hold for a week and a half as finals hit for uni.

I'm not sure what your release schedule is, but there's two more pieces coming, related to switch reconstruction, one of which I can have done in 2 days time.

You'll notice that part of the new ConditionDetection involves guessing Branch targets which are continue keywords, as those have different ordering priorities. I'll need to do something similar to switch detection, which means separating it from and moving it after LoopDetection. This would produce another BlockTransform phase, and I don't understand the system enough to know the performance or flow impacts yet. What would you suggest?

dgrunwald · 2018-06-13T20:22:33Z

Oh right, switch blocks can have two exits (break+continue). Currently it's using the loop logic for finding a single exit point, so that can't work correctly.
But whatever the chosen exit point for switch is, it can always be reached via break;, so the ConditionDetection logic based on an exit point at the end of the block doesn't apply either.
But the transform ordering is tricky, since a single "continue-block" only exists after the big BlockTransform combined multiple blocks into a single block (consider a continue block involving ?. or array initializers...).
But I don't think continue; within switch within a for loop occur frequently enough that we should care about this case... (the more common continue; within switch within while/foreach loops should also be possible with the current LoopDetection-based design for switch)

dgrunwald

Overall: I like the approach, and think it works great.
Thanks a lot for your work here! I'll merge the PR and add a commit of my own on top of it (with purely cosmetic changes I made during the review).

Most of the remaining gotos in the round trip tests are in some way related to switch. There seem to be some bugs in the logic that determines the switch body (particularly with nested switches); but there's also a lot of tricky cases where we over-aggressively detect a switch and cause problems for later pipeline stages. I have no idea on how to solve those (though we could improve things somewhat by fine-tuning the sparse-switch heuristic).
Some other gotos are caused by us not inlining generated value temporaries (keyword: STRUCT_SPLITTING_IMPROVED), which in turn makes it impossible to reconstruct the short-circuiting operators. Properly solving that will first require support for ref-like types...
I didn't see any where the goto would have been avoidable if ConditionDetection made better choices.

dgrunwald · 2018-06-13T19:23:07Z

ICSharpCode.Decompiler.Tests/PrettyTestRunner.cs

@@ -106,7 +106,8 @@ public void ShortCircuit([ValueSource("defaultOptions")] CSharpCompilerOptions c
 		public void ExceptionHandling([ValueSource("defaultOptions")] CSharpCompilerOptions cscOptions)
 		{
 			RunForLibrary(cscOptions: cscOptions, decompilerSettings: new DecompilerSettings {
-				NullPropagation = false
+				NullPropagation = false,
+				RemoveDeadCode = !cscOptions.HasFlag(CSharpCompilerOptions.UseRoslyn)
 			});


RemoveDeadCode was introduced for F# (which emits much more dead code than C#).
It's not enabled by default, so it feels weird to have to use it in the C# tests.

As far as I can tell, it's only for while (true) loops compiled with legacy csc in debug mode?
I guess it's fine to special-case this for the test case... but maybe it would be better to have a transform that removes this specific kind of dead store independent of the RemoveDeadCode configuration option.

A transform would be better, I'll leave that one for you

dgrunwald · 2018-06-13T19:25:34Z

ICSharpCode.Decompiler/IL/ControlFlow/ConditionDetection.cs

+						return forIncrement;
+				} catch (InvalidOperationException) {
+					// multiple potential increment blocks. Can get this because we don't check that the while loop
+					// has a condition first, as we don't need to do too much of HighLevelLoopTransform's job.


GetIncrementBlock should be changed to return null instead of throwing an exception.

I originally had it return null, but the code looked ugly in comparison, and wasn't necessary inside HighLevelLoopTransform because of preconditions. Avoiding exceptions as part of normal control flow is a design principle I'm aware of, so I'll favour that.

dgrunwald · 2018-06-13T19:58:06Z

ICSharpCode.Decompiler/IL/ControlFlow/ConditionDetection.cs

+			//save a copy
+			var trueInst = ifInst.TrueInst;
+
+			if (ifInst != block.Instructions.SecondToLastOrDefault()) {


Moving instructions from the current block into a nested block is potentially problematic, because the following block transforms apply only to the current block, any nested blocks are expected to be already completely transformed.
But I guess it's OK in this case because it's only moving instructions that were previously inlined from another block.

Good point, works out for us here.

dgrunwald · 2018-06-13T20:04:14Z

ICSharpCode.Decompiler/IL/ControlFlow/ConditionDetection.cs

+			if (exitInst is Branch branch
+			    && branch.TargetBlock.Parent == currentContainer
+			    && branch.TargetBlock.IncomingEdgeCount == 1
+			    && branch.TargetBlock.FinalInstruction is Nop) {


After the old ConditionDetection was originally written, we strengthened the Block invariants to allow a non-Nop FinalInstruction only for the inline block types:

branch.TargetBlock must be directly within a container (via Branch invariant)

Any block in a container must have type ControlFlow (via BlockContainer invariant)

ControlFlow blocks can't have a FinalInstruction (via Block invariant)
So these checks are redundant now. I'll modify the code to replace them with an assertion.

Although, this doesn't hold for most blocks being checked, since the ThenInst of a IfInstruction could theoretically be an expression represented by an inline block (a ? new C { ... } : ...).

dgrunwald · 2018-06-13T20:11:22Z

ICSharpCode.Decompiler/IL/ControlFlow/ConditionDetection.cs

+			int falseInstIndex = block.Instructions.IndexOf(ifInst) + 1;
+			AddExits(block, falseInstIndex, elseExits);
+
+			var commonExits = elseExits.Where(e1 => thenExits.Any(e2 => DetectExitPoints.CompatibleExitInstruction(e1, e2)));


Quadratic search. Could be optimized using a HashSet with a CompatibleExitInstructionComparer...
But I guess usually there won't be that many exits.

correct, clearer code first, optimisations if obvious or profiling hotspot, generally not that many exits (especially since it's only looking for exits which could be moved to the block root via inversions.

Chicken-Bones · 2018-06-13T23:05:14Z

Thanks for this, switch detection, particularly over-aggressive UseILSwitch will be addressed in my next PR[s]

Chicken-Bones added 2 commits June 12, 2018 18:49

Improve control flow decompilation in ConditionDetection

3fb7c71

Improve control flow decompilation with some compilers

9937302

Add a ControlFlowSimplification step after SplitVariables Enable dead code removal in some unit tests

dgrunwald self-assigned this Jun 12, 2018

dgrunwald approved these changes Jun 13, 2018

View reviewed changes

dgrunwald merged commit 9937302 into icsharpcode:master Jun 13, 2018

dgrunwald added a commit that referenced this pull request Jun 13, 2018

Cosmetic changes during review of PR #1176

4b96f48

dgrunwald added a commit that referenced this pull request Jun 13, 2018

Merge pull request #1176: Improve control flow decompilation

32a0e72

siegfriedpammer mentioned this pull request Dec 11, 2018

C# decompilation: issues with complex conditions. #1094

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve control flow decompilation (fixes #1133) #1176

Improve control flow decompilation (fixes #1133) #1176

Chicken-Bones commented Jun 12, 2018 •

edited

Loading

dgrunwald commented Jun 12, 2018

Chicken-Bones commented Jun 12, 2018 •

edited

Loading

dgrunwald commented Jun 13, 2018

dgrunwald left a comment

dgrunwald Jun 13, 2018

Chicken-Bones Jun 13, 2018

dgrunwald Jun 13, 2018

Chicken-Bones Jun 13, 2018 •

edited

Loading

dgrunwald Jun 13, 2018

Chicken-Bones Jun 13, 2018

dgrunwald Jun 13, 2018

dgrunwald Jun 13, 2018

Chicken-Bones Jun 13, 2018

Chicken-Bones commented Jun 13, 2018

Improve control flow decompilation (fixes #1133) #1176

Improve control flow decompilation (fixes #1133) #1176

Conversation

Chicken-Bones commented Jun 12, 2018 • edited Loading

dgrunwald commented Jun 12, 2018

Chicken-Bones commented Jun 12, 2018 • edited Loading

dgrunwald commented Jun 13, 2018

dgrunwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chicken-Bones Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chicken-Bones commented Jun 13, 2018

Chicken-Bones commented Jun 12, 2018 •

edited

Loading

Chicken-Bones commented Jun 12, 2018 •

edited

Loading

Chicken-Bones Jun 13, 2018 •

edited

Loading