From 0b64cc2fb1c7605fbadefb3166c75d6f97d5a77f Mon Sep 17 00:00:00 2001 From: Daniel Sun Date: Fri, 24 Feb 2017 16:39:59 +0800 Subject: [PATCH 1/2] GROOVY-8085: Exception in "finally" not caught by outer "try" --- .../groovy/classgen/asm/CompileStack.java | 15 +++--- src/test/groovy/bugs/Groovy8085Bug.groovy | 49 +++++++++++++++++++ 2 files changed, 57 insertions(+), 7 deletions(-) create mode 100644 src/test/groovy/bugs/Groovy8085Bug.groovy diff --git a/src/main/org/codehaus/groovy/classgen/asm/CompileStack.java b/src/main/org/codehaus/groovy/classgen/asm/CompileStack.java index 73973071ddf..d793bdf68e8 100644 --- a/src/main/org/codehaus/groovy/classgen/asm/CompileStack.java +++ b/src/main/org/codehaus/groovy/classgen/asm/CompileStack.java @@ -772,12 +772,12 @@ private void applyBlockRecorder(List blocks) { MethodVisitor mv = controller.getMethodVisitor(); - Label end = new Label(); - mv.visitInsn(NOP); - mv.visitLabel(end); - Label newStart = new Label(); - for (BlockRecorder fb : blocks) { + Label end = new Label(); + mv.visitInsn(NOP); + mv.visitLabel(end); + Label newStart = new Label(); + if (visitedBlocks.contains(fb)) continue; fb.closeRange(end); @@ -787,10 +787,11 @@ private void applyBlockRecorder(List blocks) { fb.excludedStatement.run(); fb.startRange(newStart); + + mv.visitInsn(NOP); + mv.visitLabel(newStart); } - mv.visitInsn(NOP); - mv.visitLabel(newStart); } public void applyBlockRecorder() { diff --git a/src/test/groovy/bugs/Groovy8085Bug.groovy b/src/test/groovy/bugs/Groovy8085Bug.groovy new file mode 100644 index 00000000000..40b222287e3 --- /dev/null +++ b/src/test/groovy/bugs/Groovy8085Bug.groovy @@ -0,0 +1,49 @@ +/* + * 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 groovy.bugs + +class Groovy8085Bug extends GroovyTestCase { + void testTryCatchFinally() { + assertScript ''' + try { + try { + true + } finally { + 99 / 0 + } + } catch (Exception e) { + System.out.println("catch!!!"); + } + ''' + } + + void testTryCatchFinallyWithExplicitReturn() { + assertScript ''' + try { + try { + return true + } finally { + 99 / 0 + } + } catch (Exception e) { + System.out.println("catch!!!"); + } + ''' + } +} From 427233fd16246f8444c94cfde5d052fc6fa7f30c Mon Sep 17 00:00:00 2001 From: Daniel Sun Date: Fri, 24 Feb 2017 23:25:26 +0800 Subject: [PATCH 2/2] Avoid generating useless bytecode --- .../groovy/classgen/asm/CompileStack.java | 8 ++-- src/test/groovy/bugs/Groovy8085Bug.groovy | 42 +++++++++++++++++++ 2 files changed, 46 insertions(+), 4 deletions(-) diff --git a/src/main/org/codehaus/groovy/classgen/asm/CompileStack.java b/src/main/org/codehaus/groovy/classgen/asm/CompileStack.java index d793bdf68e8..ba856bceca1 100644 --- a/src/main/org/codehaus/groovy/classgen/asm/CompileStack.java +++ b/src/main/org/codehaus/groovy/classgen/asm/CompileStack.java @@ -257,7 +257,7 @@ public VariableScope getScope() { * @param store defines if the toplevel argument of the stack should be stored * @return the index used for this temporary variable */ - public int defineTemporaryVariable(org.codehaus.groovy.ast.Variable var, boolean store) { + public int defineTemporaryVariable(Variable var, boolean store) { return defineTemporaryVariable(var.getName(), var.getType(),store); } @@ -773,12 +773,11 @@ private void applyBlockRecorder(List blocks) { MethodVisitor mv = controller.getMethodVisitor(); for (BlockRecorder fb : blocks) { + if (visitedBlocks.contains(fb)) continue; + Label end = new Label(); mv.visitInsn(NOP); mv.visitLabel(end); - Label newStart = new Label(); - - if (visitedBlocks.contains(fb)) continue; fb.closeRange(end); @@ -786,6 +785,7 @@ private void applyBlockRecorder(List blocks) { // here to avoid double visiting of finally statements fb.excludedStatement.run(); + Label newStart = new Label(); fb.startRange(newStart); mv.visitInsn(NOP); diff --git a/src/test/groovy/bugs/Groovy8085Bug.groovy b/src/test/groovy/bugs/Groovy8085Bug.groovy index 40b222287e3..f1a9310c99f 100644 --- a/src/test/groovy/bugs/Groovy8085Bug.groovy +++ b/src/test/groovy/bugs/Groovy8085Bug.groovy @@ -33,6 +33,27 @@ class Groovy8085Bug extends GroovyTestCase { ''' } + void testTryCatchFinally2() { + assertScript ''' + def visitSequence = [] + try { + try { + true + } finally { + visitSequence << 'innerFinally' + 99 / 0 + } + } catch (Exception e) { + visitSequence << 'outerCatch' + System.out.println("catch!!!"); + } finally { + visitSequence << 'outerFinally' + } + + assert ['innerFinally', 'outerCatch', 'outerFinally'] == visitSequence + ''' + } + void testTryCatchFinallyWithExplicitReturn() { assertScript ''' try { @@ -46,4 +67,25 @@ class Groovy8085Bug extends GroovyTestCase { } ''' } + + void testTryCatchFinallyWithExplicitReturn2() { + assertScript ''' + def visitSequence = [] + try { + try { + return true + } finally { + visitSequence << 'innerFinally' + 99 / 0 + } + } catch (Exception e) { + visitSequence << 'outerCatch' + System.out.println("catch!!!"); + } finally { + visitSequence << 'outerFinally' + } + + assert ['innerFinally', 'outerCatch', 'outerFinally'] == visitSequence + ''' + } }