Do not patch dead branch instructions

Author: DSouzaM

truffle/src/com.oracle.truffle.api.bytecode.test/src/com/oracle/truffle/api/bytecode/test/DeadCodeTest.java

@@ -41,12 +41,14 @@
 package com.oracle.truffle.api.bytecode.test;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
 
 import org.junit.Test;
 
 import com.oracle.truffle.api.TruffleLanguage;
 import com.oracle.truffle.api.bytecode.AbstractBytecodeTruffleException;
 import com.oracle.truffle.api.bytecode.BytecodeConfig;
+import com.oracle.truffle.api.bytecode.BytecodeLabel;
 import com.oracle.truffle.api.bytecode.BytecodeParser;
 import com.oracle.truffle.api.bytecode.BytecodeRootNode;
 import com.oracle.truffle.api.bytecode.BytecodeRootNodes;
@@ -562,6 +564,73 @@ public void testUnreachableFinallyWithLabel() {
                         "return");
     }
 
+    @Test
+    public void testUnreachableBranchIsNotPatched() {
+        /**
+         * This is a regression test for a branch fix-up bug. The branch instruction below is dead,
+         * but its location was included in the list of "fix up" locations. When the label was
+         * emitted, we would "fix up" that location, overwriting the load_argument (from the
+         * exception handler) that happened to be at that location.
+         *
+         * @formatter:off
+         * try {
+         *   return throw();
+         *   branch lbl;  // dead
+         * } finally {
+         *   load_argument(0);
+         * }
+         * lbl:
+         * @formatter:on
+         */
+        DeadCodeTestRootNode node = (DeadCodeTestRootNode) parse(b -> {
+            b.beginRoot(LANGUAGE);
+            b.beginBlock();
+
+            BytecodeLabel lbl = b.createLabel();
+            b.beginFinallyTry(b.createLocal());
+
+            b.emitLoadArgument(0); // finally
+
+            b.beginBlock(); // begin try
+
+            b.beginReturn();
+            b.emitThrow();
+            b.endReturn();
+
+            b.emitBranch(lbl);
+
+            b.endBlock(); // end try
+
+            b.endFinallyTry();
+
+            b.emitLabel(lbl);
+
+            b.endBlock();
+            b.endRoot();
+        }).getRootNode();
+
+        assertInstructions(node,
+                        "c.Throw",
+                        "load.argument",
+                        "pop",
+                        "return",
+                        "load.argument",
+                        "pop",
+                        "throw",
+                        "trap");
+        node.getIntrospectionData().getInstructions().stream() //
+                        .filter(insn -> insn.getName().equals("load.argument")) //
+                        .forEach(insn -> assertEquals(0, insn.getArgumentValues().get(0).getInteger()));
+        try {
+            node.getCallTarget().call(42);
+            fail("exception expected");
+        } catch (TestException ex) {
+            // pass
+        } catch (Exception ex) {
+            fail("Wrong exception encountered: " + ex.getMessage());
+        }
+    }
+
     private static void emitUnreachableCode(Builder b) {
         // custom operation
         b.beginAdd();

truffle/src/com.oracle.truffle.api.bytecode/src/com/oracle/truffle/api/bytecode/introspection/Argument.java

@@ -86,7 +86,7 @@ public ArgumentType getKind() {
     }
 
     public int getInteger() {
-        if (getKind() != ArgumentType.CONSTANT) {
+        if (getKind() != ArgumentType.INTEGER) {
             throw new UnsupportedOperationException(String.format("Not supported for argument type %s.", getKind()));
         }
         return (short) data[2];

truffle/src/com.oracle.truffle.dsl.processor/src/com/oracle/truffle/dsl/processor/bytecode/generator/BytecodeDSLNodeFactory.java

@@ -3424,29 +3424,20 @@ private CodeTree createOperationBeginData(CodeTreeBuilder b, OperationModel oper
             return switch (operation.kind) {
                 case STORE_LOCAL, STORE_LOCAL_MATERIALIZED -> {
                     if (model.usesBoxingElimination()) {
-
                         yield createOperationData("StoreLocalData", "(BytecodeLocalImpl)" + operation.getOperationArgumentName(0), UNINIT);
-                    } else
-
-                    {
+                    } else {
                         yield CodeTreeBuilder.singleString(operation.getOperationArgumentName(0));
                     }
                 }
-                case LOAD_LOCAL_MATERIALIZED ->
-
-                {
+                case LOAD_LOCAL_MATERIALIZED -> {
                     yield CodeTreeBuilder.singleString(operation.getOperationArgumentName(0));
                 }
                 case IF_THEN -> createOperationData("IfThenData", UNINIT, "this.reachable");
                 case IF_THEN_ELSE -> createOperationData("IfThenElseData", UNINIT, UNINIT, "this.reachable", "this.reachable");
                 case CONDITIONAL -> {
                     if (model.usesBoxingElimination()) {
-
                         yield createOperationData("ConditionalData", UNINIT, UNINIT, "this.reachable", "this.reachable", UNINIT, UNINIT);
-                    } else
-
-                    {
-
+                    } else {
                         yield createOperationData("ConditionalData", UNINIT, UNINIT, "this.reachable", "this.reachable");
                     }
                 }
@@ -4155,12 +4146,15 @@ private void buildEmitOperationInstruction(CodeTreeBuilder b, OperationModel ope
                     b.end();
                     // Mark the branch target as uninitialized. Add this location to a work list to
                     // be processed once the label is defined.
+
+                    b.startIf().string("this.reachable").end().startBlock();
                     b.startStatement().startCall("registerUnresolvedLabel");
                     b.string("labelImpl");
                     b.string("bci + 1");
                     b.string("currentStackHeight");
                     b.end(2);
                     b.newLine();
+                    b.end();
 
                     // Branches inside finally handlers can only be relative to the handler,
                     // otherwise a finally handler emitted before a "return" could branch out of the
@@ -4171,8 +4165,10 @@ private void buildEmitOperationInstruction(CodeTreeBuilder b, OperationModel ope
                     emitThrowIllegalStateException(b, "\"Branches inside finally handlers can only target labels defined in the same handler.\"");
                     b.end();
 
+                    b.startIf().string("this.reachable").end().startBlock();
                     b.lineComment("We need to track branch targets inside finally handlers so that they can be adjusted each time the handler is emitted.");
                     b.statement("finallyTryContext.finallyRelativeBranches.add(bci + 1)");
+                    b.end();
 
                     b.end();
                     yield new String[]{UNINIT};