8356407: Part of class verification is skipped in AOT training run

Co-authored-by: Vladimir Ivanov <[email protected]>
Reviewed-by: matsaave, coleenp

Author: iklam

src/hotspot/share/classfile/systemDictionaryShared.cpp

@@ -752,29 +752,43 @@ void SystemDictionaryShared::dumptime_classes_do(MetaspaceClosure* it) {
   }
 }
 
-bool SystemDictionaryShared::add_verification_constraint(InstanceKlass* k, Symbol* name,
-         Symbol* from_name, bool from_field_is_protected, bool from_is_array, bool from_is_object) {
+// Called from VerificationType::is_reference_assignable_from() before performing the assignability check of
+//     T1 must be assignable from T2
+// Where:
+//     L is the class loader of <k>
+//     T1 is the type resolved by L using the name <name>
+//     T2 is the type resolved by L using the name <from_name>
+//
+// The meaning of (*skip_assignability_check):
+//     true:  is_reference_assignable_from() should SKIP the assignability check
+//     false: is_reference_assignable_from() should COMPLETE the assignability check
+void SystemDictionaryShared::add_verification_constraint(InstanceKlass* k, Symbol* name,
+         Symbol* from_name, bool from_field_is_protected, bool from_is_array, bool from_is_object,
+         bool* skip_assignability_check) {
   assert(CDSConfig::is_dumping_archive(), "sanity");
   DumpTimeClassInfo* info = get_info(k);
   info->add_verification_constraint(k, name, from_name, from_field_is_protected,
                                     from_is_array, from_is_object);
 
-  if (CDSConfig::is_dumping_dynamic_archive()) {
-    // For dynamic dumping, we can resolve all the constraint classes for all class loaders during
-    // the initial run prior to creating the archive before vm exit. We will also perform verification
-    // check when running with the archive.
-    return false;
+  if (CDSConfig::is_dumping_classic_static_archive() && !is_builtin(k)) {
+    // This applies ONLY to the "classic" CDS static dump, which reads the list of
+    // unregistered classes (those intended for custom class loaders) from the classlist
+    // and loads them using jdk.internal.misc.CDS$UnregisteredClassLoader.
+    //
+    // When the classlist contains an unregistered class k, the supertypes of k are also
+    // recorded in the classlist. However, the classlist does not contain information about
+    // any class X that's not a supertype of k but is needed in the verification of k.
+    // As a result, CDS$UnregisteredClassLoader will not know how to resolve X.
+    //
+    // Therefore, we tell the verifier to refrain from resolving X. Instead, X is recorded
+    // (symbolically) in the verification constraints of k. In the production run,
+    // when k is loaded, we will go through its verification constraints and resolve X to complete
+    // the is_reference_assignable_from() checks.
+    *skip_assignability_check = true;
   } else {
-    if (is_builtin(k)) {
-      // For builtin class loaders, we can try to complete the verification check at dump time,
-      // because we can resolve all the constraint classes. We will also perform verification check
-      // when running with the archive.
-      return false;
-    } else {
-      // For non-builtin class loaders, we cannot complete the verification check at dump time,
-      // because at dump time we don't know how to resolve classes for such loaders.
-      return true;
-    }
+    // In all other cases, we are using an *actual* class loader to load k, so it should be able
+    // to resolve any types that are needed for the verification of k.
+    *skip_assignability_check = false;
   }
 }
 

src/hotspot/share/classfile/systemDictionaryShared.hpp

@@ -233,9 +233,10 @@ class SystemDictionaryShared: public SystemDictionary {
   // ensures that you cannot load a shared class if its super type(s) are changed. However,
   // we need an additional check to ensure that the verification_constraints did not change
   // between dump time and runtime.
-  static bool add_verification_constraint(InstanceKlass* k, Symbol* name,
+  static void add_verification_constraint(InstanceKlass* k, Symbol* name,
                   Symbol* from_name, bool from_field_is_protected,
-                  bool from_is_array, bool from_is_object) NOT_CDS_RETURN_(false);
+                  bool from_is_array, bool from_is_object,
+                  bool* skip_assignability_check);
   static void check_verification_constraints(InstanceKlass* klass,
                                              TRAPS) NOT_CDS_RETURN;
   static void add_enum_klass_static_field(InstanceKlass* ik, int root_index);

src/hotspot/share/classfile/verificationType.cpp

@@ -106,16 +106,19 @@ bool VerificationType::is_reference_assignable_from(
       return true;
     }
 
+#if INCLUDE_CDS
     if (CDSConfig::is_dumping_archive()) {
-      if (SystemDictionaryShared::add_verification_constraint(klass,
+      bool skip_assignability_check = false;
+      SystemDictionaryShared::add_verification_constraint(klass,
               name(), from.name(), from_field_is_protected, from.is_array(),
-              from.is_object())) {
-        // If add_verification_constraint() returns true, the resolution/check should be
-        // delayed until runtime.
+              from.is_object(), &skip_assignability_check);
+      if (skip_assignability_check) {
+        // We are not able to resolve name() or from.name(). The actual assignability check
+        // will be delayed until runtime.
         return true;
       }
     }
-
+#endif
     return resolve_and_check_assignability(klass, name(), from.name(),
           from_field_is_protected, from.is_array(), from.is_object(), THREAD);
   } else if (is_array() && from.is_array()) {

test/hotspot/jtreg/runtime/cds/appcds/aotCache/AOTCacheSupportForCustomLoaders.java

@@ -29,10 +29,11 @@
  * @requires vm.cds.supports.aot.class.linking
  * @comment work around JDK-8345635
  * @requires !vm.jvmci.enabled
- * @library /test/lib
+ * @library /test/lib /test/hotspot/jtreg/runtime/cds/appcds/test-classes
+ * @build ReturnIntegerAsString
  * @build AOTCacheSupportForCustomLoaders
  * @run driver jdk.test.lib.helpers.ClassFileInstaller -jar app.jar AppWithCustomLoaders AppWithCustomLoaders$MyLoader
- * @run driver jdk.test.lib.helpers.ClassFileInstaller -jar cust.jar AppWithCustomLoaders$MyLoadeeA AppWithCustomLoaders$MyLoadeeB
+ * @run driver jdk.test.lib.helpers.ClassFileInstaller -jar cust.jar AppWithCustomLoaders$MyLoadeeA AppWithCustomLoaders$MyLoadeeB ReturnIntegerAsString
  * @run driver AOTCacheSupportForCustomLoaders AOT
  */
 
@@ -50,7 +51,8 @@ public static void main(String... args) throws Exception {
             .appCommandLine("AppWithCustomLoaders")
             .setAssemblyChecker((OutputAnalyzer out) -> {
                     out.shouldMatch("cds,class.*unreg AppWithCustomLoaders[$]MyLoadeeA")
-                       .shouldMatch("cds,class.*array \\[LAppWithCustomLoaders[$]MyLoadeeA;");
+                       .shouldMatch("cds,class.*array \\[LAppWithCustomLoaders[$]MyLoadeeA;")
+                       .shouldNotMatch("cds,class.* ReturnIntegerAsString");
                 })
             .setProductionChecker((OutputAnalyzer out) -> {
                     out.shouldContain("Using AOT-linked classes: true");
@@ -69,6 +71,16 @@ public static void main(String args[]) throws Exception {
         Class klass = loader.loadClass("AppWithCustomLoaders$MyLoadeeA");
         klass.newInstance();
 
+        // Test 2: VerificationType::is_reference_assignable_from() cannot be skipped (JDK-8356407)
+        try {
+            Class bad = loader.loadClass("ReturnIntegerAsString");
+            Object o = bad.newInstance(); // force verification
+            System.out.println("Expected String but got: " + o.toString().getClass());
+            throw new RuntimeException("VerifyError expected but not thrown");
+        } catch (VerifyError ve) {
+            System.out.println("Expected: " + ve);
+        }
+
         // TODO: more test cases JDK-8354557
     }
 

test/hotspot/jtreg/runtime/cds/appcds/test-classes/ReturnIntegerAsString.jasm

@@ -0,0 +1,56 @@
+/*
+ * Copyright (c) 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ *
+ */
+
+
+/*
+
+class ReturnIntegerAsString {
+    String doit() {
+        return new Integer(1);
+    }
+}
+
+*/
+
+public super class ReturnIntegerAsString
+    version 60:0
+{
+  public Method "<init>":"()V"
+    stack 1 locals 1
+  {
+        aload_0;
+        invokespecial    Method java/lang/Object."<init>":"()V";
+        return;
+  }
+  public Method toString:"()Ljava/lang/String;"
+    stack 3 locals 2
+  {
+        new              class java/lang/Integer;
+        dup;
+        iconst_1;
+        invokespecial    Method java/lang/Integer."<init>":"(I)V";
+        areturn;
+  }
+
+} // end Class ReturnIntegerAsString