8246546: Simplify SystemDictionary::is_shared_class_visible

Reviewed-by: minqi, ccheung

src/hotspot/share/classfile/moduleEntry.cpp

@@ -29,6 +29,7 @@
 #include "classfile/javaClasses.inline.hpp"
 #include "classfile/moduleEntry.hpp"
 #include "logging/log.hpp"
+#include "memory/filemap.hpp"
 #include "memory/resourceArea.hpp"
 #include "memory/universe.hpp"
 #include "oops/oopHandle.inline.hpp"
@@ -55,6 +56,9 @@ void ModuleEntry::set_location(Symbol* location) {
 
   if (location != NULL) {
     location->increment_refcount();
+    CDS_ONLY(if (UseSharedSpaces) {
+        _shared_path_index = FileMapInfo::get_module_shared_path_index(location);
+      });
   }
 }
 

src/hotspot/share/classfile/moduleEntry.hpp

@@ -69,6 +69,7 @@ private:
   GrowableArray<ModuleEntry*>* _reads; // list of modules that are readable by this module
   Symbol* _version;                    // module version number
   Symbol* _location;                   // module location
+  CDS_ONLY(int _shared_path_index;)    // >=0 if classes in this module are in CDS archive
   bool _can_read_all_unnamed;
   bool _has_default_read_edges;        // JVMTI redefine/retransform support
   bool _must_walk_reads;               // walk module's reads list at GC safepoints to purge out dead modules
@@ -90,6 +91,7 @@ public:
     _must_walk_reads = false;
     _is_patched = false;
     _is_open = false;
+    CDS_ONLY(_shared_path_index = -1);
   }
 
   Symbol*          name() const                        { return literal(); }
@@ -154,6 +156,7 @@ public:
 
   void set_is_patched() {
       _is_patched = true;
+      CDS_ONLY(_shared_path_index = -1); // Mark all shared classes in this module invisible.
   }
   bool is_patched() {
       return _is_patched;
@@ -182,6 +185,8 @@ public:
   void print(outputStream* st = tty);
   void verify();
 
+  CDS_ONLY(int shared_path_index() { return _shared_path_index;})
+
   JFR_ONLY(DEFINE_TRACE_ID_METHODS;)
 };
 

src/hotspot/share/classfile/systemDictionary.cpp

@@ -1241,31 +1241,44 @@ InstanceKlass* SystemDictionary::load_shared_boot_class(Symbol* class_name,
   return NULL;
 }
 
