RFR: 8280682: Refactor AOT code source validation checks [v2]

[David Holmes]

On Fri, 14 Feb 2025 19:21:32 GMT, Calvin Cheung <ccheung at openjdk.org> wrote:

>> This changset refactors CDS class paths and module paths validation code into a new class `AOTCodeSource` and related class `AOTCodeSourceConfig`. Code has been moved from filemap.[c|h]pp, classLoader.[c|h]pp, and classLoaderExt.[c|h]pp to aotCodeSource.[c|h]pp. CDS dependencies have been removed from `classLoader.cpp`. More refactoring could be done, such as removing `classLoaderExt.cpp`, in a future RFE.
>> 
>> Passed tiers 1 - 5 testing.
>
> Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision:
> 
>   @iklam and @ashu-mehra comment

This is a very large change to try and digest. Is it really just a refactor? I've made a few comments in places where it seems functionality may have been changed as well.

src/hotspot/share/cds/aotCodeSource.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.

If the code has been moved from other files, the current opinion/consensus is that the first copyright year should be the oldest first year from all the files from which the code was obtained.

src/hotspot/share/cds/aotCodeSource.cpp line 106:

> 104:     for (const char* bootcp = Arguments::get_boot_class_path(); *bootcp != '\0'; ++bootcp) {
> 105:       if (*bootcp == *os::path_separator()) {
> 106:         ++ bootcp;

Nit (possibly pre-existing) - no space before/after unary operators

src/hotspot/share/cds/aotCodeSource.hpp line 125:

> 123: // during AOTCache creation are the same as when the AOTCache is used during runtime.
> 124: // Non-existent entries are recorded during AOTCache creation. Those non-existent entries
> 125: // must not exist during runtime.

Does this mean that if Foo.jar is on the classpath but does not in fact exist, then we record it was on the classpath and require it to be on the classpath at runtime, but also to still not exist?

src/hotspot/share/cds/aotCodeSource.hpp line 128:

> 126: //
> 127: // Some details on validation:
> 128: // - the boot classpath could be appended during runtime if there's no app classpath and

Suggestion:

// - the boot classpath can be appended to at runtime if there's no app classpath and no

src/hotspot/share/cds/aotCodeSource.hpp line 130:

> 128: // - the boot classpath could be appended during runtime if there's no app classpath and
> 129: //   module path specified when an AOTCache is created;
> 130: // - the app classpath could be appended during runtime;

Suggestion:

// - the app classpath can be appended to at runtime;

src/hotspot/share/cds/aotCodeSource.hpp line 131:

> 129: //   module path specified when an AOTCache is created;
> 130: // - the app classpath could be appended during runtime;
> 131: // - the module path during runtime could be a superset of the one specified during AOTCache creation.

Suggestion:

// - the module path at runtime can be a superset of the one specified during AOTCache creation.

src/hotspot/share/runtime/threads.cpp line 809:

> 807:     vm_exit_during_initialization("ClassLoader::initialize_module_path() failed unexpectedly");
> 808:   }
> 809: #endif

Not obvious where this functionality is now handled.

test/hotspot/jtreg/runtime/cds/appcds/BootClassPathMismatch.java line 243:

> 241:      * No error - bootclasspath can be appended during runtime if no -cp is specified.
> 242:      */
> 243:     public void testBootClassPathAppend() throws Exception {

A refactoring should not be introducing new test cases. Did you refactor and enhance?

test/hotspot/jtreg/runtime/cds/appcds/NonExistClasspath.java line 70:

> 68:             .assertNormalExit();
> 69: 
> 70:         // Replace nonExistPath with another non-existent file in the CP, it should still work

Not at all clearr why these test cases have been removed?

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

PR Review: https://git.openjdk.org/jdk/pull/23476#pullrequestreview-2622538668
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959178211
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959126336
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959181514
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959181842
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959183865
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959184959
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959192451
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959193331
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1959194900