8361638: java.lang.classfile.CodeBuilder.CatchBuilder should not throw IllegalArgumentException for representable exception handlers

liach · liach · commit 6fb6f3d39b32 · 2025-08-15T04:25:37.000Z
Reviewed-by: asotona
diff --git a/src/java.base/share/classes/java/lang/classfile/CodeBuilder.java b/src/java.base/share/classes/java/lang/classfile/CodeBuilder.java
@@ -330,6 +330,11 @@ default CodeBuilder ifThenElse(Opcode opcode,
 
     /**
      * A builder to add catch blocks.
+     * <p>
+     * The order of catch blocks is significant.  When an exception is thrown
+     * by the try block, the first catch block whose exception type is {@linkplain
+     * Class#isAssignableFrom(Class) the same class as or a superclass of} the
+     * class of exception thrown is branched to (JVMS {@jvms 2.10}).
      *
      * @see #trying
      * @see ExceptionCatch
@@ -348,13 +353,16 @@ sealed interface CatchBuilder permits CatchBuilderImpl {
          * If the type of exception is {@code null} then the catch block catches
          * all exceptions.
          *
+         * @apiNote
+         * If the type of exception to catch is already handled by previous
+         * catch blocks, this block will never be executed.
+         *
          * @param exceptionType the type of exception to catch, may be {@code null}
          * @param catchHandler handler that receives a {@link BlockCodeBuilder} to
          *                     generate the body of the catch block
          * @return this builder
-         * @throws IllegalArgumentException if an existing catch block catches
-         *         an exception of the given type or {@code exceptionType}
-         *         represents a primitive type
+         * @throws IllegalArgumentException if {@code exceptionType} represents
+         *         a primitive type
          * @see #catchingMulti
          * @see #catchingAll
          */
@@ -372,12 +380,16 @@ sealed interface CatchBuilder permits CatchBuilderImpl {
          * If list of exception types is empty then the catch block catches all
          * exceptions.
          *
+         * @apiNote
+         * If every type of exception to catch is already handled by previous
+         * catch blocks, this block will never be executed.
+         *
          * @param exceptionTypes the types of exception to catch
          * @param catchHandler handler that receives a {@link BlockCodeBuilder}
          *                     to generate the body of the catch block
          * @return this builder
-         * @throws IllegalArgumentException if an existing catch block catches
-         *         one or more exceptions of the given types
+         * @throws IllegalArgumentException if any exception type represents a
+         *         primitive type
          * @see #catching
          * @see #catchingAll
          */
@@ -392,10 +404,12 @@ sealed interface CatchBuilder permits CatchBuilderImpl {
          * The caught exception will be on top of the operand stack when the
          * catch block is entered.
          *
+         * @apiNote
+         * Since this block intercepts all exceptions, all subsequent catch
+         * blocks will never be executed.
+         *
          * @param catchAllHandler handler that receives a {@link BlockCodeBuilder}
          *                        to generate the body of the catch block
-         * @throws IllegalArgumentException if an existing catch block catches
-         *         all exceptions
          * @see #catching
          * @see #catchingMulti
          */
diff --git a/src/java.base/share/classes/java/lang/classfile/instruction/ExceptionCatch.java b/src/java.base/share/classes/java/lang/classfile/instruction/ExceptionCatch.java
@@ -39,8 +39,10 @@
  * A pseudo-instruction modeling an entry in the {@code exception_table} array
  * of a {@link CodeAttribute Code} attribute.  Catch (JVMS {@jvms 3.12}) and
  * finally (JVMS {@jvms 3.14}) blocks in Java source code compile to exception
- * table entries.  Delivered as a {@link CodeElement} when traversing the
- * contents of a {@link CodeModel}.
+ * table entries.  The order of exception table entries is significant: when an
+ * exception is thrown in a method, execution branches to the first matching
+ * exception handler if such a handler exists (JVMS {@jvms 2.10}). Delivered as
+ * a {@link CodeElement} when traversing the contents of a {@link CodeModel}.
  * <p>
  * An exception table entry is composite:
  * {@snippet lang=text :
diff --git a/src/java.base/share/classes/jdk/internal/classfile/impl/CatchBuilderImpl.java b/src/java.base/share/classes/jdk/internal/classfile/impl/CatchBuilderImpl.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2022, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -27,8 +27,9 @@
 import java.lang.classfile.CodeBuilder;
 import java.lang.classfile.Label;
 import java.lang.classfile.Opcode;
+import java.lang.classfile.constantpool.ClassEntry;
 import java.lang.constant.ClassDesc;
-import java.lang.constant.ConstantDesc;
+import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
@@ -39,14 +40,12 @@ public final class CatchBuilderImpl implements CodeBuilder.CatchBuilder {
     final CodeBuilder b;
     final BlockCodeBuilderImpl tryBlock;
     final Label tryCatchEnd;
-    final Set<ConstantDesc> catchTypes;
     BlockCodeBuilderImpl catchBlock;
 
     public CatchBuilderImpl(CodeBuilder b, BlockCodeBuilderImpl tryBlock, Label tryCatchEnd) {
         this.b = b;
         this.tryBlock = tryBlock;
         this.tryCatchEnd = tryCatchEnd;
-        this.catchTypes = new HashSet<>();
     }
 
     @Override
@@ -59,15 +58,21 @@ public CodeBuilder.CatchBuilder catchingMulti(List<ClassDesc> exceptionTypes, Co
         Objects.requireNonNull(exceptionTypes);
         Objects.requireNonNull(catchHandler);
 
-        if (catchBlock == null) {
-            if (tryBlock.reachable()) {
-                b.branch(Opcode.GOTO, tryCatchEnd);
+        // nullable list of CP entries - null means catching all (0)
+        List<ClassEntry> entries = new ArrayList<>(Math.max(1, exceptionTypes.size()));
+        if (exceptionTypes.isEmpty()) {
+            entries.add(null);
+        } else {
+            for (var exceptionType : exceptionTypes) {
+                var entry = b.constantPool().classEntry(exceptionType); // throws IAE
+                entries.add(entry);
             }
         }
+        // End validation
 
-        for (var exceptionType : exceptionTypes) {
-            if (!catchTypes.add(exceptionType)) {
-                throw new IllegalArgumentException("Existing catch block catches exception of type: " + exceptionType);
+        if (catchBlock == null) {
+            if (tryBlock.reachable()) {
+                b.branch(Opcode.GOTO, tryCatchEnd);
             }
         }
 
@@ -82,13 +87,9 @@ public CodeBuilder.CatchBuilder catchingMulti(List<ClassDesc> exceptionTypes, Co
         catchBlock = new BlockCodeBuilderImpl(b, tryCatchEnd);
         Label tryStart = tryBlock.startLabel();
         Label tryEnd = tryBlock.endLabel();
-        if (exceptionTypes.isEmpty()) {
-            catchBlock.exceptionCatchAll(tryStart, tryEnd, catchBlock.startLabel());
-        }
-        else {
-            for (var exceptionType : exceptionTypes) {
-                catchBlock.exceptionCatch(tryStart, tryEnd, catchBlock.startLabel(), exceptionType);
-            }
+        for (var entry : entries) {
+            // This accepts null for catching all
+            catchBlock.exceptionCatch(tryStart, tryEnd, catchBlock.startLabel(), entry);
         }
         catchBlock.start();
         catchHandler.accept(catchBlock);
diff --git a/test/jdk/jdk/classfile/BuilderTryCatchTest.java b/test/jdk/jdk/classfile/BuilderTryCatchTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2022, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -23,6 +23,7 @@
 
 /*
  * @test
+ * @bug 8361638
  * @summary Testing ClassFile builder blocks.
  * @run junit BuilderTryCatchTest
  */
@@ -37,6 +38,7 @@
 
 import static java.lang.classfile.ClassFile.ACC_PUBLIC;
 import static java.lang.classfile.ClassFile.ACC_STATIC;
+import static java.lang.constant.ConstantDescs.*;
 import static org.junit.jupiter.api.Assertions.*;
 import org.junit.jupiter.api.Test;
 
@@ -47,20 +49,40 @@
 import java.lang.invoke.MethodType;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.Collections;
 import java.util.List;
 import java.util.function.Consumer;
 
-import static java.lang.constant.ConstantDescs.CD_Double;
-import static java.lang.constant.ConstantDescs.CD_Integer;
-import static java.lang.constant.ConstantDescs.CD_Object;
-import static java.lang.constant.ConstantDescs.CD_String;
-
 class BuilderTryCatchTest {
 
     static final ClassDesc CD_IOOBE = IndexOutOfBoundsException.class.describeConstable().get();
     static final ClassDesc CD_NPE = NullPointerException.class.describeConstable().get();
     static final MethodTypeDesc MTD_String = MethodType.methodType(String.class).describeConstable().get();
 
+    @Test
+    void testExceptionalContracts() throws Throwable {
+        generateTryCatchMethod(catchBuilder -> {
+            Consumer<CodeBuilder.BlockCodeBuilder> handler = tb -> tb.pop().aconst_null().areturn();
+            assertThrows(NullPointerException.class, () -> catchBuilder.catching(CD_NPE, null));
+            assertThrows(NullPointerException.class, () -> catchBuilder.catchingMulti(null, handler));
+            assertThrows(NullPointerException.class, () -> catchBuilder.catchingMulti(List.of(), null));
+            assertThrows(NullPointerException.class, () -> catchBuilder.catchingMulti(Collections.singletonList(null), null));
+            assertThrows(NullPointerException.class, () -> catchBuilder.catchingAll(null));
+            catchBuilder.catchingMulti(List.of(CD_IOOBE, CD_NPE), tb -> {
+                tb.invokevirtual(CD_Object, "toString", MTD_String);
+                tb.astore(1);
+            });
+            catchBuilder.catchingAll(tb -> tb.pop().loadConstant("all").areturn());
+            assertThrows(IllegalArgumentException.class, () -> catchBuilder.catching(CD_int, handler));
+            assertDoesNotThrow(() -> catchBuilder.catching(CD_NPE, handler));
+            assertDoesNotThrow(() -> catchBuilder.catching(null, handler));
+            assertDoesNotThrow(() -> catchBuilder.catchingMulti(List.of(), handler));
+            assertDoesNotThrow(() -> catchBuilder.catchingMulti(List.of(CD_Exception, CD_IOOBE), handler));
+            assertThrows(IllegalArgumentException.class, () -> catchBuilder.catchingMulti(List.of(CD_long, CD_Throwable), handler));
+            assertDoesNotThrow(() -> catchBuilder.catchingAll(handler));
+        });
+    }
+
     @Test
     void testTryCatchCatchAll() throws Throwable {
         byte[] bytes = generateTryCatchMethod(catchBuilder -> {
