RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v3]

[Ioi Lam]

On Fri, 4 Apr 2025 18:13:24 GMT, Timofei Pushkin <tpushkin at openjdk.org> wrote:

>> src/java.base/share/classes/jdk/internal/misc/CDS.java line 446:
>> 
>>> 444:         protected Class<?> findClass(String name) throws ClassNotFoundException {
>>> 445:             // Unregistered classes should be found in load(...), registered classes should be
>>> 446:             // handeled by parent loaders
>> 
>> This does seem like a good simplification, as we no longer need to check the `currentClassName`, `currentSuperClass`, etc. 
>> 
>> We had a plan for supported multiple classes for the same name, but we never go around doing it (even though the classlist format supports that with the use of IDs).
>> 
>> I am not sure if the current tests includes a case where a class name `Foo` exists both in the classpath as well as in a custom loader. If not, I think we should add such a case (to ensure that UnregisteredClassLoader never delegates to the application class loader and finds the wrong `Foo`).
>
> Thank you for the review!
> 
>> We had a plan for supported multiple classes for the same name, but we never go around doing it (even though the classlist format supports that with the use of IDs).
> 
> With this approach having multiple unregistered classes with the same name is not possible (JVM will not allow this for a single class loader) so if you are still planning to support this in the near future it would be better to go with the approach originally suggested in the issue: continue using multiple unregistered class loaders but record class loader ID in class list and create only one archiving class loader per recorded ID. But that would be a larger change since it requires class list format to be changed.
> 
>> I am not sure if the current tests includes a case where a class name Foo exists both in the classpath as well as in a custom loader
> 
> Pre-existing `ClassFromSystemModule` test seems to test the boot classpath case. I've also added a new test for the app classpath case.

We don't have immediate plans for supporting multiple classes of the same name. I suspect any future enhancements would be available only through new "AOT" workflow, where the training information, including the unregistered classes, would be encoded in the binary AOT configuration file. Therefore, these future enhancements will not affect this particular case (loading unregistered classes from the classlist text file).

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2029445262