DRILL-6868: Upgrade Janino compiler to 3.0.11

- Remove workaround where removing adjacent ALOAD-POP instruction pairs
- Remove ModifiedUnparser and use DeepCopier for modifying methods instead of modifying it with custom Unparser implementation

closes #1553

5 files changed, 58 insertions, 454 deletions

diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/AloadPopRemover.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/AloadPopRemover.java
deleted file mode 100644
index a6df261d3..000000000
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/AloadPopRemover.java
+++ /dev/null
@@ -1,333 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements.  See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership.  The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License.  You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package org.apache.drill.exec.compile.bytecode;
-
-import org.objectweb.asm.AnnotationVisitor;
-import org.objectweb.asm.Attribute;
-import org.objectweb.asm.Handle;
-import org.objectweb.asm.Label;
-import org.objectweb.asm.MethodVisitor;
-import org.objectweb.asm.Opcodes;
-import org.objectweb.asm.TypePath;
-
-
-/**
- * Remove any adjacent ALOAD-POP instruction pairs.
- *
- * The Janino compiler generates an instruction stream where it will ALOAD a
- * holder's objectref, and then immediately POP it because the compiler has
- * recognized that the method call that it loaded the objectref for is static
- * (i.e., no this pointer is required). This causes a problem with our scalar
- * replacement strategy, because once we remove the holders, we simply eliminate
- * the ALOAD instructions. In this case, the POP gets left behind, and breaks
- * the program stack.
- *
- * This class looks for ALOADs that are immediately followed by a POP, and removes
- * the pair of instructions altogether.
- *
- * It is unknown if other compilers besides Janino generate this instruction sequence,
- * but to be on the safe side, we'll use this class unconditionally to filter bytecode.
- *
- * TODO: this might be easier to do by going off an InsnList; the state machine would
- * be in the loop that visits the instructions then.
- */
-public class AloadPopRemover extends MethodVisitor {
-  private final static int NONE = -1; // var value to indicate no captured ALOAD
-  private int var = NONE;
-
-  /**
-   * Constructor.
-   *
-   * See {@link org.objectweb.asm.MethodVisitor#MethodVisitor(int)}.
-   */
-  public AloadPopRemover(final int api) {
-    super(api);
-  }
-
-  /**
-   * Constructor.
-   *
-   * See {@link org.objectweb.asm.MethodVisitor#MethodVisitor(int, MethodVisitor)}.
-   */
-  public AloadPopRemover(final int api, final MethodVisitor mv) {
-    super(api, mv);
-  }
-
-  /**
-   * Process a deferred ALOAD instruction, if there is one.
-   *
-   * If there is no deferred ALOAD, does nothing, and returns false.
-   *
-   * If there is a deferred ALOAD, and we're on a POP instruction
-   * (indicated by onPop), does nothing (the ALOAD is not forwarded),
-   * and returns true.
-   *
-   * If there is a deferred ALOAD and we're not on a POP instruction,
-   * forwards the deferred ALOAD, and returns false
-   *
-   * @param onPop true if the current instruction is POP
-   * @return true if onPop and there was a deferred ALOAD, false otherwise;
-   *   basically, returns true if the ALOAD-POP optimization is required
-   */
-  private boolean processDeferredAload(final boolean onPop) {
-    // if the previous instruction wasn't an ALOAD, then there's nothing to do
-    if (var == NONE) {
-      return false;
-    }
-
-    // clear the variable index, but save it for local use
-    final int localVar = var;
-    var = NONE;
-
-    // if the next instruction is a POP, don't emit the deferred ALOAD
-    if (onPop) {
-      return true;
-    }
-
-    // if we got here, we're not on a POP, so emit the deferred ALOAD
-    super.visitVarInsn(Opcodes.ALOAD, localVar);
-    return false;
-  }
-
-  @Override
-  public AnnotationVisitor visitAnnotation(final String desc, final boolean visible) {
-    processDeferredAload(false);
-    final AnnotationVisitor annotationVisitor = super.visitAnnotation(desc, visible);
-    return annotationVisitor;
-  }
-
-  @Override
-  public AnnotationVisitor visitAnnotationDefault() {
-    processDeferredAload(false);
-    final AnnotationVisitor annotationVisitor = super.visitAnnotationDefault();
-    return annotationVisitor;
-  }
-
-  @Override
-  public void visitAttribute(final Attribute attr) {
-    processDeferredAload(false);
-    super.visitAttribute(attr);
-  }
-
-  @Override
-  public void visitCode() {
-    processDeferredAload(false);
-    super.visitCode();
-  }
-
-  @Override
-  public void visitEnd() {
-    processDeferredAload(false);
-    super.visitEnd();
-  }
-
-  @Override
-  public void visitFieldInsn(final int opcode, final String owner, final String name,
-      final String desc) {
-    processDeferredAload(false);
-    super.visitFieldInsn(opcode, owner, name, desc);
-  }
-
-  @Override
-  public void visitFrame(final int type, final int nLocal,
-      final Object[] local, final int nStack, final Object[] stack) {
-    processDeferredAload(false);
-    super.visitFrame(type, nLocal, local, nStack, stack);
-  }
-
-  @Override
-  public void visitIincInsn(final int var, final int increment) {
-    processDeferredAload(false);
-    super.visitIincInsn(var, increment);
-  }
-
-  @Override
-  public void visitInsn(final int opcode) {
-    /*
-     * If we don't omit an ALOAD with a following POP, then forward this.
-     * In other words, if we omit an ALOAD because we're on the following POP,
-     * don't forward this POP, which omits the ALOAD-POP pair.
-     */
-    if (!processDeferredAload(Opcodes.POP == opcode)) {
-      super.visitInsn(opcode);
-    }
-  }
-
-  @Override
-  public AnnotationVisitor visitInsnAnnotation(final int typeRef,
-      final TypePath typePath, final String desc, final boolean visible) {
-    processDeferredAload(false);
-    final AnnotationVisitor annotationVisitor = super.visitInsnAnnotation(
-        typeRef, typePath, desc, visible);
-    return annotationVisitor;
-  }
-
-  @Override
-  public void visitIntInsn(final int opcode, final int operand) {
-    processDeferredAload(false);
-    super.visitIntInsn(opcode, operand);
-  }
-
-  @Override
-  public void visitInvokeDynamicInsn(final String name, final String desc,
-      final Handle bsm, final Object... bsmArgs) {
-    processDeferredAload(false);
-    super.visitInvokeDynamicInsn(name, desc, bsm, bsmArgs);
-  }
-
-  @Override
-  public void visitJumpInsn(final int opcode, final Label label) {
-    processDeferredAload(false);
-    super.visitJumpInsn(opcode, label);
-  }
-
-  @Override
-  public void visitLabel(final Label label) {
-    processDeferredAload(false);
-    super.visitLabel(label);
-  }
-
-  @Override
-  public void visitLdcInsn(final Object cst) {
-    processDeferredAload(false);
-    super.visitLdcInsn(cst);
-  }
-
-  @Override
-  public void visitLineNumber(final int line, final Label start) {
-    processDeferredAload(false);
-    super.visitLineNumber(line, start);
-  }
-
-  @Override
-  public void visitLocalVariable(final String name, final String desc,
-      final String signature, final Label start, final Label end, final int index) {
-    processDeferredAload(false);
-    super.visitLocalVariable(name, desc, signature, start, end, index);
-  }
-
-  @Override
-  public AnnotationVisitor visitLocalVariableAnnotation(final int typeRef,
-      final TypePath typePath, final Label[] start, final Label[] end,
-      final int[] index, final String desc, final boolean visible) {
-    processDeferredAload(false);
-    final AnnotationVisitor annotationVisitor =
-        super.visitLocalVariableAnnotation(typeRef, typePath, start, end, index,
-            desc, visible);
-    return annotationVisitor;
-  }
-
-  @Override
-  public void visitLookupSwitchInsn(final Label dflt, final int[] keys,
-      final Label[] labels) {
-    processDeferredAload(false);
-    super.visitLookupSwitchInsn(dflt, keys, labels);
-  }
-
-  @Override
-  public void visitMaxs(final int maxStack, final int maxLocals) {
-    processDeferredAload(false);
-    super.visitMaxs(maxStack, maxLocals);
-  }
-
-  @Deprecated
-  @Override
-  public void visitMethodInsn(final int opcode, final String owner,
-      final String name, final String desc) {
-    processDeferredAload(false);
-    super.visitMethodInsn(opcode, owner, name, desc);
-  }
-
-  @Override
-  public void visitMethodInsn(final int opcode, final String owner,
-      final String name, final String desc, final boolean itf) {
-    processDeferredAload(false);
-    super.visitMethodInsn(opcode, owner, name, desc, itf);
-  }
-
-  @Override
-  public void visitMultiANewArrayInsn(final String desc, final int dims) {
-    processDeferredAload(false);
-    super.visitMultiANewArrayInsn(desc, dims);
-  }
-
-  @Override
-  public void visitParameter(final String name, final int access) {
-    processDeferredAload(false);
-    super.visitParameter(name, access);
-  }
-
-  @Override
-  public AnnotationVisitor visitParameterAnnotation(final int parameter,
-      final String desc, final boolean visible) {
-    processDeferredAload(false);
-    final AnnotationVisitor annotationVisitor =
-        super.visitParameterAnnotation(parameter, desc, visible);
-    return annotationVisitor;
-  }
-
-  @Override
-  public void visitTableSwitchInsn(final int min, final int max,
-      final Label dflt, final Label... labels) {
-    processDeferredAload(false);
-    super.visitTableSwitchInsn(min, max, dflt, labels);
-  }
-
-  @Override
-  public AnnotationVisitor visitTryCatchAnnotation(final int typeRef,
-      final TypePath typePath, final String desc, final boolean visible) {
-    processDeferredAload(false);
-    final AnnotationVisitor annotationVisitor =
-        super.visitTryCatchAnnotation(typeRef, typePath, desc, visible);
-    return annotationVisitor;
-  }
-
-  @Override
-  public void visitTryCatchBlock(final Label start, final Label end,
-      final Label handler, final String type) {
-    processDeferredAload(false);
-    super.visitTryCatchBlock(start, end, handler, type);
-  }
-
-  @Override
-  public AnnotationVisitor visitTypeAnnotation(final int typeRef,
-      final TypePath typePath, final String desc, final boolean visible) {
-    processDeferredAload(false);
-    final AnnotationVisitor annotationVisitor =
-        super.visitTypeAnnotation(typeRef, typePath, desc, visible);
-    return annotationVisitor;
-  }
-
-  @Override
-  public void visitTypeInsn(final int opcode, final String type) {
-    processDeferredAload(false);
-    super.visitTypeInsn(opcode, type);
-  }
-
-  @Override
-  public void visitVarInsn(final int opcode, final int var) {
-    processDeferredAload(false);
-
-    // if we see an ALOAD, defer forwarding it until we know what the next instruction is
-    if (Opcodes.ALOAD == opcode) {
-      this.var = var;
-    } else {
-      super.visitVarInsn(opcode, var);
-    }
-  }
-}
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ValueHolderReplacementVisitor.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ValueHolderReplacementVisitor.java
index 1ed9ea592..9094b33a1 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ValueHolderReplacementVisitor.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ValueHolderReplacementVisitor.java
@@ -55,14 +55,8 @@ public class ValueHolderReplacementVisitor extends ClassVisitor {
       innerVisitor = new CheckMethodVisitorFsm(api, innerVisitor);
     }
 
