RFR: 8342089: Require --enable-native-access to be the same between CDS dump time and run time [v4]

[Ioi Lam]

On Fri, 22 Nov 2024 20:45:30 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> Applications that use JNI or FFM need to use the `--enable-native-access` flag, or include the Enable-Native-Access attribute in their JAR files. Currently, the CDS archive cannot use optimized module handling when `--enable-native-access` is specified, so such applications do not support CDS. 
>> 
>> This patch no longer disables optimized module graph so long as the `--enable-native-access` is consistent between dump time and runtime. The modules list provided by the option is stored in the RO region of the CDS archive. Verified with tier 1-5 tests and a new regression test.
>
> Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Calvin comments

Changes requested by iklam (Reviewer).

src/hotspot/share/cds/cdsConfig.cpp line 256:

> 254:       !Arguments::is_module_path_property(key) &&
> 255:       !Arguments::is_add_modules_property(key) &&
> 256:       !Arguments::is_enable_native_access_property(key)) {

It's rather cumbersome to call `Arguments::is_internal_module_property()` first and then filter the results. When we add more support for full module graph, the list of `!Arguments::is_xxx()` will likely increase. I think it's better to refactor the Arguments class like this:


// Returns true if property is one of those recognized by is_internal_module_property() but
// is not supported by CDS archived full module graph.
bool Arguments::is_non_cds_compatible_internal_module_property(const char* property) {
  return internal_module_property_helper(property, true);
}

bool Arguments::is_internal_module_property(const char* property) {
  return internal_module_property_helper(property, false);
}

bool Arguments::internal_module_property_helper(const char* property, bool check_for_cds) {
  if (strncmp(property, MODULE_PROPERTY_PREFIX, MODULE_PROPERTY_PREFIX_LEN) == 0) {
    const char* property_suffix = property + MODULE_PROPERTY_PREFIX_LEN;
    if (matches_property_suffix(property_suffix, ADDEXPORTS, ADDEXPORTS_LEN) ||
        matches_property_suffix(property_suffix, ADDREADS, ADDREADS_LEN) ||
        matches_property_suffix(property_suffix, ADDOPENS, ADDOPENS_LEN) ||
        matches_property_suffix(property_suffix, PATCH, PATCH_LEN) ||
        matches_property_suffix(property_suffix, LIMITMODS, LIMITMODS_LEN) ||
        matches_property_suffix(property_suffix, UPGRADE_PATH, UPGRADE_PATH_LEN) ||
        matches_property_suffix(property_suffix, ILLEGAL_NATIVE_ACCESS, ILLEGAL_NATIVE_ACCESS_LEN)) {
      // CDS notes: if any of these properties are specified, CDS archived full module graph. is disabled.
      return true;
    }

    if (!check_for_cds) {
      // CDS notes: these properties are supported by CDS archived full module graph.
      if (matches_property_suffix(property_suffix, PATH, PATH_LEN) ||
          matches_property_suffix(property_suffix, ADDMODS, ADDMODS_LEN) ||
          matches_property_suffix(property_suffix, ENABLE_NATIVE_ACCESS, ENABLE_NATIVE_ACCESS_LEN)) {
        return true;
      }
    }
  }

  return false;
}

src/hotspot/share/cds/metaspaceShared.cpp line 428:

> 426:   CDS_JAVA_HEAP_ONLY(Modules::serialize(soc);)
> 427:   CDS_JAVA_HEAP_ONLY(Modules::serialize_addmods_names(soc);)
> 428:   CDS_JAVA_HEAP_ONLY(Modules::serialize_native_access_flags(soc);)

These three `Modules::xxx` functions can be combined into a single `Modules::serialize_archived_module_info()` function.

src/hotspot/share/cds/metaspaceShared.cpp line 576:

> 574:   CDS_JAVA_HEAP_ONLY(Modules::dump_addmods_names();)
> 575:   // Write native enable-native-access flag into archive
> 576:   CDS_JAVA_HEAP_ONLY(Modules::dump_native_access_flag());

These three functions can be combined into a single ``Modules::dump_archived_module_info()` API.

(If that single function becomes too large, it can be divided into several smaller private functions. But that detail shouldn't be exposed out of the `Modules` class).

src/hotspot/share/classfile/modules.cpp line 601:

> 599: 
> 600:   // Don't hold onto the pointer, in case we might decide to unmap the archive.
> 601:   *archived_flag = nullptr;

A `check_xxx` function is usually not expected to modify one of its incoming parameters. Also, `char**` variables are messy. It's better to do this in the caller:



check_archived_flag_consistency(_archived_main_module_name, runtime_main_module, "jdk.module.main");
check_archived_flag_consistency(_archived_native_access_flags, get_native_access_flags_as_sorted_string(), "jdk.module.enable.native.access");
...

 // Don't hold onto the pointers, in case we might decide to unmap the archive.
_archived_main_module_name = nullptr;
_archived_native_access_flags = nullptr;

src/hotspot/share/classfile/modules.cpp line 616:

> 614: 
> 615: void Modules::dump_native_access_flag() {
> 616:   unsigned int count = Arguments::enable_native_access_count();

`count` is unused.

src/hotspot/share/classfile/modules.cpp line 660:

> 658:   char* prop_name = resource_allocate_bytes(prop_len);
> 659:   GrowableArray<const char*> list;
> 660:   for (unsigned int i = 0; i < property_count; i++) {

There's no need for property_count. You can just keep reading until Arguments::get_property() returns null. That way, there's no need to expose `Arguments::addmods_count()`, etc.


for (int i = 0; ; i++) {
  jio_snprintf(prop_name, prop_len, "%s.%d", property, i);
  const char* prop_value = Arguments::get_property(prop_name);
  if (prop_value == nullptr) {
    break;
  }
  ...
}


Also, for existing code: it's better to use a few extra bytes so you never need to worry about buffer overflow.


// theoretical string size limit for decimal int, but the following loop will end much sooner due to
// OS command-line size limit.
const int max_digits = 10;

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

PR Review: https://git.openjdk.org/jdk/pull/22305#pullrequestreview-2457148975
PR Review Comment: https://git.openjdk.org/jdk/pull/22305#discussion_r1855796657
PR Review Comment: https://git.openjdk.org/jdk/pull/22305#discussion_r1855797037
PR Review Comment: https://git.openjdk.org/jdk/pull/22305#discussion_r1855798813
PR Review Comment: https://git.openjdk.org/jdk/pull/22305#discussion_r1855817626
PR Review Comment: https://git.openjdk.org/jdk/pull/22305#discussion_r1855802066
PR Review Comment: https://git.openjdk.org/jdk/pull/22305#discussion_r1855809705