This is an automated email from the ASF dual-hosted git repository. asf-gitbox-commits pushed a commit to branch GROOVY-12161 in repository https://gitbox.apache.org/repos/asf/groovy.git
commit 47a13fc0cde67baf89ab70d1725d694d85a66c53 Author: Daniel Sun <[email protected]> AuthorDate: Mon Jul 13 23:47:11 2026 +0900 GROOVY-12161: Omit identity catch-all for try/catch without finally and slim exception bytecode --- .../codehaus/groovy/classgen/asm/CompileStack.java | 18 +- .../groovy/classgen/asm/StatementWriter.java | 147 ++++++----- src/test/groovy/bugs/Groovy11362.groovy | 6 +- src/test/groovy/bugs/Groovy12161.groovy | 281 +++++++++++++++++++++ 4 files changed, 387 insertions(+), 65 deletions(-) diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java b/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java index 73e6ecdaec..b2982ccf32 100644 --- a/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java +++ b/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java @@ -931,6 +931,16 @@ public class CompileStack { applyBlockRecorder(blockRecorders); } + /** + * Closes protected ranges, inlines each finally/synchronized guard, then + * re-opens a range after the guards for any code that follows (e.g. the + * return-value reload). + * <p> + * A leading {@code NOP} keeps the closed range non-empty when the try body + * is only an abrupt exit (empty exception-table ranges are illegal). No + * trailing {@code NOP} is emitted; the restarted range binds to the + * following real instruction (load/return/goto). + */ private void applyBlockRecorder(final Collection<BlockRecorder> blockRecorders) { if (blockRecorders.isEmpty() || blockRecorders.size() == visitedBlocks.size()) return; @@ -941,19 +951,19 @@ public class CompileStack { if (visitedBlocks.contains(recorder)) continue; Label end = new Label(); + // Guarantee a non-empty protected range before excluding finally. mv.visitInsn(NOP); mv.visitLabel(end); - recorder.closeRange(end); - // we exclude the finally block from the exception table - // here to avoid double visiting of finally statements + // Exclude the finally body from the exception table so it is not + // re-entered if it throws (avoid double application of finally). recorder.excludedStatement.run(); recorder.startRange(start); } - mv.visitInsn(NOP); + // start marks the first instruction after all inlined finally blocks. mv.visitLabel(start); } diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java b/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java index 97b036a209..0db3a26312 100644 --- a/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java +++ b/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java @@ -460,9 +460,21 @@ public class StatementWriter { /** * Generates bytecode for a try/catch/finally statement. - * Handles exception table registration, finally-block inlining at every - * exit path, and a catch-all rethrow for exceptions not handled by - * any {@code catch} clause. + * <p> + * A {@link BlockRecorder} is always registered for the try (and catch) + * regions so that: + * <ul> + * <li>a non-empty finally is inlined on every abrupt exit + * ({@code return}/{@code break}/{@code continue});</li> + * <li>exception-table ranges are closed before any enclosing finally is + * inlined (required when an inner try/catch without its own finally + * sits inside an outer try/finally — see GROOVY-8229);</li> + * <li>GROOVY-9805 stack-map casts remain active for assignments in the + * region ({@link CompileStack#hasBlockRecorder()}).</li> + * </ul> + * When the finally clause is empty (plain {@code try}/{@code catch}), the + * catch-all identity rethrow and the empty shared-finally block are omitted + * so the shape matches javac more closely and stays more JIT-friendly. * * @param statement the try/catch/finally statement to compile */ @@ -475,82 +487,99 @@ public class StatementWriter { Statement tryStatement = statement.getTryStatement(); Statement finallyStatement = statement.getFinallyStatement(); + boolean hasFinally = !isEmptyStatement(finallyStatement); + List<CatchStatement> catchStatements = statement.getCatchStatements(); + BlockRecorder tryBlock = makeBlockRecorder(finallyStatement); startRange(tryBlock, mv); tryStatement.visit(controller.getAcg()); - // skip past catch block(s) - Label finallyStart = new Label(); - boolean fallthroughFinally = false; + // Destination for normal completion: shared finally when present, else + // the join point after all catch handlers. + Label afterHandlers = new Label(); + boolean fallthrough = false; if (maybeFallsThrough(tryStatement)) { - mv.visitJumpInsn(GOTO, finallyStart); - fallthroughFinally = true; + // Keeps an otherwise-empty try range non-empty and skips handlers. + mv.visitJumpInsn(GOTO, afterHandlers); + fallthrough = true; } closeRange(tryBlock, mv); - // pop for BlockRecorder + // pop for try BlockRecorder compileStack.pop(); - BlockRecorder catches = makeBlockRecorder(finallyStatement); - for (CatchStatement catchStatement : statement.getCatchStatements()) { - Label catchBlock = startRange(catches, mv); - - // create variable for the exception - compileStack.pushState(); - ClassNode type = catchStatement.getExceptionType(); - compileStack.defineVariable(catchStatement.getVariable(), type, true); - // handle catch body - catchStatement.visit(controller.getAcg()); - // placeholder to avoid problems with empty catch block - mv.visitInsn(NOP); - // pop for the variable - compileStack.pop(); - - // end of catch - closeRange(catches, mv); - if (maybeFallsThrough(catchStatement.getCode())) { - mv.visitJumpInsn(GOTO, finallyStart); - fallthroughFinally = true; + BlockRecorder catches = null; + if (!catchStatements.isEmpty()) { + catches = makeBlockRecorder(finallyStatement); + for (CatchStatement catchStatement : catchStatements) { + Label catchBlock = startRange(catches, mv); + + compileStack.pushState(); + ClassNode type = catchStatement.getExceptionType(); + compileStack.defineVariable(catchStatement.getVariable(), type, true); + catchStatement.visit(controller.getAcg()); + // Non-empty catch range / LVT span for empty catch bodies + // (defineVariable starts the LVT after the store). + mv.visitInsn(NOP); + compileStack.pop(); + + closeRange(catches, mv); + if (maybeFallsThrough(catchStatement.getCode())) { + mv.visitJumpInsn(GOTO, afterHandlers); + fallthrough = true; + } + compileStack.writeExceptionTable(tryBlock, catchBlock, BytecodeHelper.getClassInternalName(type)); } - String typeName = BytecodeHelper.getClassInternalName(type); - compileStack.writeExceptionTable(tryBlock, catchBlock, typeName); } - // used to handle exceptions in catches and regularly visited finals - Label catchAll = new Label(), afterCatchAll = new Label(); + if (hasFinally) { + // Catch-all after typed handlers so it does not supersede them. + Label catchAll = new Label(), afterCatchAll = new Label(); + compileStack.writeExceptionTable(tryBlock, catchAll, null); + if (catches != null) { + compileStack.writeExceptionTable(catches, catchAll, null); + compileStack.pop(); // catches BlockRecorder + } - // add "catch all" block to exception table for try part; we do this - // after the exception blocks so they are not superseded by this one - compileStack.writeExceptionTable(tryBlock, catchAll, null); - // same for the catch parts - compileStack.writeExceptionTable(catches , catchAll, null); + if (fallthrough) { + mv.visitLabel(afterHandlers); + finallyStatement.visit(controller.getAcg()); + // Skip over the catch-all finally/rethrow path. + mv.visitJumpInsn(GOTO, afterCatchAll); + } - // pop for BlockRecorder - compileStack.pop(); + mv.visitLabel(catchAll); + operandStack.push(ClassHelper.THROWABLE_TYPE); + int anyThrowable = compileStack.defineTemporaryVariable("throwable", ClassHelper.THROWABLE_TYPE, true); - if (fallthroughFinally) { - mv.visitLabel(finallyStart); finallyStatement.visit(controller.getAcg()); - // skip over the catch-finally-rethrow - mv.visitJumpInsn(GOTO, afterCatchAll); - } - - mv.visitLabel(catchAll); - operandStack.push(ClassHelper.THROWABLE_TYPE); - int anyThrowable = compileStack.defineTemporaryVariable("throwable", ClassHelper.THROWABLE_TYPE, true); - - finallyStatement.visit(controller.getAcg()); - - // load the throwable and rethrow it - mv.visitVarInsn(ALOAD, anyThrowable); - mv.visitInsn(ATHROW); + mv.visitVarInsn(ALOAD, anyThrowable); + mv.visitInsn(ATHROW); - if (fallthroughFinally) - mv.visitLabel(afterCatchAll); - compileStack.removeVar(anyThrowable); + if (fallthrough) { + mv.visitLabel(afterCatchAll); + } + compileStack.removeVar(anyThrowable); + } else { + // No catch-all identity rethrow — uncaught exceptions propagate. + if (catches != null) { + compileStack.pop(); // catches BlockRecorder + } + if (fallthrough) { + mv.visitLabel(afterHandlers); + } + } } + /** + * Pushes a {@link BlockRecorder} that inlines {@code finallyStatement} on + * abrupt exits from the protected region (return/break/continue), while + * excluding that inlined body from the exception table so finally is not + * applied twice. When the finally is empty the excluded statement is a + * no-op, but the recorder still splits exception-table ranges around + * enclosing finally inlines. + */ private BlockRecorder makeBlockRecorder(final Statement finallyStatement) { BlockRecorder recorder = new BlockRecorder(); final CompileStack compileStack = controller.getCompileStack(); @@ -573,7 +602,7 @@ public class StatementWriter { return label; } - private static void closeRange(final BlockRecorder br, final MethodVisitor mv) { + private static void closeRange(final BlockRecorder br, final MethodVisitor mv) { Label label = new Label(); mv.visitLabel(label); br.closeRange(label); diff --git a/src/test/groovy/bugs/Groovy11362.groovy b/src/test/groovy/bugs/Groovy11362.groovy index 55b7a375c1..025c232eb2 100644 --- a/src/test/groovy/bugs/Groovy11362.groovy +++ b/src/test/groovy/bugs/Groovy11362.groovy @@ -33,9 +33,11 @@ final class Groovy11362 extends AbstractBytecodeTestCase { } } ''' + // Catch parameter must be typed Exception (GROOVY-11362), not Object. + // Label numbers are not stable across try/catch codegen improvements. assert bytecode.hasSequence([ - 'LOCALVARIABLE this Lscript; L0 L6 0', - 'LOCALVARIABLE e Ljava/lang/Exception; L5 L3 1' // not Ljava/lang/Object; + 'LOCALVARIABLE this Lscript;', + 'LOCALVARIABLE e Ljava/lang/Exception;' // not Ljava/lang/Object; ]) } } diff --git a/src/test/groovy/bugs/Groovy12161.groovy b/src/test/groovy/bugs/Groovy12161.groovy new file mode 100644 index 0000000000..f91120f578 --- /dev/null +++ b/src/test/groovy/bugs/Groovy12161.groovy @@ -0,0 +1,281 @@ +/* + * 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 bugs + +import org.codehaus.groovy.classgen.asm.AbstractBytecodeTestCase +import org.codehaus.groovy.classgen.asm.InstructionSequence +import org.junit.jupiter.api.Test +import org.objectweb.asm.ClassReader +import org.objectweb.asm.tree.ClassNode +import org.objectweb.asm.tree.MethodNode +import org.objectweb.asm.tree.TryCatchBlockNode + +/** + * Bytecode-shape checks for try/catch/finally emission in {@link StatementWriter}. + * Plain try/catch (empty finally) must not emit a catch-all identity rethrow; + * try/finally must keep a catch-all so uncaught exceptions still run finally. + */ +final class Groovy12161 extends AbstractBytecodeTestCase { + + @Test + void testTryCatchWithoutFinallyHasNoCatchAllHandler() { + compile method: 'm', ''' + int m(int x) { + try { + return x + } catch (RuntimeException e) { + return -1 + } + } + ''' + + def blocks = tryCatchBlocks('m') + assert blocks.every { it.type != null }: "unexpected catch-all in\n${sequence}" + assert blocks.every { it.type == 'java/lang/RuntimeException' } + assert !hasIdentityCatchAllRethrow(sequence) + } + + @Test + void testMultiCatchWithoutFinallyRegistersOnlyTypedHandlers() { + compile method: 'm', ''' + int m(int x) { + try { + return x + } catch (IllegalArgumentException e) { + return -1 + } catch (RuntimeException e) { + return -2 + } + } + ''' + + def blocks = tryCatchBlocks('m') + assert blocks.every { it.type != null } + assert blocks*.type as Set == ['java/lang/IllegalArgumentException', 'java/lang/RuntimeException'] as Set + } + + @Test + void testTryFinallyKeepsCatchAllForUncaughtExceptions() { + compile method: 'm', ''' + int m(int x) { + int y = 0 + try { + y = x + } finally { + y = y + 1 + } + return y + } + ''' + + def blocks = tryCatchBlocks('m') + assert blocks.any { it.type == null }: "expected catch-all for non-empty finally\n${sequence}" + assert new GroovyShell().evaluate(''' + int m(int x) { + int y = 0 + try { + y = x + } finally { + y = y + 1 + } + return y + } + m(41) + ''') == 42 + } + + @Test + void testTryCatchFinallySemanticsAndCatchAll() { + def source = ''' + int m(int x) { + def side = 0 + try { + if (x < 0) throw new IllegalArgumentException('neg') + return x + } catch (IllegalArgumentException e) { + return -1 + } finally { + side = side + 1 + } + } + assert m(7) == 7 + assert m(-3) == -1 + m(7) + ''' + assert new GroovyShell().evaluate(source) == 7 + + compile method: 'm', ''' + int m(int x) { + try { + if (x < 0) throw new IllegalArgumentException('neg') + return x + } catch (IllegalArgumentException e) { + return -1 + } finally { + x = x + 1 + } + } + ''' + def blocks = tryCatchBlocks('m') + assert blocks.any { it.type == 'java/lang/IllegalArgumentException' } + assert blocks.any { it.type == null } + } + + @Test + void testTryFinallyWithReturnInlinesFinallyWithoutTrailingNops() { + def bytecode = compile(method: 'm', ''' + int m(int x) { + try { + return x + } finally { + x = x + } + } + ''') + + // Leading NOP may remain so the protected range before an inlined finally + // is non-empty; trailing NOP after the finally (old applyBlockRecorder + // shape) should not appear as NOP; load; return. + assert !bytecode.hasStrictSequence(['NOP', 'ILOAD', 'IRETURN']) + assert new GroovyShell().evaluate(''' + def holder = new Object() { + def log = [] + int m(int x) { + try { + return x + } finally { + log << 'f' + } + } + } + assert holder.m(3) == 3 + holder.log + ''') == ['f'] + } + + @Test + void testBreakInTryRunsFinally() { + assert new GroovyShell().evaluate(''' + def called = false + while (true) { + try { + break + } finally { + called = true + } + } + called + ''') + } + + @Test + void testEmptyCatchWithoutFinally() { + assert new GroovyShell().evaluate(''' + int m(int x) { + try { + if (x < 0) throw new RuntimeException('x') + return x + } catch (RuntimeException e) { + } + return -1 + } + assert m(2) == 2 + assert m(-1) == -1 + m(2) + ''') == 2 + } + + @Test + void testTryCatchFallthroughWithoutFinally() { + assert new GroovyShell().evaluate(''' + int m(int x) { + try { + x = x + 1 + } catch (Exception e) { + x = -1 + } + return x + } + m(10) + ''') == 11 + } + + @Test + void testNestedTryCatchInsideFinallyRunsFinallyOnce() { + // GROOVY-8229: outer finally must not be re-entered via an inner catch + // that incorrectly covers the outer finally's inlined body. + assert new GroovyShell().evaluate(''' + class TryCatchProblem { + static int count = 0 + static void main(args) { + def cl = { + try { + try { + assert count == 0 + } catch (Throwable e) { } + } finally { + check() + } + } + cl() + } + static void check() { + throw new UnsupportedOperationException("check call count: ${++count}") + } + } + try { + TryCatchProblem.main() + return 'no throw' + } catch (UnsupportedOperationException e) { + return e.message + } + ''') == 'check call count: 1' + } + + //-------------------------------------------------------------------------- + + private List<TryCatchBlockNode> tryCatchBlocks(final String methodName) { + assert classBytes != null: 'compile(...) first' + def cn = new ClassNode() + new ClassReader(classBytes).accept(cn, ClassReader.SKIP_DEBUG) + MethodNode mn = cn.methods.find { it.name == methodName } + assert mn != null: "method ${methodName} not found" + mn.tryCatchBlocks + } + + private static boolean hasIdentityCatchAllRethrow(final InstructionSequence seq) { + // Pattern produced by the old empty-finally path: store throwable, reload, athrow + // with no intervening finally body (adjacent ALOAD/ATHROW after ASTORE). + def ops = opcodeNames(seq) + for (int i = 0; i < ops.size() - 2; i += 1) { + if (ops[i].startsWith('ASTORE') && ops[i + 1].startsWith('ALOAD') && ops[i + 2] == 'ATHROW') { + return true + } + } + false + } + + private static List<String> opcodeNames(final InstructionSequence seq) { + seq.instructions.findAll { it && !it.startsWith('//') && !it.startsWith('L') && it != '--BEGIN--' && it != '--END--' } + .collect { line -> + def token = line.tokenize()[0] + token.startsWith('FRAME') ? 'FRAME' : token + } + } +}
