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

[Calvin Cheung]

On Thu, 13 Feb 2025 03:55:50 GMT, Ashutosh Mehra <asmehra at 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 762:
> 
>> 760:   }
>> 761: 
>> 762:   if (is_boot_classpath && runtime_css.has_next() && (need_to_check_app_classpath() || num_module_paths() > 0)) {
> 
> I am not sure I get what this block is for. Is it for the case where runtime boot cp has more entries than the dumptime boot cp, and it is checking if the extra entries really exist or they are just empty? If so, then `check_paths_existence` should only be checking the extra entries in the boot cp, not all of them.
> 
> Can you please explain this and probably add a comment as well to describe what this block is for.

I added some comment:

  // Check if the runtime boot classpath has more entries than the one stored in the archive and if the app classpath
  // or the module path requires validation.
  if (is_boot_classpath && runtime_css.has_next() && (need_to_check_app_classpath() || num_module_paths() > 0)) {
    // the check passes if all the extra runtime boot classpath entries are non-existent
    if (check_paths_existence(runtime_css)) {
      log_warning(cds)("boot classpath is longer than expected");
      return false;
    }
  }

Also fixed the `check_paths_existence()` method so it only checks the extra entries.

> src/hotspot/share/cds/aotCodeSource.cpp line 894:
> 
>> 892: // matched exactly.
>> 893: bool AOTCodeSourceConfig::need_lcp_match(AllCodeSourceStreams& all_css) const {
>> 894:   if (!need_lcp_match_helper(boot_start(), boot_end(), all_css.boot_cp()) ||
> 
> Can we reverse these conditions to make it easier to read?
> 
> 
>   if (need_lcp_match_helper(boot_start(), boot_end(), all_css.boot_cp()) &&
>       need_lcp_match_helper(app_start(), app_end(), all_css.app_cp())) {
>     return true;
>   } else {
>     return false;
>   }

Done.

> src/hotspot/share/cds/aotCodeSource.cpp line 903:
> 
>> 901: 
>> 902: bool AOTCodeSourceConfig::need_lcp_match_helper(int start, int end, CodeSourceStream& css) const {
>> 903:   if (app_end() == boot_start()) {
> 
> I feel this block belongs to the caller `need_lcp_match`.

Fixed.

> src/hotspot/share/cds/aotCodeSource.hpp line 213:
> 
>> 211: 
>> 212:   // Common accessors
>> 213:   int boot_start()                       const { return 1; }
> 
> Can we rename these methods to something like boot_start() -> boot_cp_start_index().
> At the call site it makes it clear it is referring to the bootclasspath index, and not booting something :)

I renamed them as follows:

  // Common accessors
  int boot_cp_start_index()          const { return 1; }
  int boot_cp_end_index()            const { return _boot_classpath_end; }
  int app_cp_start_index()           const { return boot_cp_end_index(); }
  int app_cp_end_index()             const { return _app_classpath_end; }
  int module_path_start_index()      const { return app_cp_end_index(); }
  int module_path_end_index()        const { return _module_end; }

> src/hotspot/share/cds/aotCodeSource.hpp line 234:
> 
>> 232:   // Functions used only during dumptime
>> 233:   static void dumptime_init(TRAPS);
>> 234:   static size_t estimate_size_for_archive() {
> 
> This method doesn't seem to be in use. Can this be removed?

Removed. I also removed the `estimate_size_for_archive_helper()` method.

> src/hotspot/share/cds/filemap.cpp line 318:
> 
>> 316:   if (header()->has_full_module_graph() && has_extra_module_paths) {
>> 317:     CDSConfig::stop_using_optimized_module_handling();
>> 318:     log_info(cds)("optimized module handling: disabled because of extra module path(s) are specified");
> 
> typo: "disabled because ~of~ extra module path(s) are specified"

Fixed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1956614255
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1956614149
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1956614064
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1956613894
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1956613789
PR Review Comment: https://git.openjdk.org/jdk/pull/23476#discussion_r1956614357