8320002: Remove obsolete CDS check in Reflection::verify_class_access()

[calvinccheung]

This fix removes an obsolete CDS check. Please review.

Passed tiers 1 - 4 testing.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8320002: Remove obsolete CDS check in Reflection::verify_class_access() (Enhancement - P4)

Reviewers

• Ioi Lam (@iklam - Reviewer)
• Matias Saavedra Silva (@matias9927 - Committer)
• David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17232/head:pull/17232
$ git checkout pull/17232

Update a local copy of the PR:
$ git checkout pull/17232
$ git pull https://git.openjdk.org/jdk.git pull/17232/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17232

View PR using the GUI difftool:
$ git pr show -t 17232

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17232.diff

Webrev

Link to Webrev Comment

[calvinccheung]

/label add hotspot-runtime

[bridgekeeper]

👋 Welcome back ccheung! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@calvinccheung
The hotspot-runtime label was successfully added.

[mlbridge]

Webrevs

• 00: Full (4525ff82)

[dholmes-ora]

This would seem to introduce a change in behaviour. During dumping we may now get an access exception when previously access was assumed to be okay.

[calvinccheung]

This would seem to introduce a change in behaviour. During dumping we may now get an access exception when previously access was assumed to be okay.

During static dump, if an exception is encountered, it would just log a CDS warning message and continue processing the next entry in the classlist. For example, the code in classListParser.cpp.

[dholmes-ora]

During static dump, if an exception is encountered, it would just log a CDS warning message and continue processing the next entry in the classlist.

Exactly! It would skip the class that triggered the exception, which is a behaviour change to what happens now.

[iklam]

The code that Calvin is removing is very old code. I guess at one time during the initial development of modules in JDK 9, even when the access was perfectly OK, the "real" check would fail because some information were not properly set up. So as a hack, someone added:

if (DumpSharedSpaces) {
  return ACCESS_OK
}
if (some_real_check()) { // <-- would cause failures/assert/etc if DumpSharedSpaces==true
}

Unfortunately there's no history for why this hack was added:

https://github.com/openjdk/jdk/blame/6e86904a94d2ed2815aa6e3364c048dac595320d/src/hotspot/share/runtime/reflection.cpp#L458-L466

However, that problem doesn't exist anymore, so we can remove this hack.

Yes, this changes behavior. However, we run a very limited set of Java code during "java -Xshare:dump". Specifically, we never execute any user-supplied code. So even if we perform the real checks, we should always get ACCESS_OK anyway. If we don't, that will be a bug, and we better know about it.

[iklam]

LGTM.

[openjdk]

@calvinccheung This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8320002: Remove obsolete CDS check in Reflection::verify_class_access()

Reviewed-by: iklam, matsaave, dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 589 new commits pushed to the master branch:

• c266800: 8325558: Add jcheck whitespace checking for properties files
• ec20b0a: 8325626: Allow selection of non-matching configurations using CONF=!string
• 618af39: 8325633: Use stricter assertion in callers of Space::is_aligned
• 5dbf137: 8319797: Recursive lightweight locking: Runtime implementation
• 4513da9: 8325470: [AIX] use fclose after fopen in read_psinfo
• 62a4be0: 8325635: Serial: Inline verify_used_region_at_save_marks
• b356fee: 8325458: Rename mlvmJvmtiUtils.h
• 2ed889b: 8323628: Update license on "pass-through" files
• 7c69712: 8325570: Update to Graphviz 9.0.0
• 482c100: 8322865: JavaDoc fails on aggregator modules
• ... and 579 more: https://git.openjdk.org/jdk/compare/459957f30a6e0fe40636dd72faa3f0d86151c94f...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[matias9927]

Looks good! Thanks @iklam for the explanation.

[dholmes-ora]

Yes, this changes behavior. However, we run a very limited set of Java code during "java -Xshare:dump". Specifically, we never execute any user-supplied code. So even if we perform the real checks, we should always get ACCESS_OK anyway. If we don't, that will be a bug, and we better know about it.

Seems to me like this is/was simply a quick exit path: we know/expect all checks to pass so why bother actually performing them. I'm fine with now actually performing them, in case at some point something is added that could fail, as long as we don't expect anything to actually start failing now.

Thanks.

[calvinccheung]

Thanks @dholmes-ora, @iklam, @matias9927 for the review.

/integrate

[openjdk]

Going to push as commit 221fbfb.
Since your change was applied there have been 690 commits pushed to the master branch:

• d2590c6: 8325730: StringBuilder.toString allocation for the empty String
• aa792ea: 8164094: javadoc allows to create a @link to a non-existent method
• a2d027e: 8326089: Text incorrectly placed in breadcrumbs list in generated docs
• b419e95: 8294148: Support JSplitPane for instructions and test UI
• 9538f5d: 8319651: Several network tests ignore vm flags when start java process
• 4957d6a: 8325441: IGV: update pom.xml such that IntelliJ can import as maven project
• d60331a: 8326196: Serial: Remove SerialHeap::generation_iterate
• beed414: 8326209: G1: Remove G1ConcurrentMark::_total_cleanup_time
• 56073cd: 8326171: Serial: Remove VerifyGCLevel
• bb675af: 8325542: CTW: Runner can produce negative StressSeed
• ... and 680 more: https://git.openjdk.org/jdk/compare/459957f30a6e0fe40636dd72faa3f0d86151c94f...master

Your commit was automatically rebased without conflicts.

[openjdk]

@calvinccheung Pushed as commit 221fbfb.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.