On Tue, 8 Apr 2025 12:09:59 GMT, Timofei Pushkin <tpush...@openjdk.org> wrote:
>> If a base class is package-private then its subclasses should have the same >> package name and defining class loader, otherwise `IllegalAccessError` is >> thrown when linking a subclass. Currently when dumping a static archive >> separate `URLClassLoader`s are used for each unregistered classes' source. >> Thus if two unregistered classes, a package-private base class and a sub >> class, from the same package reside in different sources >> `IllegalAccessError` will be thrown when linking the sub class. This can be >> unexpected because the app could have used a single class loader for both >> classes and thus not have seen the error — see `DifferentSourcesApp.java` >> from this patch for an example of such app. >> >> This patch fixes the issue by using a single class loader for all >> unregistered classes. CDS does not allow classes with the same name making >> such solution possible. > > Timofei Pushkin has updated the pull request incrementally with one > additional commit since the last revision: > > Remove unnecessary scoping Changes requested by iklam (Reviewer). src/java.base/share/classes/jdk/internal/misc/CDS.java line 438: > 436: // class loader. Thus it is safe to delegate their loading > to system class loader > 437: // (our parent) - this is what the default implementation of > loadClass() will do. > 438: return defineClass(name, bytes, 0, bytes.length); I didn't realize that `URLClassLoader` will by default delegate to `ClassLoader::getSystemClassLoader()`. How about rewording the comments like this to clarify? // defineClass() will internally invoke loadClass() to load supertypes of this unregistered class. // Any supertype S with the name SN must have already been loaded (enforced by the order // of classes in the classlist). In addition: // - if S is an unregistered class, S must have already been have been defined by the current class // loader, and will be found by `this.findLoadedClass(SN)` // - if S is not an unregistered class, S must have already been defined by the built-in boot, // platform, or system class loaders, and can be found by this.getParent().loadClass(SN, false) // See the implementation of ClassLoader::loadClass() for details. // // Therefore, we should resolve all supertypes to the expected ones as specified by the // <code>super:</code> and <code>interfaces:</code> attributes in the classlist. This // invariance is validated by the C++ function ClassListParser::load_class_from_source() assert getParent() == getSystemClassLoader(); return defineClass(name, bytes, 0, bytes.length); ------------- PR Review: https://git.openjdk.org/jdk/pull/24223#pullrequestreview-2750817568 PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2033724629
