MinecraftForge / ForgeFlower

Forge's modifications to FernFlower. Fixing various bugs/inconsistencies. Main Repo: https://github.com/MinecraftForge/FernFlower
Apache License 2.0
80 stars 44 forks source link

Fix loop generation causing improper loop promotion #125

Closed jaskarth closed 1 year ago

jaskarth commented 1 year ago

This PR fixes the longstanding bug that causes certain loops to get absolutely clobbered, causing multiple loops to get nested together. This can be seen in #70 and other places in Minecraft's code. When loops get misgenerated like this, the loop type can't be properly identifies and falls back to be while-true or do-while loops.

The actual mechanism behind this fix is a bit nebulous, but I'll try my best to explain: When Fernflower starts it's initial control flow graph analysis to build the statement graph, it builds the statements that it knows have control flow simple enough to be able to be represented as structured Java code. When it tries the simple method of trying to find every single type of statement (here) and finds that there's still more to analyze, it'll then try to cleave the control flow graph into a smaller subgraph in order to reduce the amount of control flow edges it has to sort through to find statements. The analysis method then tries to find the statements in the smaller subgraph, and then uses those results to finish decompiling the things it couldn't in the larger graph. The way Fernflower decides how and where to split the subgraph is exceedingly convoluted but essentially it tries to find the smallest segment in the graph that has no outgoing edges in order to create an isolated graph, with information from FastExtendedPostdominanceHelper. That class analyzes the current subgraph to find for each statement the list of statements it needs to pass through to exit the method.

In a commit in March of 2015, the original Fernflower author pushed a commit that improved the performance of the postdominance calculator by modifying the algorithm and reducing the number of allocations. However, with this change they accidentally broke the algorithm in subtle ways- multiple code constructs silently decompiled differently, such as default branches in switch and, of course, nested loops. This change made it so that certain types of nested loops wouldn't have their postdominance calculated correctly, causing the graph parser to create subgraphs in bad places- places where splitting could create multiple loop statements for a single loop construct. This is what causes the bug, as a single loop is broken up into multiple smaller fragments due to subgraph splitting, making the resulting Java code significantly harder to read and understand.

The proposed fix uses a new class to model cycles (loop constructs) in FastExtendedPostDominanceHelper, and using that model to improve how postdominance of loops is generated. All the nodes found inside of a well formed loop get the loop header added as a postdominator, which allows the subgraph algorithm to better understand the reach of the loop, to prevent it from splitting a loop multiple times. I considered the alternative solution of simply reverting the fix, but that caused way more issues as a ton of code became slightly different and multiple parts of the Fernflower and ForgeFlower codebase were unprepared for that.

Fixes #70 Diff: 1.19.2 diff

zml2008 commented 1 year ago

tbh a lot of this graph stuff is a bit over my head, but your writeup seems sufficient to explain to the world of the future why you made these changes :)