-    /*
-     * Before using the ScalarReplacementNode to rewrite method code, use the
-     * AloadPopRemover to eliminate unnecessary ALOAD-POP pairs; see the
-     * AloadPopRemover javadoc for a detailed explanation.
-     */
-    return new AloadPopRemover(api,
-        new ScalarReplacementNode(
-            className, access, name, desc, signature, exceptions, innerVisitor, verifyBytecode));
+    return new ScalarReplacementNode(className, access, name, desc, signature,
+        exceptions,innerVisitor, verifyBytecode);
   }
 
   private static class Debugger extends MethodNode {
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/MethodGrabbingVisitor.java b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/MethodGrabbingVisitor.java
index 782e1e2b8..18cc4e47c 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/MethodGrabbingVisitor.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/MethodGrabbingVisitor.java
@@ -21,10 +21,13 @@ import java.io.StringWriter;
 import java.util.HashMap;
 import java.util.Map;
 
+import org.codehaus.commons.compiler.CompileException;
 import org.codehaus.janino.Java;
 import org.codehaus.janino.Java.AbstractClassDeclaration;
 import org.codehaus.janino.Java.MethodDeclarator;
+import org.codehaus.janino.Unparser;
 import org.codehaus.janino.util.AbstractTraverser;
+import org.codehaus.janino.util.DeepCopier;
 
 public class MethodGrabbingVisitor {
 
@@ -64,9 +67,59 @@ public class MethodGrabbingVisitor {
     @Override
     public void traverseMethodDeclarator(MethodDeclarator methodDeclarator) {
       if (captureMethods) {
+        // Generates a "labeled statement".
+        // This code takes code from the method body, wraps it into the labeled statement
+        // and replaces all the return statements by break command with label.
+        //
+        // For example, the following method
+        //    public void foo(int a) {
+        //      if (a < 0) {
+        //        return;
+        //      } else {
+        //        do something;
+        //      }
+        //    }
+        //
+        // will be converted to
+        //    MethodClassName_foo: {
+        //      if (a < 0) {
+        //        break MethodClassName_foo;
+        //      } else {
+        //        do something;
+        //      }
+        //    }
+
+        // Constructs a name of the resulting label
+        // using methods class name and method name itself.
+        String[] fQCN = methodDeclarator.getDeclaringType().getClassName().split("\\.");
+        String returnLabel = fQCN[fQCN.length - 1] + "_" + methodDeclarator.name;
+        Java.Block methodBodyBlock = new Java.Block(methodDeclarator.getLocation());
+
+        // DeepCopier implementation which returns break statement with label
+        // instead if return statement.
+        DeepCopier returnStatementReplacer = new DeepCopier() {
+          @Override
+          public Java.BlockStatement copyReturnStatement(Java.ReturnStatement subject) {
+            return new Java.BreakStatement(subject.getLocation(), returnLabel);
+          }
+        };
+        try {
+          // replaces return statements and stores the result into methodBodyBlock
+          methodBodyBlock.addStatements(
+              returnStatementReplacer.copyBlockStatements(methodDeclarator.optionalStatements));
+        } catch (CompileException e) {
+          throw new RuntimeException(e);
+        }
+
+        // wraps method code with replaced return statements into label statement.
+        Java.LabeledStatement labeledStatement =
+            new Java.LabeledStatement(methodDeclarator.getLocation(), returnLabel, methodBodyBlock);
+
+        // Unparse the labeled statement.
         StringWriter writer = new StringWriter();
-        ModifiedUnparser unparser = new ModifiedUnparser(writer);
-        unparser.visitMethodDeclarator(methodDeclarator);
+        Unparser unparser = new Unparser(writer);
+        // unparses labeledStatement and stores unparsed code into writer
+        unparser.unparseBlockStatement(labeledStatement);
         unparser.close();
         writer.flush();
         methods.put(methodDeclarator.name, writer.getBuffer().toString());
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/ModifiedUnparser.java b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/ModifiedUnparser.java
deleted file mode 100644
index e7b22447c..000000000
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/ModifiedUnparser.java
+++ /dev/null
@@ -1,110 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements.  See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership.  The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License.  You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package org.apache.drill.exec.expr.fn;
-
-import java.io.Writer;
-import java.util.List;
-
-import org.codehaus.janino.Java;
-import org.codehaus.janino.Unparser;
-import org.codehaus.janino.util.AutoIndentWriter;
-
-/**
- * This is a modified version of {@link Unparser} so that we can avoid
- * rendering few things.
- */
-public class ModifiedUnparser extends Unparser {
-
-  private String returnLabel;
-
-  public ModifiedUnparser(Writer writer) {
-    super(writer);
-  }
-
-  @Override
-  public void unparseBlockStatement(Java.BlockStatement blockStatement) {
-    // Unparser uses anonymous classes for visiting statements,
-    // therefore added this check for customizing of handling ReturnStatement.
-    if (blockStatement instanceof Java.ReturnStatement) {
-      visitReturnStatement((Java.ReturnStatement) blockStatement);
-    } else {
-      super.unparseBlockStatement(blockStatement);
-    }
-  }
-
-  /**
-   * Parses specified {@link Java.MethodDeclarator}, wraps its content
-   * with replaced {@code return} statements by {@code break} ones into the
-   * block with label and stores it into {@link java.io.PrintWriter}.
-   *
-   * @param methodDeclarator method to parse
-   */
-  public void visitMethodDeclarator(Java.MethodDeclarator methodDeclarator) {
-    if (methodDeclarator.optionalStatements == null) {
-      pw.print(';');
-    } else if (methodDeclarator.optionalStatements.isEmpty()) {
-      pw.print(" {}");
-    } else {
-      pw.println(' ');
-      // Add labels to handle return statements within function templates
-      String[] fQCN = methodDeclarator.getDeclaringType().getClassName().split("\\.");
-      returnLabel = fQCN[fQCN.length - 1] + "_" + methodDeclarator.name;
-      pw.print(returnLabel);
-      pw.println(": {");
-      pw.print(AutoIndentWriter.INDENT);
-      unparseStatements(methodDeclarator.optionalStatements);
-      pw.print(AutoIndentWriter.UNINDENT);
-      pw.println("}");
-      pw.print(' ');
-    }
-  }
-
-  private void visitReturnStatement(Java.ReturnStatement returnStatement) {
-    pw.print("break ");
-    pw.print(returnLabel);
-    if (returnStatement.optionalReturnValue != null) {
-      pw.print(' ');
-      unparseAtom(returnStatement.optionalReturnValue);
-    }
-    pw.print(';');
-  }
-
-  /**
-   * The following helper method is copied from the parent class since it
-   * is declared as private in the parent class and can not be used in the child
-   * class (this).
-   */
-  private void unparseStatements(List<? extends Java.BlockStatement> statements) {
-    int state = -1;
-    for (Java.BlockStatement bs : statements) {
-      int x = (
-        bs instanceof Java.Block ? 1 :
-          bs instanceof Java.LocalClassDeclarationStatement ? 2 :
-            bs instanceof Java.LocalVariableDeclarationStatement ? 3 :
-              bs instanceof Java.SynchronizedStatement ? 4 : 99
-      );
-      if (state != -1 && state != x) {
-        pw.println(AutoIndentWriter.CLEAR_TABULATORS);
-      }
-      state = x;
-
-      unparseBlockStatement(bs);
-      pw.println();
-    }
-  }
-}
diff --git a/pom.xml b/pom.xml
index 0cc25279f..ca847d225 100644
--- a/pom.xml
+++ b/pom.xml
@@ -48,7 +48,7 @@
     <parquet.version>1.10.0</parquet.version>
     <calcite.version>1.17.0-drill-r1</calcite.version>
     <avatica.version>1.12.0</avatica.version>
-    <janino.version>3.0.10</janino.version>
+    <janino.version>3.0.11</janino.version>
     <sqlline.version>1.5.0</sqlline.version>
     <jackson.version>2.9.5</jackson.version>
     <jackson.databind.version>2.9.5</jackson.databind.version>