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

[Timofei Pushkin]

On Tue, 15 Apr 2025 00:58:19 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> 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.

I fully agree, will implement the warnings

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

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