This is an automated email from the ASF dual-hosted git repository.
paulk-asert pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/master by this push:
new 14c1c9f836 GROOVY-12161: Omit identity catch-all for try/catch without
finally and slim exception bytecode
14c1c9f836 is described below
commit 14c1c9f8369314452f37afed16bfba71a76e5d5f
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
+ }
+ }
+}
