diff --git a/src/org/jetbrains/java/decompiler/main/rels/MethodProcessor.java b/src/org/jetbrains/java/decompiler/main/rels/MethodProcessor.java index b234feaa44..553f31133f 100644 --- a/src/org/jetbrains/java/decompiler/main/rels/MethodProcessor.java +++ b/src/org/jetbrains/java/decompiler/main/rels/MethodProcessor.java @@ -7,6 +7,7 @@ import org.jetbrains.java.decompiler.code.CodeConstants; import org.jetbrains.java.decompiler.code.InstructionSequence; import org.jetbrains.java.decompiler.code.cfg.ControlFlowGraph; +import org.jetbrains.java.decompiler.code.cfg.ExceptionRangeCFG; import org.jetbrains.java.decompiler.main.DecompilerContext; import org.jetbrains.java.decompiler.main.collectors.CounterContainer; import org.jetbrains.java.decompiler.main.decompiler.CancelationManager; @@ -189,7 +190,14 @@ public static RootStatement codeToJava(StructClass cl, StructMethod mt, MethodDe debugCurrentlyDecompiling.set(root); } - decompileRecord.add("ProcessFinally_Post", root); + // In rare cases, the final round of finally processing can reveal another synchronized statement. Try to parse it now. + if (DomHelper.buildSynchronized(root)) { + decompileRecord.add("BuiltFinallySynchronized", root); + } + + if (finallyProcessed > 0) { + decompileRecord.add("ProcessFinally_Post", root); + } // remove synchronized exception handler // not until now because of comparison between synchronized statements in the finally cycle diff --git a/src/org/jetbrains/java/decompiler/modules/decompiler/FinallyProcessor.java b/src/org/jetbrains/java/decompiler/modules/decompiler/FinallyProcessor.java index 4ba7de5796..4e51a7d840 100644 --- a/src/org/jetbrains/java/decompiler/modules/decompiler/FinallyProcessor.java +++ b/src/org/jetbrains/java/decompiler/modules/decompiler/FinallyProcessor.java @@ -18,7 +18,6 @@ import org.jetbrains.java.decompiler.modules.decompiler.flow.DirectGraph; import org.jetbrains.java.decompiler.modules.decompiler.flow.DirectNode; import org.jetbrains.java.decompiler.modules.decompiler.flow.FlattenStatementsHelper; -import org.jetbrains.java.decompiler.modules.decompiler.sforms.SFormsConstructor; import org.jetbrains.java.decompiler.modules.decompiler.sforms.SSAConstructorSparseEx; import org.jetbrains.java.decompiler.modules.decompiler.sforms.SSAUConstructorSparseEx; import org.jetbrains.java.decompiler.modules.decompiler.sforms.SimpleSSAReassign; @@ -208,7 +207,18 @@ private Record getFinallyInformation(StructClass cl, StructMethod mt, RootStatem isTrueExit = false; - for (int i = 0; i < node.exprents.size(); i++) { + // Try to find the "true path" of the finally block by searching for a relevant 'athrow '. + // We should search from the initial expression of each block, except in the case where the block we're searching + // contains the relevant var itself, in which case we should search from the 2nd or 3rd expression, depending on + // the firstcode. This is because we might accidentally stumble into the same expression we were searching from, + // leading to a false failure of finally processing. + int startIdx = 0; + if (firstBlockStatement == node.block) { + // firstcode (only 0 or 2 here) determines which instruction to start from. + startIdx = firstcode == 2 ? 2 : 1; + } + + for (int i = startIdx; i < node.exprents.size(); i++) { Exprent exprent = node.exprents.get(i); if (firstcode == 0) { 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 2992dca7a9..721ab540cf 100644 --- a/src/org/jetbrains/java/decompiler/modules/decompiler/decompose/DomHelper.java +++ b/src/org/jetbrains/java/decompiler/modules/decompiler/decompose/DomHelper.java @@ -240,10 +240,10 @@ public static boolean removeSynchronizedHandler(Statement stat) { } - private static void buildSynchronized(Statement stat) { - + public static boolean buildSynchronized(Statement stat) { + boolean res = false; for (Statement st : stat.getStats()) { - buildSynchronized(st); + res |= buildSynchronized(st); } if (stat instanceof SequenceStatement) { @@ -265,16 +265,25 @@ private static void buildSynchronized(Statement stat) { next = next.getFirst(); } - if (next instanceof CatchAllStatement) { - - CatchAllStatement ca = (CatchAllStatement) next; + if (next instanceof CatchAllStatement ca) { + // See if the head of the synchronized is suitable. + // In most cases, the synchronized block will contain a monitorexit in the exit block of the catchall. + // If the synchronized statement ends in a throw expression, check for that too. boolean headOk = ca.getFirst().containsMonitorExitOrAthrow(); + // In case the statement has no exit points, it will not have a monitorexit on the exit block because + // there is no exit block. Consider it ok too. if (!headOk) { headOk = hasNoExits(ca.getFirst()); } + // In the final case, we may have incorporated all the monitorexits into a finally block, which should be + // semantically identical. Handle that too. + if (!headOk) { + headOk = ca.isFinally(); + } + // If the body of the monitor ends in a throw, it won't have a monitor exit as the catch handler will call it. // We will also not have a monitorexit in an infinite loop as there is no way to leave the statement. // However, the handler *must* have a monitorexit! @@ -284,6 +293,15 @@ private static void buildSynchronized(Statement stat) { ca.getFirst().markMonitorexitDead(); ca.getHandler().markMonitorexitDead(); + // Sometimes trailing monitorexits occur in our or our parent's successor edges too, remove them too. + for (StatEdge edge : ca.getSuccessorEdgeView(StatEdge.TYPE_REGULAR)) { + edge.getDestination().markMonitorexitDead(); + } + + for (StatEdge edge : ca.getParent().getSuccessorEdgeView(StatEdge.TYPE_REGULAR)) { + edge.getDestination().markMonitorexitDead(); + } + // remove the head block from sequence current.removeSuccessor(current.getSuccessorEdges(Statement.STATEDGE_DIRECT_ALL).get(0)); @@ -308,6 +326,7 @@ private static void buildSynchronized(Statement stat) { ca.getParent().replaceStatement(ca, sync); found = true; + res = true; break; } } @@ -319,6 +338,8 @@ private static void buildSynchronized(Statement stat) { } } } + + return res; } // Checks if a statement has no exits (disregarding exceptions) that lead outside the statement. diff --git a/test/org/jetbrains/java/decompiler/SingleClassesTest.java b/test/org/jetbrains/java/decompiler/SingleClassesTest.java index 2ab7b5497d..a67fa6b1d2 100644 --- a/test/org/jetbrains/java/decompiler/SingleClassesTest.java +++ b/test/org/jetbrains/java/decompiler/SingleClassesTest.java @@ -517,8 +517,8 @@ private void registerDefault() { register(JAVA_8, "TestOverrideIndirect"); // INN resugaring is now in a separate plugin, so this produces intentionally "bad" output registerRaw(CUSTOM, "TestIdeaNotNull"); - // TODO: Synchronized blocks don't work properly registerRaw(CUSTOM, "TestHotjava"); + registerRaw(CUSTOM, "TestJava1Synchronized"); register(JAVA_8, "TestLabeledBreaks"); // TODO: test9&10- for loop not created, loop extractor needs another pass register(JAVA_8, "TestSwitchLoop"); diff --git a/testData/classes/custom/TestJava1Synchronized.class b/testData/classes/custom/TestJava1Synchronized.class new file mode 100644 index 0000000000..ed780b5843 Binary files /dev/null and b/testData/classes/custom/TestJava1Synchronized.class differ diff --git a/testData/classes/custom/source/TestJava1Synchronized.java b/testData/classes/custom/source/TestJava1Synchronized.java new file mode 100644 index 0000000000..5a5e57319b --- /dev/null +++ b/testData/classes/custom/source/TestJava1Synchronized.java @@ -0,0 +1,43 @@ +public class TestJava1Synchronized { + public void test1(int in) { + synchronized (this) { + if (in == 0) { + System.out.println("0"); + return; + } + + System.out.println("1"); + } + + System.out.println("2"); + } + + public void test2(int in) { + synchronized (this) { + for (int i = 0; i < in; i++) { + System.out.println("hello"); + } + } + } + + public void test3() { + try { + synchronized (this) { + System.out.println("hello"); + } + } finally { + System.out.println("finally"); + } + } + + public void test4() { + try { + System.out.println("try"); + } finally { + synchronized (this) { + System.out.println("hello"); + } + } + } + +} \ No newline at end of file diff --git a/testData/results/TestHotjava.dec b/testData/results/TestHotjava.dec index 4bb87a2799..1b7bdef6c9 100644 --- a/testData/results/TestHotjava.dec +++ b/testData/results/TestHotjava.dec @@ -11,53 +11,26 @@ public class TestHotjava { } } - // $VF: Could not inline inconsistent finally blocks - // $VF: Could not create synchronized statement, marking monitor enters and exits public void testMonitor1() { - synchronized (this){} // $VF: monitorenter // 15 - - try { + synchronized (this) {// 15 System.out.println("Synchronized");// 16 - } catch (Throwable var3) { - // $VF: monitorexit - throw var3; } - - // $VF: monitorexit } - // $VF: Could not inline inconsistent finally blocks - // $VF: Could not create synchronized statement, marking monitor enters and exits public void testMonitor2(Object var1) { - synchronized (var1){} // $VF: monitorenter // 21 - - try { + synchronized (var1) {// 21 System.out.println("Synchronized");// 22 - } catch (Throwable var4) { - // $VF: monitorexit - throw var4; } - - // $VF: monitorexit } - // $VF: Could not inline inconsistent finally blocks - // $VF: Could not create synchronized statement, marking monitor enters and exits public void testMonitor3() { - synchronized (this){} // $VF: monitorenter // 27 - - try { + synchronized (this) {// 27 try { System.out.println("Try");// 28 29 } finally { System.out.println("Jsr");// 31 } - } catch (Throwable var10) { - // $VF: monitorexit - throw var10; } - - // $VF: monitorexit } } @@ -94,65 +67,56 @@ class 'TestHotjava' { } method 'testMonitor1 ()V' { - 0 16 - 2 16 - 3 16 - 4 19 - 5 19 - 6 19 - 7 19 - 8 19 - 9 19 - a 19 - b 19 - d 25 - e 26 - 10 21 - 11 22 + 0 14 + 2 14 + 3 14 + 4 15 + 5 15 + 6 15 + 7 15 + 8 15 + 9 15 + a 15 + b 15 + e 17 } method 'testMonitor2 (Ljava/lang/Object;)V' { - 0 31 - 2 31 - 3 31 - 4 34 - 5 34 - 6 34 - 7 34 - 8 34 - 9 34 - a 34 - b 34 - d 40 - e 41 - 10 36 - 11 37 + 0 20 + 2 20 + 3 20 + 4 21 + 5 21 + 6 21 + 7 21 + 8 21 + 9 21 + a 21 + b 21 + e 23 } method 'testMonitor3 ()V' { - 0 46 - 2 46 - 3 46 - 4 50 - 5 50 - 6 50 - 7 50 - 8 50 - 9 50 - a 50 - b 50 - 13 52 - 14 52 - 15 52 - 18 52 - 19 52 - 1a 52 - 1b 52 - 1c 52 - 25 59 - 26 60 - 28 55 - 29 56 + 0 26 + 2 26 + 3 26 + 4 28 + 5 28 + 6 28 + 7 28 + 8 28 + 9 28 + a 28 + b 28 + 13 30 + 14 30 + 15 30 + 18 30 + 19 30 + 1a 30 + 1b 30 + 1c 30 + 26 33 } } @@ -162,11 +126,11 @@ Lines mapping: 7 <-> 8 8 <-> 8 10 <-> 10 -15 <-> 17 -16 <-> 20 -21 <-> 32 -22 <-> 35 -27 <-> 47 -28 <-> 51 -29 <-> 51 -31 <-> 53 +15 <-> 15 +16 <-> 16 +21 <-> 21 +22 <-> 22 +27 <-> 27 +28 <-> 29 +29 <-> 29 +31 <-> 31 \ No newline at end of file diff --git a/testData/results/TestJava1Synchronized.dec b/testData/results/TestJava1Synchronized.dec new file mode 100644 index 0000000000..03f73b1c1d --- /dev/null +++ b/testData/results/TestJava1Synchronized.dec @@ -0,0 +1,173 @@ +public class TestJava1Synchronized { + public void test1(int var1) { + synchronized (this) {// 3 + if (var1 == 0) {// 4 + System.out.println("0");// 5 + return; + } + + System.out.println("1");// 9 + } + + System.out.println("2");// 12 + } + + public void test2(int var1) { + synchronized (this) {// 16 + for (int var4 = 0; var4 < var1; var4++) {// 17 + System.out.println("hello");// 18 + } + } + } + + public void test3() { + try { + synchronized (this) {// 24 25 + System.out.println("hello");// 26 + } + } finally { + System.out.println("finally");// 29 + } + } + + public void test4() { + try { + System.out.println("try");// 34 35 + } finally { + synchronized (this) {// 37 + System.out.println("hello");// 38 + } + } + } +} + +class 'TestJava1Synchronized' { + method 'test1 (I)V' { + 0 2 + 2 2 + 3 2 + 4 3 + 5 3 + 6 3 + 7 3 + 8 4 + 9 4 + a 4 + b 4 + c 4 + d 4 + e 4 + f 4 + 14 8 + 15 8 + 16 8 + 17 8 + 18 8 + 19 8 + 1a 8 + 1b 8 + 1e 11 + 1f 11 + 20 11 + 26 5 + 29 11 + 2a 11 + 2c 11 + 2d 11 + 2e 11 + } + + method 'test2 (I)V' { + 0 15 + 2 15 + 3 15 + 4 16 + 5 16 + 6 16 + a 17 + b 17 + c 17 + d 17 + e 17 + f 17 + 10 17 + 11 17 + 12 16 + 13 16 + 14 16 + 15 16 + 16 16 + 17 16 + 18 16 + 19 16 + 1a 16 + 1d 20 + } + + method 'test3 ()V' { + 0 24 + 2 24 + 3 24 + 4 25 + 5 25 + 6 25 + 7 25 + 8 25 + 9 25 + a 25 + b 25 + 19 28 + 1a 28 + 1b 28 + 1e 28 + 1f 28 + 20 28 + 21 28 + 24 30 + } + + method 'test4 ()V' { + 0 34 + 1 34 + 2 34 + 3 34 + 4 34 + 5 34 + 6 34 + 7 34 + d 36 + 13 36 + 14 36 + 17 37 + 18 37 + 19 37 + 1a 37 + 1b 37 + 1c 37 + 1d 37 + 1e 37 + 20 38 + 21 40 + } +} + +Lines mapping: +3 <-> 3 +4 <-> 4 +5 <-> 5 +9 <-> 9 +12 <-> 12 +16 <-> 16 +17 <-> 17 +18 <-> 18 +24 <-> 25 +25 <-> 25 +26 <-> 26 +29 <-> 29 +34 <-> 35 +35 <-> 35 +37 <-> 37 +38 <-> 38 +Not mapped: +2 +6