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

[Ioi Lam]

On Mon, 14 Apr 2025 15:23:07 GMT, Timofei Pushkin <tpushkin at openjdk.org> wrote:

>> test/hotspot/jtreg/runtime/cds/appcds/customLoader/ClassFromClasspath.java line 53:
>> 
>>> 51:         out.shouldContain("unreg CustomLoadee");
>>> 52:     }
>>> 53: }
>> 
>> For completeness, I think we should have a more complicated scenario:
>> 
>> - load CustomLoadee in both the app loader and a custom loader
>> - load CustomLoadeeChild in the custom loader. Its super class should be the one defined in the custom loader
>> 
>> At run time, verify that CustomLoadeeChild is archived and its super class is defined in the custom loader
>
> This case will work, I've added the test, however I found another case which won't be handled correctly, neither before nor after this change.
> 
> This is the case you've suggested, it works fine both before and after the change.
> 
> java/lang/Object id: 0
> CustomLoadee3 id: 1
> CustomLoadee3 id: 2 super: 0 source: a.jar
> CustomLoadee3Child id: 3 super: 2 source: a.jar
> 
> 
> However the below one doesn't work neither before nor after this change. At dump time, 2 is loaded instead of 1 as the super of 3 because 2, 3 are loaded by the same class loader and 1, 2 have the same name. It is possible to get such class list if 2 is loaded by a non-delegating unregistered loader while 3 is loaded by a different delegating one.
> 
> # Difference with the previous: super of the last class
> java/lang/Object id: 0
> CustomLoadee3 id: 1
> CustomLoadee3 id: 2 super: 0 source: a.jar
> CustomLoadee3Child id: 3 super: 1 source: a.jar
> 
> 
> And the following case is working without this change but will not work with it. It is working now because there will be two different unregistered loaders for classes 2 and 3 (because they have the same source), however with this change there will be one and it will re-use 2 already loaded by it when loading 3.
> 
> # Difference with the previous: source of the last class
> java/lang/Object id: 0
> CustomLoadee3 id: 1
> CustomLoadee3 id: 2 super: 0 source: a.jar
> CustomLoadee3Child id: 3 super: 1 source: b.jar
> 
> 
> Although the last case is a regression, I am not sure how much of a problem this is since in general having a registered super with the same name as another unregistered class is not supported anyway as shown by the second case.

Thanks for doing the investigation. I think we are always going to have some corner cases that cannot be covered:

- For your second case, we would need a separate ClassLoader per class.
- However, that will result in the IllegalAccessError in the current PR

The problem is we are trying to reconstruct the classes without reconstructing the ClassLoaders. A real solution would probably require updating the classlist format.

However, future CDS improvements will be based on the AOT cache introduced in JEP 483. As of JDK 25, we have changed the AOT cache configuration file from a text file (classlist) to a binary file (see https://github.com/openjdk/jdk/pull/23484) . Also, the unregistered classes are saved from the training run, where we still have ClassLoader information (see https://github.com/openjdk/jdk/pull/23926). Therefore, I think we should just leave the old classlist format as is, and accept that certain unregistered classes cannot be supported. I'll try add some test cases ([JDK-8354557](https://bugs.openjdk.org/browse/JDK-8354557)) for the AOT cache using the test cases you identified above.

This PR address the IllegalAccessError problem and simplifies the implementation of classlist handling. However, it causes incompatibility (in your third case). I think we should change the error checking code in `InstanceKlass* ClassListParser::load_class_from_source()` to print a warning message and continue. These are the rare cases that we cannot support. Since CDS is just a cache mechanism, it should be acceptable that certain classes are not supported. We can use your second and third test cases to validate the warning messages.

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

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