On Tue, 15 Apr 2025 00:58:19 GMT, Ioi Lam <ik...@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