-// Check if a shared class can be loaded by the specific classloader:
+// Check if a shared class can be loaded by the specific classloader.
-//
-// NULL classloader:
-//   - Module class from "modules" jimage. ModuleEntry must be defined in the classloader.
-//   - Class from -Xbootclasspath/a. The class has no defined PackageEntry, or must
-//     be defined in an unnamed module.
 bool SystemDictionary::is_shared_class_visible(Symbol* class_name,
                                                InstanceKlass* ik,
                                                PackageEntry* pkg_entry,
                                                Handle class_loader, TRAPS) {
   assert(!ModuleEntryTable::javabase_moduleEntry()->is_patched(),
          "Cannot use sharing if java.base is patched");
-  if (ik->shared_classpath_index() < 0) {
+
-    // path_index < 0 indicates that the class is intended for a custom loader
+  // (1) Check if we are loading into the same loader as in dump time.
-    // and should not be loaded by boot/platform/app loaders
+
-    if (is_builtin_class_loader(class_loader())) {
+  if (ik->is_shared_boot_class()) {
+    if (class_loader() != NULL) {
+      return false;
+    }
+  } else if (ik->is_shared_platform_class()) {
+    if (class_loader() != java_platform_loader()) {
+      return false;
+    }
+  } else if (ik->is_shared_app_class()) {
+    if (class_loader() != java_system_loader()) {
+      return false;
+    }
+  } else {
+    // ik was loaded by a custom loader during dump time
+    if (class_loader_data(class_loader)->is_builtin_class_loader_data()) {
       return false;
     } else {
       return true;
     }
   }
 
-  // skip class visibility check
+  // (2) Check if we are loading into the same module from the same location as in dump time.
+
   if (MetaspaceShared::use_optimized_module_handling()) {
-    assert(SystemDictionary::is_shared_class_visible_impl(class_name, ik, pkg_entry, class_loader, THREAD), "Optimizing module handling failed.");
+    // Class visibility has not changed between dump time and run time, so a class
+    // that was visible (and thus archived) during dump time is always visible during runtime.
+    assert(SystemDictionary::is_shared_class_visible_impl(class_name, ik, pkg_entry, class_loader, THREAD),
+           "visibility cannot change between dump time and runtime");
     return true;
   }
   return is_shared_class_visible_impl(class_name, ik, pkg_entry, class_loader, THREAD);
@@ -1275,70 +1288,45 @@ bool SystemDictionary::is_shared_class_visible_impl(Symbol* class_name,
                                                     InstanceKlass* ik,
                                                     PackageEntry* pkg_entry,
                                                     Handle class_loader, TRAPS) {
-  int path_index = ik->shared_classpath_index();
+  int scp_index = ik->shared_classpath_index();
-  ClassLoaderData* loader_data = class_loader_data(class_loader);
+  assert(!ik->is_shared_unregistered_class(), "this function should be called for built-in classes only");
-  SharedClassPathEntry* ent =
+  assert(scp_index >= 0, "must be");
-            (SharedClassPathEntry*)FileMapInfo::shared_path(path_index);
+  SharedClassPathEntry* scp_entry = FileMapInfo::shared_path(scp_index);
   if (!Universe::is_module_initialized()) {
-    assert(ent != NULL && ent->is_modules_image(),
+    assert(scp_entry != NULL && scp_entry->is_modules_image(),
            "Loading non-bootstrap classes before the module system is initialized");
     assert(class_loader.is_null(), "sanity");
     return true;
   }
-  // Get the pkg_entry from the classloader
-  ModuleEntry* mod_entry = NULL;
-  TempNewSymbol pkg_name = pkg_entry != NULL ? pkg_entry->name() :
-                                               ClassLoader::package_from_class_name(class_name);
-  if (pkg_name != NULL) {
-    if (loader_data != NULL) {
-      if (pkg_entry != NULL) {
-        mod_entry = pkg_entry->module();
-        // If the archived class is from a module that has been patched at runtime,
-        // the class cannot be loaded from the archive.
-        if (mod_entry != NULL && mod_entry->is_patched()) {
-          return false;
-        }
-      }
-    }
-  }
 
-  if (class_loader.is_null()) {
+  ModuleEntry* mod_entry = (pkg_entry == NULL) ? NULL : pkg_entry->module();
-    assert(ent != NULL, "Shared class for NULL classloader must have valid SharedClassPathEntry");
+  bool should_be_in_named_module = (mod_entry != NULL && mod_entry->is_named());
-    // The NULL classloader can load archived class originated from the
+  bool was_archived_from_named_module = scp_entry->in_named_module();
-    // "modules" jimage and the -Xbootclasspath/a. For class from the
+  bool visible;
-    // "modules" jimage, the PackageEntry/ModuleEntry must be defined
-    // by the NULL classloader.
-    if (mod_entry != NULL) {
-      // PackageEntry/ModuleEntry is found in the classloader. Check if the
-      // ModuleEntry's location agrees with the archived class' origination.
-      if (ent->is_modules_image() && mod_entry->location()->starts_with("jrt:")) {
-        return true; // Module class from the "module" jimage
-      }
-    }
 
-    // If the archived class is not from the "module" jimage, the class can be
+  if (was_archived_from_named_module) {
-    // loaded by the NULL classloader if
+    if (should_be_in_named_module) {
-    //
+      // Is the module loaded from the same location as during dump time?
-    // 1. the class is from the unamed package
+      visible = mod_entry->shared_path_index() == scp_index;
-    // 2. or, the class is not from a module defined in the NULL classloader
+      if (visible) {
-    // 3. or, the class is from an unamed module
+        assert(!mod_entry->is_patched(), "cannot load archived classes for patched module");
-    if (!ent->is_modules_image() && ik->is_shared_boot_class()) {
-      // the class is from the -Xbootclasspath/a
-      if (pkg_name == NULL ||
-          pkg_entry == NULL ||
-          pkg_entry->in_unnamed_module()) {
-        assert(mod_entry == NULL ||
-               mod_entry == loader_data->unnamed_module(),
-               "the unnamed module is not defined in the classloader");
-        return true;
       }
-    }
-    return false;
     } else {
-    bool res = SystemDictionaryShared::is_shared_class_visible_for_classloader(
+      // During dump time, this class was in a named module, but at run time, this class should be
-              ik, class_loader, pkg_name, pkg_entry, mod_entry, CHECK_(false));
+      // in an unnamed module.
-    return res;
+      visible = false;
     }
+  } else {
+    if (should_be_in_named_module) {
+      // During dump time, this class was in an unnamed, but at run time, this class should be
+      // in a named module.
+      visible = false;
+    } else {
+      visible = true;
+    }
+  }
+
+  return visible;
 }
 
 bool SystemDictionary::check_shared_class_super_type(InstanceKlass* child, InstanceKlass* super_type,

src/hotspot/share/classfile/systemDictionaryShared.cpp

@@ -886,114 +886,6 @@ bool SystemDictionaryShared::is_sharing_possible(ClassLoaderData* loader_data) {
           SystemDictionary::is_platform_class_loader(class_loader));
 }
 
-// Currently AppCDS only archives classes from the run-time image, the
-// -Xbootclasspath/a path, the class path, and the module path.
-//
-// Check if a shared class can be loaded by the specific classloader. Following
-// are the "visible" archived classes for different classloaders.
-//
-// NULL classloader:
-//   - see SystemDictionary::is_shared_class_visible()
-// Platform classloader:
-//   - Module class from runtime image. ModuleEntry must be defined in the
-//     classloader.
-// App classloader:
-//   - Module Class from runtime image and module path. ModuleEntry must be defined in the
-//     classloader.
-//   - Class from -cp. The class must have no PackageEntry defined in any of the
-//     boot/platform/app classloader, or must be in the unnamed module defined in the
-//     AppClassLoader.
-bool SystemDictionaryShared::is_shared_class_visible_for_classloader(
-                                                     InstanceKlass* ik,
-                                                     Handle class_loader,
-                                                     Symbol* pkg_name,
-                                                     PackageEntry* pkg_entry,
-                                                     ModuleEntry* mod_entry,
-                                                     TRAPS) {
-  assert(class_loader.not_null(), "Class loader should not be NULL");
-  assert(Universe::is_module_initialized(), "Module system is not initialized");
-  ResourceMark rm(THREAD);
-
-  int path_index = ik->shared_classpath_index();
-  SharedClassPathEntry* ent =
-            (SharedClassPathEntry*)FileMapInfo::shared_path(path_index);
-
-  if (SystemDictionary::is_platform_class_loader(class_loader())) {
-    assert(ent != NULL, "shared class for PlatformClassLoader should have valid SharedClassPathEntry");
-    // The PlatformClassLoader can only load archived class originated from the
-    // run-time image. The class' PackageEntry/ModuleEntry must be
-    // defined by the PlatformClassLoader.
-    if (mod_entry != NULL) {
-      // PackageEntry/ModuleEntry is found in the classloader. Check if the
-      // ModuleEntry's location agrees with the archived class' origination.
-      if (ent->is_modules_image() && mod_entry->location()->starts_with("jrt:")) {
-        return true; // Module class from the runtime image
-      }
-    }
-  } else if (SystemDictionary::is_system_class_loader(class_loader())) {
-    assert(ent != NULL, "shared class for system loader should have valid SharedClassPathEntry");
-    if (pkg_name == NULL) {
-      // The archived class is in the unnamed package. Currently, the boot image
-      // does not contain any class in the unnamed package.
-      assert(!ent->is_modules_image(), "Class in the unnamed package must be from the classpath");
-      if (path_index >= ClassLoaderExt::app_class_paths_start_index()) {
-        assert(path_index < ClassLoaderExt::app_module_paths_start_index(), "invalid path_index");
-        return true;
-      }
-    } else {
-      // Check if this is from a PackageEntry/ModuleEntry defined in the AppClassloader.
-      if (pkg_entry == NULL) {
-        // It's not guaranteed that the class is from the classpath if the
-        // PackageEntry cannot be found from the AppClassloader. Need to check
-        // the boot and platform classloader as well.
-        ClassLoaderData* platform_loader_data =
-          ClassLoaderData::class_loader_data_or_null(SystemDictionary::java_platform_loader()); // can be NULL during bootstrap
-        if ((platform_loader_data == NULL ||
-             ClassLoader::get_package_entry(pkg_name, platform_loader_data) == NULL) &&
-             ClassLoader::get_package_entry(pkg_name, ClassLoaderData::the_null_class_loader_data()) == NULL) {
-          // The PackageEntry is not defined in any of the boot/platform/app classloaders.
-          // The archived class must from -cp path and not from the runtime image.
-          if (!ent->is_modules_image() && path_index >= ClassLoaderExt::app_class_paths_start_index() &&
-                                          path_index < ClassLoaderExt::app_module_paths_start_index()) {
-            return true;
-          }
-        }
-      } else if (mod_entry != NULL) {
-        // The package/module is defined in the AppClassLoader. We support
-        // archiving application module class from the runtime image or from
-        // a named module from a module path.
-        // Packages from the -cp path are in the unnamed_module.
-        if (ent->is_modules_image() && mod_entry->location()->starts_with("jrt:")) {
-          // shared module class from runtime image
-          return true;
-        } else if (pkg_entry->in_unnamed_module() && path_index >= ClassLoaderExt::app_class_paths_start_index() &&
-            path_index < ClassLoaderExt::app_module_paths_start_index()) {
-          // shared class from -cp
-          DEBUG_ONLY( \
-            ClassLoaderData* loader_data = class_loader_data(class_loader); \
-            assert(mod_entry == loader_data->unnamed_module(), "the unnamed module is not defined in the classloader");)
-          return true;
-        } else {
-          if(!pkg_entry->in_unnamed_module() &&
-              (path_index >= ClassLoaderExt::app_module_paths_start_index())&&
-              (path_index < FileMapInfo::get_number_of_shared_paths()) &&
-              (strcmp(ent->name(), ClassLoader::skip_uri_protocol(mod_entry->location()->as_C_string())) == 0)) {
-            // shared module class from module path
-            return true;
-          } else {
-            assert(path_index < FileMapInfo::get_number_of_shared_paths(), "invalid path_index");
-          }
-        }
-      }
-    }
-  } else {
-    // TEMP: if a shared class can be found by a custom loader, consider it visible now.
-    // FIXME: is this actually correct?
-    return true;
-  }
-  return false;
-}
-
 bool SystemDictionaryShared::has_platform_or_app_classes() {
   if (FileMapInfo::current_info()->has_platform_or_app_classes()) {
     return true;
@@ -1026,7 +918,7 @@ bool SystemDictionaryShared::has_platform_or_app_classes() {
 // [b] BuiltinClassLoader.loadClassOrNull() first calls findLoadedClass(name).
 // [c] At this point, if we can find the named class inside the
 //     shared_dictionary, we can perform further checks (see
-//     is_shared_class_visible_for_classloader() to ensure that this class
+//     SystemDictionary::is_shared_class_visible) to ensure that this class
 //     was loaded by the same class loader during dump time.
 //
 // Given these assumptions, we intercept the findLoadedClass() call to invoke

src/hotspot/share/classfile/systemDictionaryShared.hpp

@@ -242,12 +242,6 @@ public:
 
   // Check if sharing is supported for the class loader.
   static bool is_sharing_possible(ClassLoaderData* loader_data);
-  static bool is_shared_class_visible_for_classloader(InstanceKlass* ik,
-                                                      Handle class_loader,
-                                                      Symbol* pkg_name,
-                                                      PackageEntry* pkg_entry,
-                                                      ModuleEntry* mod_entry,
-                                                      TRAPS);
 
   static bool add_unregistered_class(InstanceKlass* k, TRAPS);
   static InstanceKlass* dump_time_resolve_super_or_fail(Symbol* child_name,

src/hotspot/share/memory/filemap.cpp

@@ -252,6 +252,7 @@ void SharedClassPathEntry::init_as_non_existent(const char* path, TRAPS) {
 }
 
 void SharedClassPathEntry::init(bool is_modules_image,
+                                bool is_module_path,
                                 ClassPathEntry* cpe, TRAPS) {
   Arguments::assert_is_dumping_archive();
   _timestamp = 0;
@@ -272,6 +273,7 @@ void SharedClassPathEntry::init(bool is_modules_image,
         _from_class_path_attr = cpe->from_class_path_attr();
       }
       _filesize = st.st_size;
+      _is_module_path = is_module_path;
     }
   } else {
     // The file/dir must exist, or it would not have been added
@@ -296,6 +298,7 @@ void SharedClassPathEntry::set_name(const char* name, TRAPS) {
 
 void SharedClassPathEntry::copy_from(SharedClassPathEntry* ent, ClassLoaderData* loader_data, TRAPS) {
   _type = ent->_type;
+  _is_module_path = ent->_is_module_path;
   _timestamp = ent->_timestamp;
   _filesize = ent->_filesize;
   _from_class_path_attr = ent->_from_class_path_attr;
@@ -450,10 +453,11 @@ void FileMapInfo::allocate_shared_path_table() {
 int FileMapInfo::add_shared_classpaths(int i, const char* which, ClassPathEntry *cpe, TRAPS) {
   while (cpe != NULL) {
     bool is_jrt = (cpe == ClassLoader::get_jrt_entry());
+    bool is_module_path = i >= ClassLoaderExt::app_module_paths_start_index();
     const char* type = (is_jrt ? "jrt" : (cpe->is_jar_file() ? "jar" : "dir"));
     log_info(class, path)("add %s shared path (%s) %s", which, type, cpe->name());
     SharedClassPathEntry* ent = shared_path(i);
-    ent->init(is_jrt, cpe, THREAD);
+    ent->init(is_jrt, is_module_path, cpe, THREAD);
     if (cpe->is_jar_file()) {
       update_jar_manifest(cpe, ent, THREAD);
     }
@@ -513,6 +517,38 @@ int FileMapInfo::num_non_existent_class_paths() {
   }
 }
 
+int FileMapInfo::get_module_shared_path_index(Symbol* location) {
+  if (location->starts_with("jrt:", 4) && get_number_of_shared_paths() > 0) {
+    assert(shared_path(0)->is_modules_image(), "first shared_path must be the modules image");
+    return 0;
+  }
+
+  if (ClassLoaderExt::app_module_paths_start_index() >= get_number_of_shared_paths()) {
+    // The archive(s) were created without --module-path option
+    return -1;
+  }
+
+  if (!location->starts_with("file:", 5)) {
+    return -1;
+  }
+
+  // skip_uri_protocol was also called during dump time -- see ClassLoaderExt::process_module_table()
+  ResourceMark rm;
+  const char* file = ClassLoader::skip_uri_protocol(location->as_C_string());
+  for (int i = ClassLoaderExt::app_module_paths_start_index(); i < get_number_of_shared_paths(); i++) {
+    SharedClassPathEntry* ent = shared_path(i);
+    assert(ent->in_named_module(), "must be");
+    bool cond = strcmp(file, ent->name()) == 0;
+    log_debug(class, path)("get_module_shared_path_index (%d) %s : %s = %s", i,
+                           location->as_C_string(), ent->name(), cond ? "same" : "different");
+    if (cond) {
+      return i;
+    }
+  }
+
+  return -1;
+}
+
 class ManifestStream: public ResourceObj {
   private:
   u1*   _buffer_start; // Buffer bottom

src/hotspot/share/memory/filemap.hpp

@@ -58,6 +58,7 @@ class SharedClassPathEntry {
   void set_name(const char* name, TRAPS);
 
   u1     _type;
+  bool   _is_module_path;
   bool   _from_class_path_attr;
   time_t _timestamp;          // jar timestamp,  0 if is directory, modules image or other
   long   _filesize;           // jar/jimage file size, -1 if is directory, -2 if other
@@ -65,7 +66,7 @@ class SharedClassPathEntry {
   Array<u1>*   _manifest;
 
 public:
-  void init(bool is_modules_image, ClassPathEntry* cpe, TRAPS);
+  void init(bool is_modules_image, bool is_module_path, ClassPathEntry* cpe, TRAPS);
   void init_as_non_existent(const char* path, TRAPS);
   void metaspace_pointers_do(MetaspaceClosure* it);
   bool validate(bool is_class_path = true) const;
@@ -96,6 +97,10 @@ public:
   }
   bool check_non_existent() const;
   void copy_from(SharedClassPathEntry* ent, ClassLoaderData* loader_data, TRAPS);
+  bool in_named_module() {
+    return is_modules_image() || // modules image doesn't contain unnamed modules
+           _is_module_path;      // module path doesn't contain unnamed modules
+  }
 };
 
 struct ArchiveHeapOopmapInfo {
@@ -522,6 +527,8 @@ public:
     return _shared_path_table.size();
   }
 
+  static int get_module_shared_path_index(Symbol* location) NOT_CDS_RETURN_(-1);
+
   char* region_addr(int idx);
 
   // The offset of the first core region in the archive, relative to SharedBaseAddress

test/hotspot/jtreg/runtime/cds/appcds/jigsaw/PatchModule/Simple.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2020, 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
@@ -76,5 +76,20 @@ public class Simple {
             "-Xlog:class+path=info",
             "PatchMain", "javax.naming.spi.NamingManager")
             .assertSilentlyDisabledCDS(0, "I pass!");
+
+        // ========================================
+        System.out.println("Dump again without --patch-module");
+        output =
+            TestCommon.dump(null,
+                TestCommon.list("javax/naming/spi/NamingManager"));
+        output.shouldHaveExitValue(0);
+
+        TestCommon.run(
+            "-XX:+UnlockDiagnosticVMOptions",
+            "--patch-module=java.naming=" + moduleJar,
+            "-Xlog:class+load",
+            "-Xlog:class+path=info",
+            "PatchMain", "javax.naming.spi.NamingManager")
+            .assertSilentlyDisabledCDS(0, "I pass!");
     }
 }