On Sat, 12 Apr 2025 18:37:18 GMT, Ioi Lam <[email protected]> wrote:
>> During an application's training run, it's possible to inject classes into
>> the built-in platform/app class loaders with reflection calls.
>>
>> - Before [JDK-8348426](https://bugs.openjdk.org/browse/JDK-8348426), only
>> the names of these classes were recorded in the AOT config file. When the
>> AOT cache is generated, these classes are automatically filtered out.
>> - Since [JDK-8348426](https://bugs.openjdk.org/browse/JDK-8348426), these
>> classes are stored as parsed InstanceKlasses in the AOT config file, and
>> will be transferred into the AOT cache. This new behavior may cause some
>> applications to fail, as they may inject bytecodes that have environment
>> dependencies.
>>
>> For safety, this PR filters out such injected classes from the AOT config
>> file.
>
> Ioi Lam has updated the pull request with a new target base due to a merge or
> a rebase. The incremental webrev excludes the unrelated changes brought in by
> the merge/rebase. The pull request contains two additional commits since the
> last revision:
>
> - Merge branch 'master' into
> 8352001-exclude-injected-classes-from-builtin-loaders
> - 8352001: AOT cache should not contain classes injected into built-in class
> loaders
Looks good overall. I have a question in classLoaderExt.cpp.
src/hotspot/share/classfile/classLoaderExt.cpp line 105:
> 103:
> 104: if (CDSConfig::is_dumping_preimage_static_archive() ||
> CDSConfig::is_dumping_dynamic_archive()) {
> 105:
> AOTClassLocationConfig::dumptime()->check_invalid_classpath_index(classpath_index,
> result);
In case the `classpath_index` is invalid, I don't think we should call
`AOTClassLocationConfig::dumptime_update_max_used_index()`. Maybe the
`check_invalid_classpath_index()` function should return a bool and have
`ClassLoaderExt::record_result()` update the classpath index and max used index.
-------------
PR Review: https://git.openjdk.org/jdk/pull/24046#pullrequestreview-2769994967
PR Review Comment: https://git.openjdk.org/jdk/pull/24046#discussion_r2045596814
