From ebb04ab373c5984d43e8723d112013e591be991c Mon Sep 17 00:00:00 2001 From: Jasmine Karthikeyan <25208576+jaskarth@users.noreply.github.com> Date: Sat, 30 Sep 2023 21:27:29 +0100 Subject: [PATCH] Fix incorrect dominator filtering on finally blocks --- .../decompiler/decompose/DomHelper.java | 18 +- .../decompiler/decompose/DomTracer.java | 28 +- .../DominatorTreeExceptionFilter.java | 18 +- .../FastExtendedPostdominanceHelper.java | 5 +- .../java/decompiler/SingleClassesTest.java | 1 + testData/results/pkg/TestSynchronizedLoop.dec | 281 +++++++++--------- .../results/pkg/TestTryLoopSimpleFinally.dec | 38 +-- testData/results/pkg/TestTryReturn.dec | 68 ++--- testData/results/pkg/TestTrySplit.dec | 73 +++++ testData/src/java8/pkg/TestTrySplit.java | 23 ++ 10 files changed, 336 insertions(+), 217 deletions(-) create mode 100644 testData/results/pkg/TestTrySplit.dec create mode 100644 testData/src/java8/pkg/TestTrySplit.java diff --git a/src/org/jetbrains/java/decompiler/modules/decompiler/decompose/DomHelper.java b/src/org/jetbrains/java/decompiler/modules/decompiler/decompose/DomHelper.java index 51dc4245fc..2992dca7a9 100644 --- a/src/org/jetbrains/java/decompiler/modules/decompiler/decompose/DomHelper.java +++ b/src/org/jetbrains/java/decompiler/modules/decompiler/decompose/DomHelper.java @@ -183,9 +183,12 @@ public static VBStyleCollection, Integer> calcPostDominators(State lstPosts.add(stt.id); } + // The postdom list for this statement must be sorted based on the post reverse postorder of the general statement that it's contained in + // This should lead to proper iteration during general statement creation. lstPosts.sort(Comparator.comparing(mapSortOrder::get)); - if (lstPosts.size() > 1 && (int) lstPosts.get(0) == st.id) { + // After sorting, ensure that the statement that owns this postdominance list comes last, if it comes first. + if (lstPosts.size() > 1 && lstPosts.get(0) == st.id) { lstPosts.add(lstPosts.remove(0)); } @@ -432,7 +435,7 @@ private static boolean processStatement( tracer.info(general, "Find simple statements"); - // Find statements in this subgraph from the basicblocks that comprise it + // Find statements in this subgraph from the basic blocks that comprise it if (findSimpleStatements(general, mapExtPost, tracer)) { tracer.success(general, "Found some simple statements"); reducibility = 0; @@ -448,7 +451,7 @@ private static boolean processStatement( Statement stat = findGeneralStatement(general, forceall, mapExtPost/*, finallyInfos*/); if (stat != null) { - tracer.success(general, "Found general statement: " + stat, stat); + tracer.successCreated(general, "Found general statement: " + stat + " (" + stat.getStats() + ")", stat); // Recurse on the subgraph general statement that we found, and inherit the postdominator set if it's the first statement in the current general boolean complete = processStatement(stat, root, general.getFirst() == stat ? mapExtPost : new HashMap<>(), tracer /*, finallyInfos*/); @@ -633,9 +636,12 @@ private static Statement findGeneralStatement( } setHandlers.removeAll(setNodes); + // Make sure that all the nodes that we're trying to create a new subgraph from are found in any relevant exception handlers + // This serves to avoid breaking the statement finder by having a subgraph only contain part of an exception handler boolean exceptionsOk = true; for (Statement handler : setHandlers) { - if (!handler.getNeighbours(StatEdge.TYPE_EXCEPTION, EdgeDirection.BACKWARD).containsAll(setNodes)) { + List exceptionRange = handler.getNeighbours(StatEdge.TYPE_EXCEPTION, EdgeDirection.BACKWARD); + if (!exceptionRange.containsAll(setNodes)) { exceptionsOk = false; break; } @@ -698,7 +704,7 @@ private static boolean findSimpleStatements(Statement stat, HashMap if -> loop) @@ -710,7 +716,7 @@ private static boolean findSimpleStatements(Statement stat, HashMap props) { - if (COLLECT_STRINGS) { - this.string += ("[" + gen + "] " + s + "\n"); - } - if (COLLECT_DOTS) { HashMap map = new HashMap<>(props); if (map.containsKey(gen)) { @@ -34,6 +30,14 @@ private void add(Statement gen, String s, Map props) { map.put(gen, "xlabel=\"" + s + "\""); } DotExporter.toDotFile(gen, this.structMethod, this.filePrefix, "g" + this.counter, map); + + if (COLLECT_STRINGS) { + string += "(g" + this.counter +") "; + } + } + + if (COLLECT_STRINGS) { + this.string += ("[" + gen + "] " + s + "\n"); } this.counter++; @@ -52,11 +56,17 @@ void info(Statement stat, String s) { } void success(Statement stat, String s) { - this.add(stat, s, Map.of(stat, "fillcolor=lightgreen,style=filled")); + this.add(stat, s, Map.of(stat, "fillcolor=lawngreen,style=filled")); } - void success(Statement stat, String s, Statement stat2) { - this.add(stat, s, Map.of(stat, "fillcolor=lightgreen,style=filled", stat2, "fillcolor=lawngreen,style=filled")); + void successCreated(Statement stat, String s, Statement newStat) { + Map props = new HashMap<>(); + props.put(stat, "fillcolor=orange,style=filled"); + props.put(newStat, "fillcolor=lawngreen,style=filled"); + for (Statement st : newStat.getStats()) { + props.put(st, "fillcolor=pink,style=filled"); + } + this.add(stat, s, props); } @Override diff --git a/src/org/jetbrains/java/decompiler/modules/decompiler/decompose/DominatorTreeExceptionFilter.java b/src/org/jetbrains/java/decompiler/modules/decompiler/decompose/DominatorTreeExceptionFilter.java index b6942a6d29..6c24a31963 100644 --- a/src/org/jetbrains/java/decompiler/modules/decompiler/decompose/DominatorTreeExceptionFilter.java +++ b/src/org/jetbrains/java/decompiler/modules/decompiler/decompose/DominatorTreeExceptionFilter.java @@ -4,6 +4,7 @@ import org.jetbrains.java.decompiler.modules.decompiler.StatEdge; import org.jetbrains.java.decompiler.modules.decompiler.stats.Statement; import org.jetbrains.java.decompiler.modules.decompiler.stats.Statement.EdgeDirection; +import org.jetbrains.java.decompiler.util.DotExporter; import org.jetbrains.java.decompiler.util.collections.VBStyleCollection; import java.util.*; @@ -76,13 +77,16 @@ private void buildDominatorTree() { private void buildExceptionRanges() { for (Statement stat : statement.getStats()) { - List lstPreds = stat.getNeighbours(StatEdge.TYPE_EXCEPTION, EdgeDirection.BACKWARD); - if (!lstPreds.isEmpty()) { + // Internally, jumps into a finally handler are represented as exception edges with the exceptions field set to null + // Don't consider those for exception filtering, as it can lead to incorrect subgraph splitting within catch blocks + List edges = stat.getPredecessorEdges(StatEdge.TYPE_EXCEPTION); + edges.removeIf(e -> e.getExceptions() == null); + if (!edges.isEmpty()) { Set set = new LinkedHashSet<>(); - for (Statement st : lstPreds) { - set.add(st.id); + for (StatEdge st : edges) { + set.add(st.getSource().id); } mapExceptionRanges.put(stat.id, set); @@ -131,11 +135,9 @@ private void buildFilter(Integer id) { Integer exit; if (!range.contains(childid)) { exit = childid; - } - else if (map.containsKey(handler)) { + } else if (map.containsKey(handler)) { exit = -1; - } - else { + } else { exit = mapChild.get(handler); } diff --git a/src/org/jetbrains/java/decompiler/modules/decompiler/decompose/FastExtendedPostdominanceHelper.java b/src/org/jetbrains/java/decompiler/modules/decompiler/decompose/FastExtendedPostdominanceHelper.java index cbea6556f0..eefaefc755 100644 --- a/src/org/jetbrains/java/decompiler/modules/decompiler/decompose/FastExtendedPostdominanceHelper.java +++ b/src/org/jetbrains/java/decompiler/modules/decompiler/decompose/FastExtendedPostdominanceHelper.java @@ -185,10 +185,13 @@ private void filterOnExceptionRanges(DominatorTreeExceptionFilter filter) { for (int head : new HashSet<>(mapExtPostdominators.keySet())) { FastFixedSet set = mapExtPostdominators.get(head); for (Iterator it = set.iterator(); it.hasNext(); ) { - if (!filter.acceptStatementPair(head, it.next())) { + Integer next = it.next(); + + if (!filter.acceptStatementPair(head, next)) { it.remove(); } } + if (set.isEmpty()) { mapExtPostdominators.remove(head); } diff --git a/test/org/jetbrains/java/decompiler/SingleClassesTest.java b/test/org/jetbrains/java/decompiler/SingleClassesTest.java index 41091dea70..cca239d695 100644 --- a/test/org/jetbrains/java/decompiler/SingleClassesTest.java +++ b/test/org/jetbrains/java/decompiler/SingleClassesTest.java @@ -678,6 +678,7 @@ private void registerDefault() { // TODO: Can't inline field initializer for a final field that depends on initialization in a static call register(JAVA_8, "TestStaticBlockFinalField"); register(JAVA_8, "TestWhileLambda"); + register(JAVA_8, "TestTrySplit"); } private void registerEntireClassPath() { diff --git a/testData/results/pkg/TestSynchronizedLoop.dec b/testData/results/pkg/TestSynchronizedLoop.dec index ab424dab2b..7749a5a12c 100644 --- a/testData/results/pkg/TestSynchronizedLoop.dec +++ b/testData/results/pkg/TestSynchronizedLoop.dec @@ -37,26 +37,27 @@ public class TestSynchronizedLoop { public void test12(int i) { synchronized (this) {// 41 - label79: { - System.out.println(1);// 42 + System.out.println(1);// 42 - while (i > 0) {// 43 - i--;// 44 - System.out.println(1.5);// 45 - - try { - System.out.println(1.6);// 47 - } finally { - System.out.println(1.7);// 49 - if (i > 5) {// 50 - break label79; - } + while (true) { + if (i <= 0) {// 43 + System.out.println(2);// 56 + if (i > -2) {// 57 + System.out.println(3);// 58 } + break; } - System.out.println(2);// 56 - if (i > -2) {// 57 - System.out.println(3);// 58 + i--;// 44 + System.out.println(1.5);// 45 + + try { + System.out.println(1.6);// 47 + } finally { + System.out.println(1.7);// 49 + if (i > 5) {// 50 + break; + } } } } @@ -138,113 +139,113 @@ class 'pkg/TestSynchronizedLoop' { method 'test12 (I)V' { 0 38 3 38 - 4 40 - 5 40 - 6 40 - 7 40 - 8 40 + 4 39 + 5 39 + 6 39 + 7 39 + 8 39 b 42 c 42 - f 43 - 10 43 - 11 43 - 12 44 - 13 44 - 14 44 - 15 44 - 16 44 - 17 44 - 18 44 - 1b 47 - 1c 47 - 1d 47 - 1e 47 - 1f 47 - 20 47 - 21 47 - 38 49 - 39 49 - 3a 49 - 3b 49 - 3c 49 - 3d 49 - 3e 49 - 3f 49 - 40 49 - 41 50 - 42 50 - 43 50 - 50 56 - 51 56 - 52 56 - 53 56 - 54 56 - 55 56 - 56 56 - 57 57 - 58 57 - 59 57 - 5a 57 - 5d 58 - 5e 58 - 5f 58 - 60 58 - 61 58 - 70 63 - 71 63 - 72 63 - 75 64 - 76 64 - 77 64 - 78 64 - 79 64 - 7a 64 - 7d 66 + f 50 + 10 50 + 11 50 + 12 51 + 13 51 + 14 51 + 15 51 + 16 51 + 17 51 + 18 51 + 1b 54 + 1c 54 + 1d 54 + 1e 54 + 1f 54 + 20 54 + 21 54 + 38 56 + 39 56 + 3a 56 + 3b 56 + 3c 56 + 3d 56 + 3e 56 + 3f 56 + 40 56 + 41 57 + 42 57 + 43 57 + 50 43 + 51 43 + 52 43 + 53 43 + 54 43 + 55 43 + 56 43 + 57 44 + 58 44 + 59 44 + 5a 44 + 5d 45 + 5e 45 + 5f 45 + 60 45 + 61 45 + 70 64 + 71 64 + 72 64 + 75 65 + 76 65 + 77 65 + 78 65 + 79 65 + 7a 65 + 7d 67 } method 'testLoop (D)V' { - 0 69 - 1 69 - 2 69 - 3 69 - 4 69 - 5 69 - 8 70 - b 70 - c 72 - d 72 - e 72 - f 72 - 10 72 - 12 73 - 13 73 - 14 73 - 16 73 - 21 77 + 0 70 + 1 70 + 2 70 + 3 70 + 4 70 + 5 70 + 8 71 + b 71 + c 73 + d 73 + e 73 + f 73 + 10 73 + 12 74 + 13 74 + 14 74 + 16 74 + 21 78 } method 'testFlatten ()V' { - 0 80 - 1 80 - 2 80 - 3 80 - 4 81 - 5 81 - 6 81 - 7 81 - 8 83 - 9 83 - a 83 - b 83 - c 83 - 10 84 - 14 84 - 16 85 - 17 85 - 18 85 - 1a 85 - 1d 86 - 2c 88 + 0 81 + 1 81 + 2 81 + 3 81 + 4 82 + 5 82 + 6 82 + 7 82 + 8 84 + 9 84 + a 84 + b 84 + c 84 + 10 85 + 14 85 + 16 86 + 17 86 + 18 86 + 1a 86 + 1d 87 + 2c 89 } } @@ -265,31 +266,31 @@ Lines mapping: 34 <-> 33 37 <-> 36 41 <-> 39 -42 <-> 41 +42 <-> 40 43 <-> 43 -44 <-> 44 -45 <-> 45 -47 <-> 48 -49 <-> 50 -50 <-> 51 -56 <-> 57 -57 <-> 58 -58 <-> 59 -62 <-> 64 -63 <-> 65 -65 <-> 67 -68 <-> 70 -69 <-> 71 -74 <-> 73 -75 <-> 74 -81 <-> 78 -84 <-> 81 -85 <-> 82 -86 <-> 84 -87 <-> 85 -88 <-> 86 -89 <-> 87 -91 <-> 89 +44 <-> 51 +45 <-> 52 +47 <-> 55 +49 <-> 57 +50 <-> 58 +56 <-> 44 +57 <-> 45 +58 <-> 46 +62 <-> 65 +63 <-> 66 +65 <-> 68 +68 <-> 71 +69 <-> 72 +74 <-> 74 +75 <-> 75 +81 <-> 79 +84 <-> 82 +85 <-> 83 +86 <-> 85 +87 <-> 86 +88 <-> 87 +89 <-> 88 +91 <-> 90 Not mapped: 11 36 @@ -297,4 +298,4 @@ Not mapped: 53 60 76 -79 +79 \ No newline at end of file diff --git a/testData/results/pkg/TestTryLoopSimpleFinally.dec b/testData/results/pkg/TestTryLoopSimpleFinally.dec index f943352550..59126197d4 100644 --- a/testData/results/pkg/TestTryLoopSimpleFinally.dec +++ b/testData/results/pkg/TestTryLoopSimpleFinally.dec @@ -25,12 +25,12 @@ public class TestTryLoopSimpleFinally { try { while (x >= 0) {// 27 Scanner scanner = new Scanner(file);// 28 - if (x % 11 != 0) {// 30 - x -= scanner.nextInt();// 35 - } else { + if (x % 11 == 0) {// 30 System.out.println("nice");// 31 return;// 32 } + + x -= scanner.nextInt();// 35 } } finally { System.out.println("Finally");// 38 @@ -67,18 +67,18 @@ class 'pkg/TestTryLoopSimpleFinally' { f 27 10 27 11 27 - 14 30 - 15 30 - 16 30 - 17 30 - 18 30 - 19 30 - 24 31 - 26 28 - 27 28 - 28 28 - 29 28 - 2b 28 + 14 28 + 15 28 + 16 28 + 17 28 + 18 28 + 19 28 + 24 29 + 26 32 + 27 32 + 28 32 + 29 32 + 2b 32 37 37 3c 35 3d 35 @@ -99,13 +99,13 @@ Lines mapping: 27 <-> 26 28 <-> 27 30 <-> 28 -31 <-> 31 -32 <-> 32 -35 <-> 29 +31 <-> 29 +32 <-> 30 +35 <-> 33 38 <-> 36 39 <-> 38 Not mapped: 17 22 36 -40 +40 \ No newline at end of file diff --git a/testData/results/pkg/TestTryReturn.dec b/testData/results/pkg/TestTryReturn.dec index 81fa73078c..342c535a5b 100644 --- a/testData/results/pkg/TestTryReturn.dec +++ b/testData/results/pkg/TestTryReturn.dec @@ -55,20 +55,20 @@ public class TestTryReturn { public boolean testFinally3(boolean b, boolean c, int a, Supplier supplier) { boolean var5; try { - if (b) {// 50 - return c && supplier.get();// 51 - } + if (!b) {// 50 + if (a > 0) {// 54 + return a == 1;// 55 + } - if (a <= 0) {// 54 return supplier.get();// 58 } - var5 = a == 1; + var5 = c && supplier.get(); } finally { System.out.println("Finally");// 60 } - return var5;// 55 + return var5;// 51 } public boolean testFinally4(Supplier supplier) { @@ -335,30 +335,30 @@ class 'pkg/TestTryReturn' { method 'testFinally3 (ZZILjava/util/function/Supplier;)Z' { 0 57 1 57 - 4 58 - 5 58 - 8 58 - 9 58 - a 58 - b 58 - c 58 - d 58 - e 58 - 12 58 - 13 58 - 14 58 - 15 58 - 29 58 - 2a 61 - 2b 61 - 2e 65 - 2f 65 - 30 65 - 38 65 - 39 65 - 42 70 - 43 70 - 44 70 + 4 65 + 5 65 + 8 65 + 9 65 + a 65 + b 65 + c 65 + d 65 + e 65 + 12 65 + 13 65 + 14 65 + 15 65 + 1d 65 + 1e 65 + 27 70 + 28 70 + 29 70 + 2a 58 + 2b 58 + 2e 59 + 2f 59 + 30 59 + 44 59 45 62 46 62 47 62 @@ -618,9 +618,9 @@ Lines mapping: 43 <-> 52 45 <-> 52 50 <-> 58 -51 <-> 59 -54 <-> 62 -55 <-> 71 +51 <-> 71 +54 <-> 59 +55 <-> 60 58 <-> 63 60 <-> 68 65 <-> 75 @@ -686,4 +686,4 @@ Not mapped: 157 175 191 -196 +196 \ No newline at end of file diff --git a/testData/results/pkg/TestTrySplit.dec b/testData/results/pkg/TestTrySplit.dec new file mode 100644 index 0000000000..51eb2fe90b --- /dev/null +++ b/testData/results/pkg/TestTrySplit.dec @@ -0,0 +1,73 @@ +package pkg; + +public class TestTrySplit { + public void test() { + Object obj = null;// 5 + + try { + obj = new Object();// 7 + return;// 17 + } catch (ArithmeticException var6) {// 8 + if (obj != null) {// 9 + System.out.println("a");// 10 + } + + throwMyException(var6.getMessage());// 13 + } finally { + System.out.println("b");// 16 + } + }// 14 + + public static void throwMyException(String message) { + throw new RuntimeException(message);// 21 + } +} + +class 'pkg/TestTrySplit' { + method 'test ()V' { + 0 4 + 1 4 + 9 7 + 12 8 + 15 9 + 16 10 + 17 10 + 1a 11 + 1b 11 + 1c 11 + 1d 11 + 1e 11 + 1f 11 + 22 14 + 23 14 + 24 14 + 25 14 + 26 14 + 31 18 + 33 16 + 34 16 + 35 16 + 36 16 + 37 16 + 38 16 + } + + method 'throwMyException (Ljava/lang/String;)V' { + 4 21 + 8 21 + } +} + +Lines mapping: +5 <-> 5 +7 <-> 8 +8 <-> 10 +9 <-> 11 +10 <-> 12 +13 <-> 15 +14 <-> 19 +16 <-> 17 +17 <-> 9 +21 <-> 22 +Not mapped: +18 \ No newline at end of file diff --git a/testData/src/java8/pkg/TestTrySplit.java b/testData/src/java8/pkg/TestTrySplit.java new file mode 100644 index 0000000000..95dba15748 --- /dev/null +++ b/testData/src/java8/pkg/TestTrySplit.java @@ -0,0 +1,23 @@ +package pkg; + +public class TestTrySplit { + public void test() { + Object obj = null; + try { + obj = new Object(); + } catch (ArithmeticException ex) { + if (obj != null) { + System.out.println("a"); + } + + throwMyException(ex.getMessage()); + return; + } finally { + System.out.println("b"); + } + } + + public static void throwMyException(String message) { + throw new RuntimeException(message); + } +}