Better handle synchronized blocks created with jsr and ret

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

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 <var>'.
+        // 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) {

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.

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");

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");
+      }
+    }
+  }
+
+}

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