On Tue, 18 Feb 2025 07:08:07 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Calvin Cheung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @iklam and @ashu-mehra comment
>
> 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.
Since most of the code and logic are from filemap.[c|h]pp, I've updated the
copyright year to 2003, 2025.
> 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
Fixed.
> 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?
Actually, we don't require the non-existent entries to be on the classpath at
runtime. The appcds/NonExistClasspath.java test has cases to cover that.
So I've updated the comment as follows:
// Non-existent entries are recorded during AOTCache creation. Those
non-existent entries,
// if they are specified at runtime, must not exist.
Also fixed a bug so that the behavior is the same as before this refactoring.
> 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
Fixed.
> 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;
Fixed.
> 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.
Fixed.
> 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?
This is a missing testcase and works the same before and after refactoring.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1962467161
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1962466958
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1962467710
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1962467840
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1962467951
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1962468046
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1962468281
